Re: [MPS-test] scratch/igc branch (commit: 42731228d24) crashed

[Pip Cet <pipcet@protonmail.com>]

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

On Monday, August 5th, 2024 at 16:20, Pip Cet <pipcet@protonmail.com> wrote:
> "Eval Exec" execvy@gmail.com writes:
> 
> > I wanted to help test the scratch/igc branch, so I compiled it from
> > commit 42731228d24.
> 
> Thank you for doing that!
> 
> > Then I build it by
> > ```bash
> > make extraclean
> > ./autogen.sh \
> > && ./configure \
> > --prefix=$(realpath ../emacs-build)\
> > --with-mps \
> > --with-imagemagick \
> > --with-modules --with-x-toolkit=lucid --without-compress-install \
> > --without-toolkit-scroll-bars --with-native-compilation --with-mailutils\
> > --with-tree-sitter --with-xinput2 \
> > --with-dbus --with-native-compilation=aot \
> > --with-file-notification=inotify\
> > && make -j20 install
> 
> 
> Like Eli, I'm suspecting the Lucid widget code...
> 
> > It works perfectly for the first hour, but then it crashes, and I
> > don't know how to reproduce the issue.
> > 
> > I think the crash is caused by "window size adjustments"
> > ```
> > (gdb) bt
> > #0 0x00007f47afea2efc in __pthread_kill_implementation () from
> > /nix/store/dbcw19dshdwnxdv5q2g6wldj6syyvq7l-glibc-2.39-52/lib/libc.so.6
> > #1 0x00007f47afe52e86 in raise () from
> > /nix/store/dbcw19dshdwnxdv5q2g6wldj6syyvq7l-glibc-2.39-52/lib/libc.so.6
> > #2 0x00000000004243a4 in terminate_due_to_signal (sig=sig@entry=8,
> > backtrace_limit=backtrace_limit@entry=40) at emacs.c:470
> > #3 0x00000000004248fc in handle_fatal_signal (sig=sig@entry=8) at sysdep.c:1800
> > #4 0x000000000056f568 in deliver_thread_signal (sig=8,
> > handler=0x4248f1 <handle_fatal_signal>) at sysdep.c:1792
> > #5 0x000000000056f669 in deliver_fatal_thread_signal (sig=<optimized
> > out>) at sysdep.c:1812
> > #6 <signal handler called>
> > #7 0x000000000069e35e in pixel_to_char_size
> > (pixel_width=pixel_width@entry=482,
> > pixel_height=pixel_height@entry=76,
> > char_width=char_width@entry=0x7ffced20da38,
> > char_height=char_height@entry=0x7ffced20da3c,
> > ew=<optimized out>) at widget.c:172
> 
> 
> That code reads:
> 
> static void
> pixel_to_char_size (EmacsFrame ew, Dimension pixel_width,
> Dimension pixel_height, int *char_width, int *char_height)
> {
> struct frame *f = ew->emacs_frame.frame;
> 
> 
> *char_width = FRAME_PIXEL_WIDTH_TO_TEXT_COLS (f, (int) pixel_width);
> *char_height = FRAME_PIXEL_HEIGHT_TO_TEXT_LINES (f, (int) pixel_height);
> }
> 
> Which gets a frame pointer from a structure (ew) that MPS doesn't know
> about; but frames can move, so that explains the bug, because MPS
> doesn't know to update the pointer in the EmacsFrame structure.
> 
> Unfortunately, the structure is allocated by XtCreateWidget, which calls
> malloc(), IIUC. So it's not immediately obvious (to me) how to fix it.
> My best idea so far is to create an ambiguous root covering the frame
> pointer and store that in the structure instead.

So I've done that, can you try the attached patch? This is a first stab (thus the ambiguous alloc), but it should prevent the crash we've seen.

TODO list for this:
- use precise alloc
- check framep, not *framep, in EmacsFrameInitialize

Pip

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-mps-lucid.patch --]
[-- Type: text/x-patch; name=0001-mps-lucid.patch, Size: 6710 bytes --]

