runtime: fix gc issue, deref() can trigger gc

[Jafaral]

@cjeffery , the comment at the top of the function says nothing to be tended. But if I recall, deref() can allocate. Am I missing something?

[Don-Ward]

@cjeffery , the comment at the top of the function says nothing to be tended. But if I recall, deref() can allocate. Am I missing something? Using handy command git -L: The comment was introduced by 984fa078 05-May-2001 | Introduction of the Unicon source tree onto Source Forge. [Clinton Jeffery] and, at that point, was correct -- there were no allocations. It was rendered incorrect by f2a31ac9 21-Mar-2002 | list allocation optimization; dbm subscript support [Clinton Jeffery] where an alcstr call was made (albeit protected by #ifdef Dbm). It remained like that until 0fa15a6f 22-Jan-2010 | more support for int-real arrays as part of the list data type + initial data parallel for arrays [Jafar Al-Gharaibeh] where a further alcReal call was added. And it's been like that (with two calls to an allocation routine) more or less ever since.

So, no I don't think you missed anything. The only case where the comment is correct is if Dbm is not defined and DescriptorDouble is defined.

[Jafaral]

@Don-Ward , so not only deref() that allocates, but we have more allocation now. DBM does use bp, but arrays don't, so at least arrays not affected. In all cases, looks like this fix should go in. What to merge?

[cjeffery]

we definitely don't want untended variables living through a GC. And I confirm Don's excellent analysis. Was one of these untended variables discovered the hard way, or is this a by-product of code review, or what?

Some additional thoughts. deref() is called very often and is performance critical. Correctness is the most important thing, but tending is costly and we don't want to tend these variables except when absolutely necessary. It is not like we ever turn off Dbm and not like we can require DescriptorDouble of all implementations from here on out, so #ifdef's will not save us. What might save us if if we can move tending inside blocks where it doesn't happen for the frequent cheap deref's, it only happens for expensive derefs. What else might save us is if the "needs tending" variable isn't actually required to survive across an allocation -- if we can guarantee it is unused/dead after the allocation, or can easily be reinitialized after an allocation and before it is used again.

So for instance, if after the alcstr, bp and v are never used again, then no tending needed, right? What about the cnv:string() earlier in that function -- it can allocate and it is really there that bp needs tending. I would put bp back to untended but unless we change the cnv:string to a cnv:tmp_string we need to create a tended union block *tbp = bp; inside the if (status & Fs_Dbm) so that only Dbm deref uses have to pay for the safety here.

For the alcreal(), does bp or v ever survive (get referenced) after the alcreal()? I don't see a path where it can.

To sum up: let's avoid tending in defer() and preserve original speed for the common/original cases, and tend under Dbm and maybe not under #ifndef DescriptorDouble.

[Jafaral]

It was discovered as part of a GC crash. We are chasing another bug but don't know the root cause yet. If you want to open a PR with your suggested changes I'd happily test it @cjeffery