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

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

[Fejfighter]

[-- Attachment #1.1: Type: text/plain, Size: 592 bytes --]

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'

[-- Attachment #1.2: Type: text/html, Size: 707 bytes --]

[-- Attachment #2: mps-igc-pgtk.patch --]
[-- Type: text/x-patch, Size: 1495 bytes --]

diff --git a/src/atimer.c b/src/atimer.c
index 8253ae3a166..5bd282a310d 100644
--- a/src/atimer.c
+++ b/src/atimer.c
@@ -17,6 +17,7 @@
 along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #include <config.h>
+#include "igc.h"
 
 #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);
diff --git a/src/pgtkterm.c b/src/pgtkterm.c
index ef940509626..c0ed68d6820 100644
--- a/src/pgtkterm.c
+++ b/src/pgtkterm.c
@@ -7166,7 +7166,11 @@ #define NUM_ARGV 10
   if (ckd_add (&nbytes, SBYTES (Vinvocation_name), SBYTES (system_name) + 2))
     memory_full (SIZE_MAX);
   dpyinfo->x_id = ++x_display_id;
+#ifdef HAVE_MPS
+  dpyinfo->x_id_name = igc_xzalloc_ambig (nbytes);
+#else
   dpyinfo->x_id_name = xmalloc (nbytes);
+#endif
   char *nametail = lispstpcpy (dpyinfo->x_id_name, Vinvocation_name);
   *nametail++ = '@';
   lispstpcpy (nametail, system_name);
@@ -7302,7 +7306,14 @@ pgtk_delete_display (struct pgtk_display_info *dpyinfo)
     }
 
   pgtk_free_devices (dpyinfo);
+
+#ifdef HAVE_MPS
+  igc_xfree (dpyinfo->x_id_name);
+  igc_xfree (dpyinfo);
+#else
+  xfree (dpyinfo->x_id_name);
   xfree (dpyinfo);
+#endif
 }
 
 char *

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

[Gerd Möllmann]

Fejfighter <fejfighter@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.)

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

[Jeff Walsh]

[-- Attachment #1.1: Type: text/plain, Size: 1511 bytes --]

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@gmail.com>
wrote:

> Fejfighter <fejfighter@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.)
>

[-- Attachment #1.2: Type: text/html, Size: 2099 bytes --]

[-- Attachment #2: 0001-Mark-atimer-allocation-as-ambiguous-root.patch --]
[-- Type: text/x-patch, Size: 1070 bytes --]

From 080541ae5463eda9b20d9ec92cb41202ba21d87c Mon Sep 17 00:00:00 2001
From: Jeff Walsh <fejfighter@gmail.com>
Date: Tue, 5 Nov 2024 23:09:56 +1100
Subject: [PATCH] Mark atimer allocation as ambiguous root

* src/atimer.c (start_atimer): Time could be passed a lisp object for
data, we should allocate this as an ambiguous root
---
 src/atimer.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/atimer.c b/src/atimer.c
index 8253ae3a166..5bd282a310d 100644
--- a/src/atimer.c
+++ b/src/atimer.c
@@ -17,6 +17,7 @@
 along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #include <config.h>
+#include "igc.h"
 
 #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);
-- 
2.47.0

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

[Gerd Möllmann]

Jeff Walsh <fejfighter@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?

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

[Gerd Möllmann]

Jeff Walsh <fejfighter@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?

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

[Jeff Walsh]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

> 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@gmail.com>
wrote:

> Jeff Walsh <fejfighter@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?
>

[-- Attachment #2: Type: text/html, Size: 1625 bytes --]

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

[Gerd Möllmann]

Jeff Walsh <fejfighter@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@gmail.com> wrote:
>
>  Jeff Walsh <fejfighter@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?

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

[Eli Zaretskii]

> Cc: 74224@debbugs.gnu.org
> From: Gerd Möllmann <gerd.moellmann@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).

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

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 74224@debbugs.gnu.org
>> From: Gerd Möllmann <gerd.moellmann@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.