diff --git a/src/widget.c b/src/widget.c
index d22732ff93a..cc1aeeb25a9 100644
--- a/src/widget.c
+++ b/src/widget.c
@@ -32,6 +32,7 @@
 #include "sysstdio.h"
 #include "xterm.h"
 #include "frame.h"
+#include "igc.h"
 
 #include <X11/StringDefs.h>
 #include <X11/IntrinsicP.h>
@@ -61,7 +62,7 @@ #define offset(field) offsetof (EmacsFrameRec, emacs_frame.field)
 
   {(char *) XtNemacsFrame, (char *) XtCEmacsFrame,
      XtRPointer, sizeof (XtPointer),
-     offset (frame), XtRImmediate, 0},
+     offset (framep), XtRImmediate, 0},
 
   {(char *) XtNminibuffer, (char *) XtCMinibuffer, XtRInt, sizeof (int),
      offset (minibuffer), XtRImmediate, (XtPointer)0},
@@ -157,7 +158,7 @@ emacsFrameClass (void)
 static void
 get_default_char_pixel_size (EmacsFrame ew, int *pixel_width, int *pixel_height)
 {
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   *pixel_width = FRAME_COLUMN_WIDTH (f);
   *pixel_height = FRAME_LINE_HEIGHT (f);
@@ -167,7 +168,7 @@ get_default_char_pixel_size (EmacsFrame ew, int *pixel_width, int *pixel_height)
 pixel_to_char_size (EmacsFrame ew, Dimension pixel_width,
 		    Dimension pixel_height, int *char_width, int *char_height)
 {
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   *char_width = FRAME_PIXEL_WIDTH_TO_TEXT_COLS (f, (int) pixel_width);
   *char_height = FRAME_PIXEL_HEIGHT_TO_TEXT_LINES (f, (int) pixel_height);
@@ -177,7 +178,7 @@ pixel_to_char_size (EmacsFrame ew, Dimension pixel_width,
 char_to_pixel_size (EmacsFrame ew, int char_width, int char_height,
 		    Dimension *pixel_width, Dimension *pixel_height)
 {
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   *pixel_width = FRAME_TEXT_COLS_TO_PIXEL_WIDTH (f, char_width);
   *pixel_height = FRAME_TEXT_LINES_TO_PIXEL_HEIGHT (f, char_height);
@@ -259,7 +260,7 @@ set_frame_size (EmacsFrame ew)
 
    */
 
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   ew->core.width = FRAME_PIXEL_WIDTH (f);
   ew->core.height = FRAME_PIXEL_HEIGHT (f);
@@ -326,7 +327,7 @@ widget_update_wm_size_hints (Widget widget, Widget frame)
 static void
 update_various_frame_slots (EmacsFrame ew)
 {
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   f->internal_border_width = ew->emacs_frame.internal_border_width;
 }
@@ -334,7 +335,7 @@ update_various_frame_slots (EmacsFrame ew)
 static void
 update_from_various_frame_slots (EmacsFrame ew)
 {
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
   struct x_output *x = f->output_data.x;
 
   ew->core.height = FRAME_PIXEL_HEIGHT (f) - x->menubar_height;
@@ -359,7 +360,7 @@ EmacsFrameInitialize (Widget request, Widget new,
 {
   EmacsFrame ew = (EmacsFrame) new;
 
-  if (!ew->emacs_frame.frame)
+  if (!*ew->emacs_frame.framep)
     {
       fputs ("can't create an emacs frame widget without a frame\n", stderr);
       exit (1);
@@ -384,7 +385,7 @@ EmacsFrameRealize (Widget widget, XtValueMask *mask,
 		   XSetWindowAttributes *attrs)
 {
   EmacsFrame ew = (EmacsFrame) widget;
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   /* This used to contain SubstructureRedirectMask, but this turns out
      to be a problem with XIM on Solaris, and events from that mask
@@ -410,14 +411,21 @@ EmacsFrameRealize (Widget widget, XtValueMask *mask,
 static void
 EmacsFrameDestroy (Widget widget)
 {
-  /* All GCs are now freed in x_free_frame_resources.  */
+  EmacsFrame ew = (EmacsFrame) widget;
+  struct frame **fp = ew->emacs_frame.framep;
+
+#ifdef HAVE_MPS
+  igc_xfree (fp);
+#else
+  xfree (fp);
+#endif
 }
 
 static void
 EmacsFrameResize (Widget widget)
 {
   EmacsFrame ew = (EmacsFrame) widget;
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   if (CONSP (frame_size_history))
     frame_size_history_extra
@@ -470,7 +478,7 @@ EmacsFrameQueryGeometry (Widget widget, XtWidgetGeometry *request,
 EmacsFrameSetCharSize (Widget widget, int columns, int rows)
 {
   EmacsFrame ew = (EmacsFrame) widget;
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   if (CONSP (frame_size_history))
     frame_size_history_extra
@@ -489,7 +497,7 @@ EmacsFrameSetCharSize (Widget widget, int columns, int rows)
 EmacsFrameExpose (Widget widget, XEvent *event, Region region)
 {
   EmacsFrame ew = (EmacsFrame) widget;
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   expose_frame (f, event->xexpose.x, event->xexpose.y,
 		event->xexpose.width, event->xexpose.height);
@@ -501,7 +509,7 @@ EmacsFrameExpose (Widget widget, XEvent *event, Region region)
 widget_store_internal_border (Widget widget)
 {
   EmacsFrame ew = (EmacsFrame) widget;
-  struct frame *f = ew->emacs_frame.frame;
+  struct frame *f = *ew->emacs_frame.framep;
 
   ew->emacs_frame.internal_border_width = f->internal_border_width;
 }
diff --git a/src/widgetprv.h b/src/widgetprv.h
index b9cd72a4bc2..e63bbda1fe3 100644
--- a/src/widgetprv.h
+++ b/src/widgetprv.h
@@ -25,7 +25,7 @@ #define _EmacsFrameP_h
 #include <X11/CoreP.h>
 
 typedef struct {
-  struct frame *frame;		/* the *emacs* frame object */
+  struct frame **framep;		/* the *emacs* frame object */
 
   /* Resources that can't be done from lisp.
    */
diff --git a/src/xfns.c b/src/xfns.c
index 3f0d8f3fcd0..53ad460903b 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -24,6 +24,7 @@ Copyright (C) 1989, 1992-2024 Free Software Foundation, Inc.
 #include <unistd.h>
 
 #include "lisp.h"
+#include "igc.h"
 #include "character.h"
 #include "xterm.h"
 #include "frame.h"
@@ -4195,12 +4196,20 @@ x_window (struct frame *f, long window_prompting)
   /* mappedWhenManaged to false tells to the paned window to not map/unmap
      the emacs screen when changing menubar.  This reduces flickering.  */
 
+  struct frame **framep;
+#ifdef HAVE_MPS
+  framep = igc_xzalloc_ambig (sizeof *framep);
+#else
+  framep = xmalloc (sizeof *framep);
+#endif
+  *framep = f;
+
   ac = 0;
   XtSetArg (al[ac], XtNmappedWhenManaged, 0); ac++;
   XtSetArg (al[ac], (char *) XtNshowGrip, 0); ac++;
   XtSetArg (al[ac], (char *) XtNallowResize, 1); ac++;
   XtSetArg (al[ac], (char *) XtNresizeToPreferred, 1); ac++;
-  XtSetArg (al[ac], (char *) XtNemacsFrame, f); ac++;
+  XtSetArg (al[ac], (char *) XtNemacsFrame, framep); ac++;
   XtSetArg (al[ac], XtNvisual, FRAME_X_VISUAL (f)); ac++;
   XtSetArg (al[ac], XtNdepth, FRAME_DISPLAY_INFO (f)->n_planes); ac++;
   XtSetArg (al[ac], XtNcolormap, FRAME_X_COLORMAP (f)); ac++;