GNU bug report logs - #74224
[PATCH][scratch/igc] pgtk: fix crashing bug in atimer used for checking scaling

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74224 in the body.
You can then email your comments to 74224 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Fejfighter <fejfighter <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH][scratch/igc] pgtk: fix crashing bug in atimer used for
 checking scaling
Date: Wed, 6 Nov 2024 19:04:42 +1100

[Message part 1 (text/plain, inline)]

This patch marks 2 outstanding ambiguous roots and appears to solve
crashing bugs I had been experiencing with igc/mps and pgtk.

I have run this locally today, and I would have normally faced crashes at
timer expiration, this appears to be holding up.



In GNU Emacs 31.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
 3.24.43, cairo version 1.18.0) of 2024-11-06 built on solidus.local
Repository revision: 96de0bf0ba9161af5d3f783b45a5a9de530b6f95
Repository branch: scratch/igc
System Description: Fedora Linux 41 (Sway)

Configured using:
 'configure --with-pgtk --with-mps CFLAGS=-O2'

[Message part 2 (text/html, inline)]

[mps-igc-pgtk.patch (text/x-patch, attachment)]

Message #8 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Fejfighter <fejfighter <at> gmail.com>
Cc: 74224 <at> debbugs.gnu.org
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in
 atimer used for checking scaling
Date: Wed, 06 Nov 2024 09:56:18 +0100

Fejfighter <fejfighter <at> gmail.com> writes:

> This patch marks 2 outstanding ambiguous roots and appears to solve
> crashing bugs I had been experiencing with igc/mps and pgtk.
>
> I have run this locally today, and I would have normally faced crashes
> at timer expiration, this appears to be holding up.

Thanks for the report, Jeff! Nice to see that someone besides me is
using this :-).

I think I see why the change in atimer.c is necessary: pgtk stores a
struct frame * as client_data in an atimer structure. That's a Lisp
object that can move during GC. Understood.

The other change in pgtkterm.c I don't understand. AFAICS, x_id_name of
the display_info structure is indeed only used as a character buffer
into which characters from Lisp strings are memcpy'd. Could you please
explain that one? (I'm macOS only, so I don't know anything about pgtk,
if that matters.)

Message #11 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Jeff Walsh <fejfighter <at> gmail.com>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: 74224 <at> debbugs.gnu.org
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in atimer
 used for checking scaling
Date: Wed, 6 Nov 2024 20:30:02 +1100

[Message part 1 (text/plain, inline)]

Hi Gerd,

No problems, I'd love to see this go in one day, it's just hard enough to
carve out time to debug crash dumps

I got overzealous with the change in pgtkterm.c, with the call from
`lispstpcpy` (and your explanation there makes sense)
looking at xterm, I see we have not marked that as a root , which is where
I think that code was ported from originally.

I have attached an updated patch, using a rebuilt igc-emacs from that
commit, which is holding up.

Thanks,


On Wed, Nov 6, 2024 at 7:56 PM Gerd Möllmann <gerd.moellmann <at> gmail.com>
wrote:

> Fejfighter <fejfighter <at> gmail.com> writes:
>
> > This patch marks 2 outstanding ambiguous roots and appears to solve
> > crashing bugs I had been experiencing with igc/mps and pgtk.
> >
> > I have run this locally today, and I would have normally faced crashes
> > at timer expiration, this appears to be holding up.
>
> Thanks for the report, Jeff! Nice to see that someone besides me is
> using this :-).
>
> I think I see why the change in atimer.c is necessary: pgtk stores a
> struct frame * as client_data in an atimer structure. That's a Lisp
> object that can move during GC. Understood.
>
> The other change in pgtkterm.c I don't understand. AFAICS, x_id_name of
> the display_info structure is indeed only used as a character buffer
> into which characters from Lisp strings are memcpy'd. Could you please
> explain that one? (I'm macOS only, so I don't know anything about pgtk,
> if that matters.)
>

[Message part 2 (text/html, inline)]

[0001-Mark-atimer-allocation-as-ambiguous-root.patch (text/x-patch, attachment)]

Message #14 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Jeff Walsh <fejfighter <at> gmail.com>
Cc: 74224 <at> debbugs.gnu.org
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in
 atimer used for checking scaling
Date: Wed, 06 Nov 2024 11:34:54 +0100

Jeff Walsh <fejfighter <at> gmail.com> writes:

> I have attached an updated patch, using a rebuilt igc-emacs from that
> commit, which is holding up.

LGTM. Can you commit that yourself to the branch?

Message #17 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Jeff Walsh <fejfighter <at> gmail.com>
Cc: 74224 <at> debbugs.gnu.org
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in
 atimer used for checking scaling
Date: Wed, 06 Nov 2024 11:36:29 +0100

Jeff Walsh <fejfighter <at> gmail.com> writes:

