Re: MPS: a random backtrace while toying with gdb

[Helmut Eller <eller.helmut@gmail.com>]

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

On Sun, Jun 30 2024, Eli Zaretskii wrote:
>> You don't do anything useful other than packing up the arguments that
>> the signal handler receives and put them in the queue.
>
> What arguments are those?  SIGCHLD doesn't tell us the PID of the
> process (or any other data that could be used to identify the
> process), AFAICT.  What if two or more sub-processes exited while we
> are in MPS-land?

This patch below implements the idea I was thinking about.

>> The MPS documentation says quite clearly that a scanner must not access
>> MPS-managed memory other than the block it receives as argument:
>
> What do you mean by "scanner"?  I was talking about our signal
> handlers.  If they qualify as "scanner", please explain why.

I mean dflt_scan.

> Anyway, if a signal handler cannot do anything useful, the only
> alternative is to block select signals around code which could violate
> those restrictions.

We will see.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-igc_check_fwd.patch --]
[-- Type: text/x-diff, Size: 694 bytes --]

From f4080ec23a0630bd3c5c37c853cd2528faec92c4 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 30 Jun 2024 20:15:51 +0200
Subject: [PATCH 1/2] Fix igc_check_fwd

* src/igc.c (igc_check_fwd): Add missing argument to has_header.
---
 src/igc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/igc.c b/src/igc.c
index 324bcfa112f..2165a69cacf 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -598,7 +598,7 @@ alloc_hash (void)
 void
 igc_check_fwd (void *client)
 {
-  if (has_header (client))
+  if (has_header (client, true))
     {
       struct igc_header *h = client_to_base (client);
       igc_assert (h->obj_type != IGC_OBJ_FWD);
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Process-SIGCHLD-asynchronously.patch --]
[-- Type: text/x-diff, Size: 7535 bytes --]

From 7b8715470bdec2dd0808c530c19ca4b749c6178b Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 30 Jun 2024 20:17:05 +0200
Subject: [PATCH 2/2] Process SIGCHLD asynchronously

Introduce a async_work_queue that is processed as part of
process_pending_signals.

* src/keyboard.h (struct async_work_item)
(enum async_work_type): New types.
(enqueue_async_work): New function.
* src/keyboard.c (struct async_work_queue): New type.
(async_work_queue): New global variable.
(async_work_queue_len, async_work_queue_pop_front)
(do_async_work, do_async_work_1): New helpers.
(enqueue_async_work): Implementation.
(process_pending_signals): Call do_async_work;
* src/process.h (process_sigchld_async): New function.
* src/process.c (process_sigchld_async): Factored out from
handle_child_signal.
(handle_child_signal): Use enqueue_async_work.
---
 src/keyboard.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/keyboard.h | 16 +++++++++++++
 src/process.c  | 62 +++++++++++++++++++++++++++----------------------
 src/process.h  |  2 ++
 4 files changed, 116 insertions(+), 27 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 06599ed0fdf..71286df8915 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -374,6 +374,15 @@ #define GROW_RAW_KEYBUF							\
    menu bar.  */
 static Lisp_Object menu_bar_touch_id;
 
+#define ASYNC_WORK_QUEUE_CAPACITY 64
+
+struct async_work_queue {
+  uint8_t start, end;
+  struct async_work_item items[ASYNC_WORK_QUEUE_CAPACITY];
+};
+
+static struct async_work_queue async_work_queue;
+
 \f
 /* Global variable declarations.  */
 
@@ -415,6 +424,7 @@ #define READABLE_EVENTS_IGNORE_SQUEEZABLES	(1 << 2)
 static char *find_user_signal_name (int);
 static void store_user_signal_events (void);
 static bool is_ignored_event (union buffered_input_event *);
+static void do_async_work (void);
 
 /* Advance or retreat a buffered input event pointer.  */
 
@@ -8176,6 +8186,7 @@ process_pending_signals (void)
   pending_signals = false;
   handle_async_input ();
   do_pending_atimers ();
+  do_async_work ();
 }
 
 /* Undo any number of BLOCK_INPUT calls down to level LEVEL,
@@ -12845,6 +12856,58 @@ is_ignored_event (union buffered_input_event *event)
   return !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events));
 }
 
+static uint8_t
+async_work_queue_len (struct async_work_queue *q)
+{
+  uint8_t cap = ASYNC_WORK_QUEUE_CAPACITY;
+  return (q->start <= q->end
+	  ? q->end - q->start
+	  : cap - q->start + q->end);
+}
+
+static struct async_work_item
+async_work_queue_pop_front (struct async_work_queue *q)
+{
+  if (async_work_queue_len (q) == 0)
+    emacs_abort ();
+  struct async_work_item item = q->items[q->start];
+  uint8_t cap = ASYNC_WORK_QUEUE_CAPACITY;
+  q->start = (q->start + 1) % cap;
+  return item;
+}
+
+void
+enqueue_async_work (struct async_work_item item)
+{
+  struct async_work_queue *q = &async_work_queue;
+  uint8_t cap = ASYNC_WORK_QUEUE_CAPACITY;
+  if (async_work_queue_len (q) == cap)
+    emacs_abort ();
+  q->items[q->end] = item;
+  q->end = (q->end + 1) % cap;
+  pending_signals = 1;
+}
+
+static void
+do_async_work_1 (struct async_work_item item)
+{
+  switch (item.type)
+    {
+    case ASYNCWORK_SIGCHLD:
+      process_sigchld_async (item.u.sigchld);
+      return;
+    }
+  emacs_abort ();
+}
+
+static void
+do_async_work (void)
+{
+  struct async_work_queue *q = &async_work_queue;
+  while (async_work_queue_len (q) > 0)
+    do_async_work_1 (async_work_queue_pop_front (q));
+}
+
 static void syms_of_keyboard_for_pdumper (void);
 
 void
diff --git a/src/keyboard.h b/src/keyboard.h
index c7ae1f7f0fa..8fb592dacff 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -527,4 +527,20 @@ kbd_buffer_store_event_hold (struct input_event *event,
 
 INLINE_HEADER_END
 
+enum async_work_type
+{
+  ASYNCWORK_SIGCHLD
+};
+
+struct async_work_item
+{
+  enum async_work_type type;
+  union
+  {
+    int sigchld;
+  } u;
+};
+
+extern void enqueue_async_work (struct async_work_item item);
+
 #endif /* EMACS_KEYBOARD_H */
