Re: [feature/igc3] weak hash table lookup can return a finalized (PVEC_FREE) object

> Is the fix LLM-produced as well?

100%

> Just to make sure I understand, when you say they were "splatted", you
> mean that their finalizers were called, but that they weren't removed
> from the hash table, correct?

Sorry, that's LLM wording. I should add, both Emacs internals
and GC's are beyond my current knowledge so I can't offer much reasoning.

I can however bridge us back to the session with the full investigation,
so the following are once more LLM answers.

But first:

> `--enable-igc-ephemeron-pool=yes`

It worked to start my usual Doom Emacs configuration. I didn't drive it around
much but so far, so good.

<< LLM:

> splatted

Yes.  To avoid confusion with MPS nulling a weak reference, I use
"splat" only for igc's splat_pvec.  The finalizer ran: finalize()
restored the header, freed the exthdr, and splat_pvec set the type to
PVEC_FREE.  The object's slots were intact.  The table entry was not
removed.  Its key still matched under `equal', so gethash returned the
PVEC_FREE object.  The core dump shows exactly this state: a
Weak_Value/equal table with two value slots that point to PVEC_FREE
closures, next to a live key.

> I'm not sure I follow: conses, strings, and symbols need no
> (Emacs-style) finalization, do they? Can you describe this bug in more
> detail?

Point withdrawn. Every weakly held object
gets an exthdr and an mps_finalize registration, whatever its type.
For a cons, string or symbol, finalize() restores the header and does
nothing else.  The object stays intact.  If a lookup returns it in the
window, it becomes reachable again and the next trace keeps it.  That
is a one-cycle delay in eviction, not a correctness problem.  The
defect only affects objects that splat_pvec mutates.

> If we cannot live with the limitations of the current approach, I think
> adding eager finalization to MPS is the easiest way forward, and the
> performance hit should be acceptable: it's a lot cheaper than MPS
> finalization.

I cannot judge the MPS cost.  The invariant any fix needs: a table
must not return an entry after its object is splatted.  Three ways to
get it:

  (a) Filter at lookup (the patch).  Closes the Lisp-visible hole.  The
      entry itself stays one more cycle.
  (b) Remove the entry in finalize(), before the splat.  The exthdr
      already records the tables and the partner key or value
      (weak_hash_put passes them to Figc__add_extra_dependency).  No
      MPS change.  Untested.
  (c) Change MPS so that it nulls weak references to a finalized object
      in the same trace.  I take this to be "eager finalization".

Today (c) does not happen.  In the reproducer, one igc--collect leaves
all 100 entries intact.  igc--process-messages splats the values, and
all 100 lookups still match.  A second igc--collect removes the
entries.

> What is "incorrect" with MPS finalization?  That is not synchronous or
that is inefficient?

This changes when the object is splatted, not when the entry is
removed.  MPS nulls the weak reference during a weak scan, in a later
trace.  Message timing does not move that.  So faster message
processing does not close the window.

For this bug it makes the window larger, not smaller.  The crash needs
the object to be already splatted while it is still in the table.
Today, messages are processed lazily, so the object often keeps its
real type for a while.  A lookup then returns a valid closure, which is
harmless.  If finalizers run right after mps_commit, the object is
splatted at once.  The dangerous state (splatted, still in the table)
starts sooner and lasts until the next trace.

So faster message processing is safe for this bug only together with
the second idea below.  It can still help for fonts, threads and
user-ptrs, which have no such table entry.

> - It may be worth asking whether the entry can be removed at the same
>   time as the splat, rather than a cycle later, which would close the
>   window instead of papering over it.

Yes.  This closes the window at the source, and it needs no MPS change.

The information is already present.  weak_hash_put records the partner
on the finalizable object: for a Weak_Value table it calls
Figc__add_extra_dependency(value, key, table).  So the value's external
header maps each table to the keys that point to it.

finalize() already reads the external header (it restores the inline
header and frees the exthdr).  It could, before splat_pvec, walk
exthdr.extra_dependency and remove each (table, key) entry directly.
finalize() does not do this today.

The removal mutates the table during message
processing, which is the same re-entrancy concern as the existing
"other threads should be suspended while finalizing" FIXME.  And
extra_dependency is itself a weak table with its own cleanup FIXME.