>  #ifdef WINDOWSNT
>  #define raise(s) w32_raise(s)
> @@ -132,7 +133,13 @@ start_atimer (enum atimer_type type, struct timespec timestamp,
>        free_atimers = t->next;
>      }
>    else
> -    t = xmalloc (sizeof *t);
> +    {
> +#ifdef HAVE_MPS
> +      t = igc_xzalloc_ambig (sizeof *t);
> +#else
> +      t = xmalloc (sizeof *t);
> +#endif
> +    }
>  
>    /* Fill the atimer structure.  */
>    memset (t, 0, sizeof *t);

On second thought, and I don't know if it's relevant, do we need to
igc_xfree that?

Message #20 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Jeff Walsh <fejfighter <at> gmail.com>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: 74224 <at> debbugs.gnu.org
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in atimer
 used for checking scaling
Date: Wed, 6 Nov 2024 21:44:57 +1100

[Message part 1 (text/plain, inline)]

> LGTM. Can you commit that yourself to the branch?

I don't have a savannah account for emacs, but I have just registered and
will chase up approvals.

timers are cleaned up by cancel_atimer(), then they get put on a free list
(`free_atimers` on line 132 in the snippet).
It appears that we rely on the OS to cleanup at emacs shutdown.


On Wed, Nov 6, 2024 at 9:36 PM Gerd Möllmann <gerd.moellmann <at> gmail.com>
wrote:

> Jeff Walsh <fejfighter <at> gmail.com> writes:
>
> >  #ifdef WINDOWSNT
> >  #define raise(s) w32_raise(s)
> > @@ -132,7 +133,13 @@ start_atimer (enum atimer_type type, struct
> timespec timestamp,
> >        free_atimers = t->next;
> >      }
> >    else
> > -    t = xmalloc (sizeof *t);
> > +    {
> > +#ifdef HAVE_MPS
> > +      t = igc_xzalloc_ambig (sizeof *t);
> > +#else
> > +      t = xmalloc (sizeof *t);
> > +#endif
> > +    }
> >
> >    /* Fill the atimer structure.  */
> >    memset (t, 0, sizeof *t);
>
> On second thought, and I don't know if it's relevant, do we need to
> igc_xfree that?
>

[Message part 2 (text/html, inline)]

Message #23 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Jeff Walsh <fejfighter <at> gmail.com>
Cc: 74224 <at> debbugs.gnu.org
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in
 atimer used for checking scaling
Date: Wed, 06 Nov 2024 11:49:02 +0100

Jeff Walsh <fejfighter <at> gmail.com> writes:

>> LGTM. Can you commit that yourself to the branch?
>
> I don't have a savannah account for emacs, but I have just registered and will chase up approvals.
>
> timers are cleaned up by cancel_atimer(), then they get put on a free list (`free_atimers` on line 132 in the snippet).
> It appears that we rely on the OS to cleanup at emacs shutdown.

Very good, thanks!

I'll commit that for you, then, and close this bug when done.

>
> On Wed, Nov 6, 2024 at 9:36 PM Gerd Möllmann <gerd.moellmann <at> gmail.com> wrote:
>
>  Jeff Walsh <fejfighter <at> gmail.com> writes:
>
>  >  #ifdef WINDOWSNT
>  >  #define raise(s) w32_raise(s)
>  > @@ -132,7 +133,13 @@ start_atimer (enum atimer_type type, struct timespec timestamp,
>  >        free_atimers = t->next;
>  >      }
>  >    else
>  > -    t = xmalloc (sizeof *t);
>  > +    {
>  > +#ifdef HAVE_MPS
>  > +      t = igc_xzalloc_ambig (sizeof *t);
>  > +#else
>  > +      t = xmalloc (sizeof *t);
>  > +#endif
>  > +    }
>  >  
>  >    /* Fill the atimer structure.  */
>  >    memset (t, 0, sizeof *t);
>
>  On second thought, and I don't know if it's relevant, do we need to
>  igc_xfree that?

Message #28 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: 74224 <at> debbugs.gnu.org, fejfighter <at> gmail.com
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in atimer
 used for checking scaling
Date: Wed, 06 Nov 2024 15:17:34 +0200

> Cc: 74224 <at> debbugs.gnu.org
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Wed, 06 Nov 2024 11:49:02 +0100
> 
> I'll commit that for you, then, and close this bug when done.

Thanks, but it seems like you committed this to the wrong branch
(tty-child-frames instead of igc).

Message #31 received at 74224 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 74224 <at> debbugs.gnu.org, fejfighter <at> gmail.com
Subject: Re: bug#74224: [PATCH][scratch/igc] pgtk: fix crashing bug in
 atimer used for checking scaling
Date: Wed, 06 Nov 2024 14:39:08 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 74224 <at> debbugs.gnu.org
>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> Date: Wed, 06 Nov 2024 11:49:02 +0100
>> 
>> I'll commit that for you, then, and close this bug when done.
>
> Thanks, but it seems like you committed this to the wrong branch
> (tty-child-frames instead of igc).

Oops. Should be fixed now.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.