diff --git a/src/process.c b/src/process.c
index a3b2d2e54bd..5939d16c37e 100644
--- a/src/process.c
+++ b/src/process.c
@@ -7584,33 +7584,8 @@ child_signal_notify (void)
 static void dummy_handler (int sig) {}
 static signal_handler_t volatile lib_child_handler;
 
-/* Handle a SIGCHLD signal by looking for known child processes of
-   Emacs whose status have changed.  For each one found, record its
-   new status.
-
-   All we do is change the status; we do not run sentinels or print
-   notifications.  That is saved for the next time keyboard input is
-   done, in order to avoid timing errors.
-
-   ** WARNING: this can be called during garbage collection.
-   Therefore, it must not be fooled by the presence of mark bits in
-   Lisp objects.
-
-   ** USG WARNING: Although it is not obvious from the documentation
-   in signal(2), on a USG system the SIGCLD handler MUST NOT call
-   signal() before executing at least one wait(), otherwise the
-   handler will be called again, resulting in an infinite loop.  The
-   relevant portion of the documentation reads "SIGCLD signals will be
-   queued and the signal-catching function will be continually
-   reentered until the queue is empty".  Invoking signal() causes the
-   kernel to reexamine the SIGCLD queue.  Fred Fish, UniSoft Systems
-   Inc.
-
-   ** Malloc WARNING: This should never call malloc either directly or
-   indirectly; if it does, that is a bug.  */
-
-static void
-handle_child_signal (int sig)
+void
+process_sigchld_async (int sig)
 {
   Lisp_Object tail, proc;
   bool changed = false;
@@ -7688,6 +7663,39 @@ handle_child_signal (int sig)
     /* Wake up `wait_reading_process_output'.  */
     child_signal_notify ();
 
+}
+
+/* Handle a SIGCHLD signal by looking for known child processes of
+   Emacs whose status have changed.  For each one found, record its
+   new status.
+
+   All we do is change the status; we do not run sentinels or print
+   notifications.  That is saved for the next time keyboard input is
+   done, in order to avoid timing errors.
+
+   ** WARNING: this can be called during garbage collection.
+   Therefore, it must not be fooled by the presence of mark bits in
+   Lisp objects.
+
+   ** USG WARNING: Although it is not obvious from the documentation
+   in signal(2), on a USG system the SIGCLD handler MUST NOT call
+   signal() before executing at least one wait(), otherwise the
+   handler will be called again, resulting in an infinite loop.  The
+   relevant portion of the documentation reads "SIGCLD signals will be
+   queued and the signal-catching function will be continually
+   reentered until the queue is empty".  Invoking signal() causes the
+   kernel to reexamine the SIGCLD queue.  Fred Fish, UniSoft Systems
+   Inc.
+
+   ** Malloc WARNING: This should never call malloc either directly or
+   indirectly; if it does, that is a bug.  */
+
+static void
+handle_child_signal (int sig)
+{
+  struct async_work_item item = {ASYNCWORK_SIGCHLD, .u.sigchld = sig};
+  enqueue_async_work (item);
+
   lib_child_handler (sig);
 #ifdef NS_IMPL_GNUSTEP
   /* NSTask in GNUstep sets its child handler each time it is called.
diff --git a/src/process.h b/src/process.h
index f497a64c3d1..1d59a658d95 100644
--- a/src/process.h
+++ b/src/process.h
@@ -308,4 +308,6 @@ pset_gnutls_cred_type (struct Lisp_Process *p, Lisp_Object val)
 
 INLINE_HEADER_END
 
+extern void process_sigchld_async (int sig);
+
 #endif /* EMACS_PROCESS_H */
-- 
2.39.2