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

<[email protected]> writes:

> Hi. This is about an immediate crash upon startup with igc3 and doom emacs.

Hi!

> What follows is a LLM-produced report and attached a suggested
> fix that has been working stable for a few weeks now.

Is the fix LLM-produced as well?

> I can reproduce reliably and retrieve more debug information if needed.
> Given the report and fix size I hope this is helpful.

Certainly!

> ----------------
>
> Branch feature/igc3, commit 07b35c82b7d ("Move 'read_commit_limit' to
> sysdep.c"), x86_64-pc-linux-gnu, --with-mps.
>
> A lookup in a weak hash table can return an object that the collector
> has already finalized and splatted into a PVEC_FREE.  Lisp then sees a
> "function" that is not a function, and printing the resulting error
> aborts Emacs.

Indeed, but the actual problem is slightly more complicated: in most GC
systems, including pre-MPS Emacs, "finalization" happens when an object
is about to be destroyed: it's the last thing to happen to an object.

In MPS, however, "finalization" only means that only weak references to
an object remain; this triggers the finalization code, but there is no
guarantee that a new strong reference to the object hasn't been created
by the time the finalization code is actually called (in MPS, this
happens asynchronously, and the mechanism to do so is quite expensive).

The MPS approach has its advantages (in particular, you can veto the
actual destruction of an object in the finalizer, which is useful for
debugging).

We're currently pretending that MPS finalization is the right thing for
Emacs finalization to use, but this isn't true: IMHO, the correct fix is
to add last-thing-to-happen-to-an-object finalization to MPS. I have
code somewhere to do that, synchronously, when we actually free the
memory associated with an MPS object (instead of copying it).

However, I've never been quite happy with it, because it means that we
occasionally walk a segment once more than previously, to find objects
in it that need to be finalized. That's a lot cheaper than the current
approach, but it also happens when Emacs isn't idle, which is a bad
time.

All that said, your fix looks like it would definitely improve things,
and I rather suspect I meant to do something like it but forgot to add
the actual code.

> How I ran into it: Emacs crashed about a second into starting a Doom
> Emacs configuration, every time.  The victim was a combined method in
> cl--generic-combined-method-memoization, which is
>
>     (make-hash-table :test #'equal :weakness 'value)
>
> Two of its values are splatted on every startup (the cl-generic-
> generalizers methods for EIEIO defclass and for cl-deftype derived
> types).

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?

> gethash returned one of them, funcall signalled
> (invalid-function #<splatted closure>), and the error handler formatted
> the error with %s, which reached print_vectorlike_unreadable and
> emacs_abort.  In the core dump the table's kv.values vector still holds
> two PVEC_FREE objects, next to a live key
> (#s(cl--generic cl-generic-generalizers ...) . METHODS).

> Proposed fix, attached as igc-weak-hash-fix.diff: skip dead entries in
> the lookup, the way DOHASH_WEAK already does.

Ah, so that's what I actually did :-)

> With this, both reproducers above are clean (gethash reports a miss) and
> the Doom configuration starts normally.  I instrumented the two skips
> before removing the counters: exactly two lookups per startup matched a
> key whose value had been splatted, reproducibly, on every start.
>
> Two further points, which I have not tried to fix:
>
> - finalize (src/igc.c) only dispatches to finalize_vector; everything
>   else falls through "default: break".  So the PVEC_FREE marker exists
>   only for pseudovectors.  A weak table whose weakly held side is a
>   cons, string or symbol goes through the same window with no marker at
>   all, and neither DOHASH_WEAK nor the check above can detect it - the
>   lookup will hand back an object that should have been evicted.

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?

> - 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.

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.

> The attachment also contains a second, optional hunk: making
> print_vectorlike_unreadable print "#<dead object>" for PVEC_FREE under
> HAVE_MPS instead of calling emacs_abort, so that this class of bug does
> not turn a Lisp-level error into a crash while the error is being
> printed.

If that helps debugging things, I think we should apply both.

Thanks!

Pip