bug#39962: 27.0.90; Crash in Emacs 27.0.90

[Pip Cet]

On Fri, Mar 13, 2020 at 9:40 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > > > 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?
>
> I'm talking about the behavior documented in the commentary.

You're right if your point is the comment should be adjusted to omit
the unnecessary, and unused, special behavior on killed buffers.

> > > and "buggy".
> >
> > It avoids segfaults or random memory corruption. How is that not "buggy"?
>
> That's not the issue here.  You said the proposed change didn't change
> the behavior "except where it was buggy"; I'm saying that it changes
> the behavior unrelated to this bug, where previous behavior was not
> buggy by any measure.

How so? Can you describe a scenario in which Emacs would behave at all
differently? valid_lisp_object_p returns a different value, sure; but
none of its callers care about the difference, so Emacs behavior
overall does not change.

> So why does it not consider the buffer reachable in this case?  The
> call to live_buffer_p is just one attempt to identify it as reachable;
> there are (or at least should be) others.

I don't think there are, no. This is the one shot we get at protecting
a stack slot that might contain the sole reference to a killed 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
>
> Why do you consider this incorrect?  The Emacs GC is "conservative",
> which means it doesn't collect anything that _might_ be a valid Lisp
> object.  In what ways does the above violate that contract?

GC is conservative; valid_lisp_object_p is documented to be precise: a
return value of 1 or 2 means that the object is valid, not that it's
potentially valid and potentially nonsense.

> > 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.
>
> Your patch modifies the notion of whether a buffer is "live",

No, it modifies a specific function (mis)named buffer_live_p. The
dozens of places in which we check whether a buffer is "live", as
opposed to "killed", are unaffected. Only GC is affected.

> on the
> assumption that this is the root cause of the failure to mark it.

I'm not sure about the philosophical implications of "root cause", but
this is a very obvious bug.

> But do we have any evidence that this is the root cause?

What kind of evidence do you want?

A buffer should be marked iff it is reachable

A buffer is marked iff it is reachable from the heap or it is
reachable from the stack and buffer_live_p returns true

Therefore, it is invalid for buffer_live_p to return false for a
buffer which is reachable from the stack.

> Moreover, by disregarding the indication of a killed buffer, doesn't
> your patch cause us not to GC killed buffers even though they are
> unreachable, or at least create a danger that we would?

Only in the rare case that they appear to be reachable through a stack
reference but actually aren't, but that's just the price we pay for
conservative GC.

> The way to understand what happened in your test case is to figure out
> how come the buffer was not found to be reachable via any other
> approach the GC makes.

There is no other approach.

> For example, shouldn't we have this buffer
> somewhere on the stack?

Precisely.

> if so, how come stack marking didn't find it?

Because we are talking about the stack marking! The stack marking
calls buffer_live_p to check whether it should actually mark the
buffer or not.

> And if we don't have it on the stack, why not?

We do.

> > How about we put out the fire rather than waiting to see whether it
> > causes any damage?
>
> The disagreement is whether there's fire, not whether we should put it
> out if there is.  You've shown that you can start a fire if you want,
> but not that the fire is already out there, burning.  E.g., I see no
> reason for some Lisp program to do what your test case does, it simply
> makes no sense.

How does it not make sense? We kill a buffer and return it.

> > 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?
>
> If it isn't clear, I'm saying that your proposed patch is not
> necessarily TRT for master, either.  I'd like to see more analysis of
> what exactly happens in that case, and why, along the above-mentioned
> lines, before I make up my mind.

It's certainly not the right thing for master! Using "live" in two
different senses like that is way too confusing for a code base that
is still being worked on.

Again, I think we're being distracted from a very simple issue: stack
marking relies on recognizing reachable objects. Reachable objects are
called "live" in GC code, so the stack marking code that calls
buffer_live_p clearly expects a return value indicating whether the
pointer it passed in points into a buffer object; it doesn't, and
shouldn't, care whether that buffer has been killed or not.