bug#39962: 27.0.90; Crash in Emacs 27.0.90

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 13 Mar 2020 13:56:07 +0000
> Cc: pieter-l@vanoostrum.org, 39962@debbugs.gnu.org, eggert@cs.ucla.edu
> 
> 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.

I don't yet think that function's behavior should be changed.  See
below.

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

The behavior of live_buffer_p and valid_lisp_object_p changed, and
those functions weren't "buggy" before.

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

I wasn't talking about behavior of Emacs as a whole.

And I don't understand why you are arguing about this.  You asked me
to say something about my notion of "visible", and I did.  Will
arguing about _my_ notion of that get us to some useful place?

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

But what does "valid" mean in this case?  The part that looks at the
stack uses the stack-marking routines, and thus inherits the
"conservative" nature of stack marking.  The code also makes it quite
clear that it only considers "live" objects as valid, and a killed
buffer is not "live".  So I still don't understand in what way the
above results are incorrect.

> > Your patch modifies the notion of whether a buffer is "live",
> 
> No, it modifies a specific function (mis)named buffer_live_p.

Which, among other things, checks whether a buffer is "live".  So it
is not necessarily mis-named.

> The dozens of places in which we check whether a buffer is "live",
> as opposed to "killed", are unaffected. Only GC is affected.

No, not only GC is affected.  Some of the callers are outside GC,
we've been there up-thread, and agreed about that.

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

This mixes two notions: a "live" buffer and a buffer that should be
marked.  They are not the same.

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

Fine, so you are saying that stack marking should disregard whether a
buffer is "live"?  Then let's make such a change only for stack
marking, not in a function called from other places.