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

From: Pip Cet <[email-masked]>
Date: Sun, 19 Jan 2025 13:37:18 +0000

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

>> Date: Sun, 19 Jan 2025 09:44:26 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> > OK, but could we at least make this somewhat cleaner?  Instead of
>>
>> I see no reason for why the old code behaved as it did, so I'm proposing
>> this:
>>
>> 1. doc change: don't use "special event" to mean two totally different
>> things in process_special_events and special_event_name; remove
>> special_event_name.
>>
>> 2. when debug-on-event is modified/called, set a per-signal flag in the
>> user_signal_info struct, saving us a strcmp.
>>
>> 3. Add a watcher as on feature/igc to reset the flags of all user
>> signals.
>
> Can we add the watcher in Lisp instead?

Yes, please.  That way, we could have a proper interactive form, and the
current watcher could use an "internal" name (--).

> Are there any reasons to have it in C

Not that I can think of, but there are plenty of reasons to add it from
Lisp instead.

> and at temacs time?

I don't think it's a realistic scenario that someone needs to debug
non-interactive temacs, cannot use the default signal, and wants to
change it.

> Also, perhaps make the function debug-on-event call the watcher, not
> itself be the watcher.  I think that might make the API cleaner.  The

You've lost me there.  I was thinking debug-on-event, a Lisp function,
calls debug--on-event, a C subr, and we also install, from Lisp, a
variable watcher on the deprecated debug-on-event variable.  No static
subrs, which would have prevented the bug.

> watcher function doesn't have to be exposed to Lisp, btw.

I'd just use an anonymous lambda constructed in Lisp?  Causes a problem,
see below.

>> +DEFUN ("debug_on_event", Fdebug_on_event, Sdebug_on_event, 1, 4, 0,
                 ^  ^

fixing that, of course.

>> +       doc: /* Make EVENT enter the debugger.
>> +
>> +EVENT should be a symbol, either sigusr1 or sigusr2.
>> +
>> +This function supports an additional calling convention to be usable as
>> +a variable watcher.  */)
>> +  (Lisp_Object arg_or_symbol, Lisp_Object newval, Lisp_Object operation,
>> +   Lisp_Object where)
>
> There's no EVENT in the function's arguments.  And what are the other
> arguments?  The last two seem to be unused?

Yes, that was a failed attempt to make the DEFUN satisfy both the
variable watcher API and be callable from Lisp.  Bad idea.

>> +	if (p->debug_on_event)
>>            {
>>              /* Enter the debugger in many ways.  */
>>              debug_on_next_call = true;
>>              debug_on_quit = true;
>>              Vquit_flag = Qt;
>>              Vinhibit_quit = Qnil;
>> -
>> -            /* Eat the event.  */
>> -            break;
>
> Why did you remove this last part?  With the change to a flag, we no
> longer have a problem that requires delaying the event processing,
> right?

> So why risk the change in behavior, something that you warned
> against when I suggested the same?

No behavior change intended!  The new code is:

	if (p->debug_on_event)
          {
            /* Enter the debugger in many ways.  */
            debug_on_next_call = true;
            debug_on_quit = true;
            Vquit_flag = Qt;
            Vinhibit_quit = Qnil;
          }
	else
	  {
             ...
	  }
	break;

So we hit the "break" statement just as we did in the original code,
unless I'm very confused.  I misread the code originally (missed the
"break", and thought we always cleared the select timeout; I still think
that's what we *should* do).

>> Is this at all what you meant?  I realize it's incomplete, but it's
>> probably worth it to do things properly.
>
> Yes, that's going in the right direction, I think.  Thanks.

Before you read the patch: bindings.el doesn't seem like the best place
for this code; it's related to signal handlers, and I added a comment
while I was there, but I couldn't find one right away.  Any suggestions?

As little C code as possible, and we'll just run with the default signal
handler until Lisp fixes it.

Potentially separate bug:

In emacs -Q, the variable documentation contains the bytecode
representation of the lambda watcher:

  Calls these functions when changed: (#[1028 \300%3!\207 [debug--on-event] 6 

(fn SYMBOL NEWVAL OPERATION WHERE)])

I don't think ordinary Emacs users can read byte-code objects.  I
certainly cannot.

Should we fix that in the documentation code, work around it by naming
the watcher, or both?

