Subject: Re: bug#75632: 31.0.50; igc: Crash report

From: Pip Cet <[email-masked]>
Date: Fri, 17 Jan 2025 20:52:58 +0000

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

>> Resent-To: [email-masked]
>> Cc: Gerd Möllmann <[email-masked]>,
>>  [email-masked]
>> Date: Fri, 17 Jan 2025 15:26:10 +0000
>> From:  Pip Cet via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <[email-masked]>
>>
>> "Ihor Radchenko" <[email-masked]> writes:
>>
>> > Pip Cet <[email-masked]> writes:
>> >
>> >> "Ihor Radchenko" <[email-masked]> writes:
>> >>
>> >>> Just got the following:
>> >>
>> >> Yes, that's the signal handling bug.  I'm not entirely sure why we
>> >> removed the fix that was in scratch/igc, but we did, so we need another
>> >> one.
>> >> ...
>> >> to handle_user_signal, which will silently ignore SIGUSR* received while
>> >> MPS may have locked the arena.
>> >
>> > I indeed sent SIGUSR2 just before I saw the crash.
>>
>> It'd be nice if that simply worked.  I think debugging with SIGUSR* is
>> important, so I've pushed a fix (and I'm closing this bug; if further
>> discussion is needed, feel free to revert and reopen).
>
> This breaks the MS-Windows build: temacs crashes as below:

Sorry.  Can you try the fix I just pushed?  While I arrived at that
independently (I was about to push when I got your email), I believe it
explains your crash as well.

>   (gdb) bt
>   #0  terminate_due_to_signal (sig=sig@entry=22,
>       backtrace_limit=backtrace_limit@entry=2147483647) at emacs.c:432
>   #1  0x00f836f4 in die (
>       msg=msg@entry=0x15369e9 <gdb_make_enums_visible+1745> "should_end >= end",
>       file=file@entry=0x1536431 <gdb_make_enums_visible+281> "pdumper.c",
>       line=line@entry=939) at alloc.c:8388
>   #2  0x00f88e8a in dump_igc_finish_obj (ctx=ctx@entry=0xbfebd8) at pdumper.c:939

This function calls igc_dump_finish_obj, which returns 0 because the GC
header for the static subr hasn't been initialized, and then causes the
assertion error.

> The following patch seems to fix that, but I'm not sure it's the
> correct fix.  Can you explain why we need to add-variable-watcher for
> this in init_keyboard, in particular in temacs?

I'm not sure what you are suggesting here.  Is there a good place that
is initialized only in the !pdumper || !temacs case?

> since init_keyboard
> is called both in temacs and in emacs, it sounds like the code, when
> called in emacs, will try to free memory that was malloced in temacs,
> is that right?

I don't think so, but it might call Fadd_variable_watcher twice.  We
should avoid that.

> diff --git a/src/keyboard.c b/src/keyboard.c
> index 82505e7..a5d85e9 100644
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -12817,6 +12817,7 @@ init_keyboard (void)
>  #endif
>
>  #ifdef HAVE_MPS
> +  if (initialized)
>    {
>      Lisp_Object watcher;

I can add that (with a FIXME comment because it breaks the !pdumper
case, which is currently an #error but appears to work).

Pip