kernel: streamline weak pointer object code; fix MakeImmutable for weak pointer objects

[fingolfin]

While preparing PR #2092 for merging, I noticed that there was a problem with the weak pointer object code in it. Also, @ChrisJefferson made some good suggestions on how to minimize the difference for the wp code in GASMAN and Julia.

The result is this PR, which cleans up the weak pointer object code. Most of the changes in here actually already wer in PR #2092, but intermingled with Julia GC specific code.

My hope is that we can merge this PR speedily, and then afterwards merge PR #2092 (which I will shortly rebased based on this PR).

[stevelinton]

This comment seems to no longer be correct. While a garbage collection might occur almost immediately after a non-zero object has been returned, the reference from the C stack should keep it alive, so with this change it is safe to assume that any object you get back is real.

[fingolfin]

Oops indeed. I could have sworn that I already removed this...?!? Thanks, will fix it ASAP

[stevelinton]

Do we need to recompute the length here? If a GC somehow managed to happen between the line 718 and here, then the last item in the weak pointer object might have evaporated, and len will now be too large. So the question is whether MakeImmutable can cause a garbage collection, which I think it probably can, thanks to PostMakeImmutable calls.

[fingolfin]

You are right. And it's worse: if a GC happens, then it may mark objects we already touched as halfdead (with GASMAN), or crash (in Julia GC, because we now have a partially invalid object).

One way to fix this is to first convert to a mutable plist (which causes no GC), then make that immutable). I'll look into it.

[fingolfin]

@stevelinton updated the MakeImmutable code, and added a test for recursive MakeImmutable. The HPC-GAP version contains a race (for which I added a FIXME), but so does its implementation for plists... One idea to fix that would to use SET_OBJ_FLAG with a new flag, say OBJ_MAKEIMMUTABLE_IN_PROGRESS (better name pending...) which we set right at the start of the MakeImmutable function. Then, use that to avoid recursion; make sure it is reset after MakeImmutable finished (although I guess that doesn't really matter either way).

BTW, is there a reason the WP object code uses GAP level InstallMethod calls to install handlers for []. []:=, IsBound etc., instead of just adding handlers to the relevant kernel tables? I am tempted to make that change (in a separate future PR, not here!), but was wondering if there are deeper reasons not to do this?

[stevelinton]

@fingolfin I don't recall any reason why the weak pointers don't use the kernel tables, and I can't think of one now.

[fingolfin]

@stevelinton Excellent!

Any remarks on the new MakeImmutable, and the idea for the object flag?

[stevelinton]

@fingolfin I'm not completely sure I understand the problem. Surely, only one thread can have write access to the weak pointer object or to the mutable non-atomic Plist it turns into. The only thing you need to do is make sure the subobjects are all made immutable (and therefore globally readable) BEFORE you make the list itself immutable. Or have I missed something?

[fingolfin]

Hmm, you are probably right: as long as the wp object is non public, it should be safe to first turn it into a mutable plist, and then make that mutable plist immutable via a call to MakeImmutable.

But note that it is problematic to make the subobjects immutable BEFORE the WP object: after all, the WP object may be its own subobject, directly or indirectly. So, if we make its subobjects immutable before making itself immutable, we run into an infinite recursion crash. Similar problems existed for plists and precords.

I punt on this now by first converting to a plist, and then making that plist immutable. But the plist code still has a hypothetical problem there (see the FIXME comment in MakeImmutablePlistInHom).

[fingolfin]

OK, I adjusted the comment, and also fixed yet another bug in the HPC-GAP version of MakeImmutableWPObj, which could lead to the length being computed incorrectly.

[stevelinton]

A wp object can't be public, I think, any more than a mutable plist can.
MakeImmutable on a cyclic object in HPC-GAP is going to be tricky. You'd need to get an exclusive lock on the whole cycle first, and then migrate them all to the public or read-only region (if appropriate) atomically. Otherwise you risk another thread either being able to access an immutable object with a mutable subobject, or being able to access an immutable object but not access one of its subobjects.

[fingolfin]

@stevelinton right, all good points... which I another reason for why I am not even trying to address this myself here, and instead delegate to the MakeImmutable function for plists (which also doesn't get it right, but at least now we only need to address this in one place less).

Anyway, I think there's no need to worry about HPC-GAP at this point, at least not in regards to this PR.

But for anybody who wants to see Julia integration move forward, we should look into getting this PR approved and merged (or alternatively, request changes, which I can then address)...