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

From: Pip Cet <[email-masked]>
Date: Wed, 22 Jan 2025 12:30:45 +0000

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

>> Date: Sun, 19 Jan 2025 13:37:18 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> > 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.
>
> I meant the interactive temacs, which begins by loading all the
> preloaded packages.  So one of those could add the watcher.

Do we really need this to be added early?  If you need another signal
before loadup finishes, you're modifying early Lisp anyway, and then you
can just call the builtin function rather than use the variable.

>> > 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.
>
> That's what I meant, yes.

I also tried to move debug-on-event (both the variable and the function)
to debug.el, which seems the most natural place.

The watcher is added in bindings.el, because that's the second place
people will look in when trying to figure out how to bind sigusr2 to a
different event.

>> 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?
>
> One of the first files we load in loadup, I think?  Like subr.el,
> perhaps?

See above.  I think if you need to put an --eval '(debug--on-event
(quote sigusr1))' in your temacs invocation, you're likely knowledgeable
enough to do so.

The only case that's really different is temacs -nl: it would probably
behave differently if it worked (to the extend bare temacs even does).

temacs -nl fixable, but I'll wait for a response before looking into
doing that properly.  Might not be worth 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.
>
> It's not optimal, indeed.  See a somewhat better arrangement we have
> with the variables which should trigger redisplay of the current
> buffer, at the end of frame.el.

I've tried this:

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 9324cf85454..f4f595aedbe 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1712,7 +1712,9 @@ help-fns--var-watchpoints
     (when watchpoints
       (princ "  Calls these functions when changed: ")
       ;; FIXME: Turn function names into hyperlinks.
-      (princ watchpoints)
+      (dolist (watchpoint watchpoints)
+        (princ (help-fns-function-name watchpoint))
+        (princ "\n  "))
       (terpri))))
 
it generates a three-digit hex hash of the function, which is a lot
nicer than bytecode, but still not ideal.

Adding a docstring and a name to the function produces:

  Calls these functions when changed: (#[1028 \300!\207 [debug--on-event] 6 (bindings.elc . 52251)])

If there's a better way to fix this minor problem, we probably should.

>> Should we fix that in the documentation code, work around it by naming
>> the watcher, or both?
>
> See above: changing the way we call the watcher is probably easier,
> and yields satisfactory results.

Here's the current patch.  It doesn't feel quite right yet, but I can't
tell why right now:

diff --git a/lisp/bindings.el b/lisp/bindings.el
index 9987f28c027..bd02aefa348 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -1642,7 +1642,13 @@ ctl-x-4-map
 (define-key ctl-x-4-map "c" 'clone-indirect-buffer-other-window)
 
 ;; Signal handlers
+(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/lisp/cus-start.el b/lisp/cus-start.el
index 0f7d7c3c020..cdf777f6c9a 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -388,11 +388,6 @@ minibuffer-prompt-properties--setter
 					    (const :tag "only shift-selection or mouse-drag" only)
 					    (const :tag "off" nil))
 				    "24.1")
-             (debug-on-event debug
-                             (choice (const :tag "None" nil)
-                                     (const :tag "When sent SIGUSR1" sigusr1)
-                                     (const :tag "When sent SIGUSR2" sigusr2))
-                             "24.1")
              (translate-upper-case-key-bindings keyboard boolean "29.1")
              ;; This is not good news because it will use the wrong
              ;; version-specific directories when you upgrade.  We need
diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index 0ca3a0f931c..ea2d89ef2a2 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -860,6 +860,39 @@ 'cancel-debug-watch
 
 (make-obsolete-variable 'debugger-previous-backtrace
                         "no longer used." "29.1")
+
+;;;###autoload
+(defun debug-on-event (event)
+  "Make EVENT enter the debugger.
+
+When Emacs receives the special 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."
+  (setq debug-on-event event)
+  (debug--on-event event))
+
+;;;###autoload
+(defcustom debug-on-event 'sigusr2
+  "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'."
+  :group 'debug
+  :type '(choice (const :tag "None" nil)
+                 (const :tag "When sent SIGUSR1" sigusr1)
+                 (const :tag "When sent SIGUSR2" sigusr2))
+  :version "24.1")
+
+(make-obsolete-variable
+ 'debug-on-event
+ "use the function `debug-on-event' instead."
+ "31.0")
+
 (defvar debugger-previous-backtrace nil)
 
 (provide 'debug)
diff --git a/src/keyboard.c b/src/keyboard.c
index 6208b333e9c..d2043d669cf 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.

Thanks for the comments!  I hope I didn't run off and do something
totally different from what you intended.

Pip