bug#39962: 27.0.90; Crash in Emacs 27.0.90

[Pip Cet <pipcet@gmail.com>]

On Thu, Mar 12, 2020 at 3:23 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > > > > Did you audit all the users of this function, both direct and
> > > > > indirect?  Some of them are outside of GC.
> > > >
> > > > Thanks for the comment; I just re-checked, and they look fine to me.
> > >
> > > ??? Fine in what way?
> >
> > It doesn't affect visible behavior of any callers, except in the case
> > where the previous behavior was buggy.
>
> I guess we have different notions of "visible"

Please say something about your notion of "visible". It doesn't affect
any of the existing C callers of valid_lisp_object_p. Are you talking
about printing valid_lisp_object_p(x) in a debugger, and not getting
the expected value? Or something else?

> and "buggy".

It avoids segfaults or random memory corruption. How is that not "buggy"?

> > I'm most certainly not changing the semantics of live_buffer, if
> > that's what you're worried about. I am changing the semantics of
> > live_buffer_p, which is an internal function, and my initial patch
> > also changed the return value of valid_lisp_object_p, to another value
> > that would be treated equivalently. If there are objections to that,
> > we can easily distinguish the two cases.
>
> I actually don't understand why we need to make such a change.

Which change? Treating the two cases differently? Because the garbage
collector needs to know whether an object is reachable, not whether
it's still a live buffer.

valid_lisp_object_p is currently documented to return 2 for a killed
buffer and 1 for a live buffer, which is weird since they're both
valid. It also returns 1 for some fake objects which aren't actually
valid:

(gdb) p current_thread->m_current_buffer
$3 = (struct buffer *) 0x555556694b10
(gdb) p valid_lisp_object_p(0x555556694b15)
$4 = 1
(gdb) p valid_lisp_object_p(0x555556694b25)
$5 = 1

Luckily, no one relies upon that documented mis-feature, so it's safe
to remove it.

> > And I think "so we don't collect reachable objects" is a fairly good
> > reason, generally.
>
> I didn't say it wasn't good, I said it didn't justify the proposed
> solution.

> How about if you tell more about the root cause of the crash you are
> trying to solve, and why disregarding the fact that a buffer is killed
> is the way to solve it?

I can try.

The garbage collector needs to mark all reachable objects. It can get
away with marking unreachable objects (and does so, for overlays in
killed buffers), but not marking a reachable object is a serious bug.

If a buffer is live, everything is fine.

If a buffer has been killed but is unreachable, everything is fine; it
will be collected by GC.

If a buffer has been killed but is reachable through mark_object,
everything is fine.

If a buffer has been killed but is reachable only through
mark_maybe_object, we fail to mark it.

We should mark it. In fact, whether a buffer object is marked should
depend only on whether it's reachable, not whether it's "live" in some
other sense.

That's all my patch does.

> > > The problem you are trying to solve is rare
> >
> > I think it would become much less rare with lexical binding in effect,
> > at least when the code's byte-compiled.
>
> That remains to be seen.

How about we put out the fire rather than waiting to see whether it
causes any damage? And, if we can agree to do so, what would you like
a patch which is actually meant for inclusion into the emacs-27 branch
(my previous patch wasn't, obviously) to look like?

> > > since this code was with us since 20 years ago without
> > > anyone bumping into it,
> >
> > That we know of. They might have just accrued it to random Emacs crashes.
>
> Then again, they might not.  We don't really have any evidence to that
> effect, all we know is that the code survived virtually intact since
> the day it was written.

I have no idea what you're trying to get at here.