Subject: bug#76091: 31.0.50; festure/igc: buffer.h:829: Emacs fatal error: assertion failed: BUFFERP (a)

From: Pip Cet <[email-masked]>
Date: Sat, 08 Feb 2025 12:28:12 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Date: Sat, 08 Feb 2025 10:24:01 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked], [email-masked], [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> >> Date: Fri, 07 Feb 2025 19:43:18 +0000
>> >> From: Pip Cet <[email-masked]>
>> >> Cc: Helmut Eller <[email-masked]>, Gregor Zattler <[email-masked]>, Eli Zaretskii <[email-masked]>, [email-masked]
>> >>
>> >> Gerd Möllmann <[email-masked]> writes:
>> >>
>> >> >>> Well, yes.  I'll split it up into individual patches, so we can review
>> >> >>> them one by one?
>> >> >>
>> >> >> Not necessary, but thanks.
>> >> >
>> >> > I'm through. Except for the shrinking case that you already mentioned I haven't found other problems.
>> >> >
>> >> > So LGTM.
>> >>
>> >> Thanks!
>> >>
>> >> Hope I didn't break anything, but I just pushed the series.  Please feel
>> >> free to revert or fix as appropriate!
>> >
>> > Here I get compilation warnings:
>> >
>> >   igc.c: In function 'igc_realloc_ambig':
>> >   igc.c:3324:55: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3324 |   ptrdiff_t old_size = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                                   ~~~~^~~~~~
>> >   igc.c:3324:36: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3324 |   ptrdiff_t old_size = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                ~~~~^~~~
>> >   igc.c:3324:55: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3324 |   ptrdiff_t old_size = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                                   ~~~~^~~~~~
>> >   igc.c:3324:36: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3324 |   ptrdiff_t old_size = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                ~~~~^~~~
>> >   igc.c: In function 'igc_xpalloc_exact':
>> >   igc.c:3397:27: warning: argument to variable-length array may be too large [-Wvla-larger-than=]
>> >    3397 |       volatile mps_word_t area[(item_size + (sizeof (mps_word_t) - 1))
>> > 	|                           ^~~~
>> >   igc.c:3397:27: note: limit is 4031 bytes, but argument may be as large as 4294967292
>> >   igc.c: In function 'igc_xnrealloc_ambig':
>> >   igc.c:3415:57: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3415 |   ptrdiff_t old_nbytes = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                                     ~~~~^~~~~~
>> >   igc.c:3415:38: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3415 |   ptrdiff_t old_nbytes = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                  ~~~~^~~~
>> >   igc.c:3415:57: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3415 |   ptrdiff_t old_nbytes = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                                     ~~~~^~~~~~
>> >   igc.c:3415:38: warning: potential null pointer dereference [-Wnull-dereference]
>> >    3415 |   ptrdiff_t old_nbytes = (char *)r->d.end - (char *)r->d.start;
>> > 	|                                  ~~~~^~~~
>> >
>> > Most of them seem to be because root_find can return NULL.  The nit
>>
>> Those are harmless; I'll add an eassert (r), that should prevent the
>> warnings (together with Gerd's fix for the igc_realloc (NULL) case).
>
> Not sure if you already did, but:
>
>  . I still see the warnings, and
>  . this is an optimized build, so eassert compiles to nothing

Oh, thanks! Yeah, I can see them in optimized builds, and they require
eassume, not eassert.  I'll suggest a patch once I've thought of a good
way to avoid those, but for now, we just might have to ignore the
warnings.

>> > Also, does this:
>> >
>> >> (Of course, with WIDE_EMACS_INT, mps_word_t is not good enough; unless
>> >> we carefully write the scan function not to assume that the two
>> >> half-words comprising a 64-bit Lisp_Object are in sync.  As Eli was very
>> >> opposed to the idea of removing WIDE_EMACS_INT again, we might have to
>> >> find a workaround there.)
>> >
>> > mean that the wide-int 32-bit build is now in trouble?
>>
>> Yes, it does.  All scanning functions need to be written to deal with
>> half-written Lisp_Object values in that case.  (Easier than it sounds
>> because we USE_LSB_TAG).
>>
>> > Or did you install some workaround for the above?
>>
>> Not yet :-)
>
> Hopefully, soon then?

I don't think there are any fundamental problems there, but it needs
some time and the setjmp() issues seem more important right now.

(Two issues: nativecomp uses %rbp and may call setjmp directly, which
scrambles the %rbp value before saving it in a jmp_buf.

And it seems that push_handler_nosignal allocates the jmp_buf from a
non-root but doesn't trace it in any way whatsoever. I have no idea why
that isn't causing more frequent crashes....)

Pip