diff --git a/lisp/bindings.el b/lisp/bindings.el
index 9987f28c027..628a71d2b4c 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -1642,7 +1642,37 @@ ctl-x-4-map
 (define-key ctl-x-4-map "c" 'clone-indirect-buffer-other-window)
 
 ;; Signal handlers
+(defun debug-on-event (event)
+  "Make EVENT enter the debugger.
+
+When Emacs receives the event specified as the argument to this
+function, it will try to break into the debugger as soon as possible
+instead of processing the event normally through `special-event-map'.
+
+EVENT should be a symbol, either `sigusr1' or `sigusr2', or nil."
+  (debug--on-event event))
+
+(defvar debug-on-event 'sigusr2
+  "Enter debugger on this event.
+When Emacs receives the event specified by this variable,
+it will try to break into the debugger as soon as possible instead
+of processing the event normally through `special-event-map'.
+
+Currently, the only supported values for this
+variable are `sigusr1' and `sigusr2'.")
+
+(make-obsolete-variable
+ 'debug-on-event
+ "use the function `debug-on-event' instead."
+ "31.0")
+
+(add-variable-watcher 'debug-on-event
+                      (lambda (_symbol newval _operation _where)
+                        (debug--on-event newval)))
+
 (define-key special-event-map [sigusr1] 'ignore)
+;; SIGUSR2, by default, invokes the debugger directly, so this binding
+;; is ignored.
 (define-key special-event-map [sigusr2] 'ignore)
 
 ;; Text conversion
diff --git a/src/keyboard.c b/src/keyboard.c
index fa19c9f8ad3..36a18976029 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8254,6 +8254,10 @@ deliver_input_available_signal (int sig)
   /* Signal number.  */
   int sig;
 
+  /* True if this event is supposed to make us enter the debugger
+     instead of being treated as an input event.  */
+  bool debug_on_event;
+
   /* Name of the signal.  */
   char *name;
 
@@ -8280,6 +8284,7 @@ add_user_signal (int sig, const char *name)
   p = xmalloc (sizeof *p);
   p->sig = sig;
   p->name = xstrdup (name);
+  p->debug_on_event = !strcmp (p->name, "sigusr2");
   p->npending = 0;
   p->next = user_signals;
   user_signals = p;
@@ -8288,42 +8293,56 @@ add_user_signal (int sig, const char *name)
   sigaction (sig, &action, 0);
 }
 
+DEFUN ("debug--on-event", Fdebug__on_event, Sdebug__on_event, 1, 1, 0,
+       doc: /* Internal function: use debug-on-event instead.  */)
+  (Lisp_Object event)
+{
+  Lisp_Object ret = Qnil;
+  CHECK_SYMBOL (event);
+
+  struct user_signal_info *p;
+  for (p = user_signals; p; p = p->next)
+    if (strcmp (SSDATA (SYMBOL_NAME (event)), p->name) == 0)
+      {
+	p->debug_on_event = true;
+	ret = Qt;
+      }
+    else
+      p->debug_on_event = false;
+
+  return ret;
+}
+
 static void
 handle_user_signal (int sig)
 {
   struct user_signal_info *p;
-  const char *special_event_name = NULL;
-
-  if (SYMBOLP (Vdebug_on_event))
-    special_event_name = SSDATA (SYMBOL_NAME (Vdebug_on_event));
 
   for (p = user_signals; p; p = p->next)
     if (p->sig == sig)
       {
-        if (special_event_name
-	    && strcmp (special_event_name, p->name) == 0)
+	if (p->debug_on_event)
           {
             /* Enter the debugger in many ways.  */
             debug_on_next_call = true;
             debug_on_quit = true;
             Vquit_flag = Qt;
             Vinhibit_quit = Qnil;
-
-            /* Eat the event.  */
-            break;
           }
-
-	p->npending++;
-#if defined (USABLE_SIGIO) || defined (USABLE_SIGPOLL)
-	if (interrupt_input)
-	  handle_input_available_signal (sig);
 	else
-#endif
 	  {
-	    /* Tell wait_reading_process_output that it needs to wake
-	       up and look around.  */
-	    if (input_available_clear_time)
-	      *input_available_clear_time = make_timespec (0, 0);
+	    p->npending++;
+#if defined (USABLE_SIGIO) || defined (USABLE_SIGPOLL)
+	    if (interrupt_input)
+	      handle_input_available_signal (sig);
+	    else
+#endif
+	      {
+		/* Tell wait_reading_process_output that it needs to wake
+		   up and look around.  */
+		if (input_available_clear_time)
+		  *input_available_clear_time = make_timespec (0, 0);
+	      }
 	  }
 	break;
       }
@@ -13139,6 +13158,7 @@ syms_of_keyboard (void)
   defsubr (&Sevent_symbol_parse_modifiers);
   defsubr (&Sevent_convert_list);
   defsubr (&Sinternal_handle_focus_in);
+  defsubr (&Sdebug__on_event);
   defsubr (&Sread_key_sequence);
   defsubr (&Sread_key_sequence_vector);
   defsubr (&Srecursive_edit);
@@ -13787,17 +13807,6 @@ syms_of_keyboard (void)
   Vselection_inhibit_update_commands
     = list2 (Qhandle_switch_frame, Qhandle_select_window);
 
-  DEFVAR_LISP ("debug-on-event",
-               Vdebug_on_event,
-               doc: /* Enter debugger on this event.
-When Emacs receives the special event specified by this variable,
-it will try to break into the debugger as soon as possible instead
-of processing the event normally through `special-event-map'.
-
-Currently, the only supported values for this
-variable are `sigusr1' and `sigusr2'.  */);
-  Vdebug_on_event = Qsigusr2;
-
   DEFVAR_BOOL ("attempt-stack-overflow-recovery",
                attempt_stack_overflow_recovery,
                doc: /* If non-nil, attempt to recover from C stack overflows.