GNU bug report logs - #75632
31.0.50; igc: Crash report

Previous Next

Full log

Message #67 received at 75632 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: yantar92 <at> posteo.net, 75632 <at> debbugs.gnu.org
Subject: Re: bug#75632: 31.0.50; igc: Crash report
Date: Sun, 19 Jan 2025 09:44:26 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Sat, 18 Jan 2025 20:40:47 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75632 <at> debbugs.gnu.org, yantar92 <at> posteo.net
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> > But if we use !initialized, the code which installs the variable
>> > watcher will not run in the dumped Emacs, right?
>>
>> Correct.  Variable watchers are symbol properties, and symbol properties
>> are restored from the dump, so there is no need to add them again.
>
> Then why do we need to do this in C and not in Lisp?  Can't we set up
> the watcher as part of some preloaded Lisp file?
>
>> >> >> IIRC, there are some loops that test Vinhibit_quit or Vquit_flag
>> >> >> directly (or use QUITP); we wouldn't be able to break out of one of
>> >> >> those if we only set Vinhibit_quit from what's currently
>> >> >> store_user_signal_events, would we?
>> >> >
>> >> > Sorry, I don't follow.  Why does it matter whether we set those
>> >> > variables from a signal handler or in gobble_input?
>> >>
>> >> We might never reach gobble_input if the variable isn't set in the
>> >> signal handler: some loops check QUITP (etc.), and if there's a bug in
>> >> them and Vinhibit_quit is set, the loop might never call maybe_quit.
>> >
>> > Then how about setting Vquit_flag and resetting Vinhibit_quit in the
>> > handler, but leaving the check whether we should call the debugger to
>> > gobble_input?
>>
>> 1. We can't do that unconditionally.  An ordinary SIGUSR2 should not
>> cause a quit, only a special event should.  So we need to check whether
>> the event we received is special or not.
>>
>> 2. probably_quit handles the quit flag first, then handles pending
>> signals only when it is called again, by the next maybe_quit.  That
>> means by the time we reach gobble_input, it's too late to call the
>> debugger.  So we need to set debug_* at the same time as setting
>> Vquit_flag (or rewrite probably_quit, which probably has very good
>> reasons for its peculiar behavior).
>
> 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.

4. Make that watcher callable as Fdebug_on_event directly.

I stopped there.  I'm confused about input_available_clear_time: it's
only cleared if we don't enter the debugger.  Adding the code to clear
it to the debug_on_event case might help us break out of some broken
pselects, and it shouldn't ever hurt us (famous last words).

But maybe I totally misunderstood what you suggested, too.

diff --git a/src/keyboard.c b/src/keyboard.c
index fa19c9f8ad3..217ec6582fc 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 its normal behavior.  */
+  bool debug_on_event;
+
   /* Name of the signal.  */
   char *name;
 
@@ -8279,6 +8283,7 @@ add_user_signal (int sig, const char *name)
 
   p = xmalloc (sizeof *p);
   p->sig = sig;
+  p->debug_on_event = false;
   p->name = xstrdup (name);
   p->npending = 0;
   p->next = user_signals;
@@ -8288,42 +8293,68 @@ add_user_signal (int sig, const char *name)
   sigaction (sig, &action, 0);
 }
 
+DEFUN ("debug_on_event", Fdebug_on_event, Sdebug_on_event, 1, 4, 0,
+       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)
+{
+  Lisp_Object ret = Qnil;
+  Lisp_Object symbol = arg_or_symbol;
+  if (EQ (symbol, Qdebug_on_event))
+    symbol = newval;
+
+  CHECK_SYMBOL (symbol);
+
+  struct user_signal_info *p;
+  for (p = user_signals; p; p = p->next)
+    if (SYMBOLP (symbol) && strcmp (SSDATA (SYMBOL_NAME (symbol)), p->name) == 0)
+      {
+	p->debug_on_event = true;
+	ret = Qt;
+      }
+    else
+      p->debug_on_event = false;
+
+  Vdebug_on_event = symbol;
+
+  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;
       }
@@ -12755,6 +12786,16 @@ init_keyboard (void)
   poll_suppress_count = 1;
   start_polling ();
 #endif
+
+  DEFSYM (Qdebug_on_event, "debug-on-event");
+
+  /* Variable watchers persist across the dump, no need to add them
+     twice.  */
+  if (!initialized)
+    {
+      Fadd_variable_watcher (Qdebug_on_event, Qdebug_on_event);
+    }
+  Fdebug_on_event (Qdebug_on_event, Vdebug_on_event, Qnil, Qnil);
 }
 
 /* This type's only use is in syms_of_keyboard, to put properties on the
@@ -13139,6 +13180,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);


> copying the name of the event aside, can we produce a C integer that
> identifies the event, and store that number instead?  Accessing

Sorry, I ignored this when writing the patch.  I don't think we should
hardcode any assumptions about signal numbers, and I don't see another
readily available integer we can use; in any case, it's now trivial to
modify debug-on-event so several events enter the debugger.  This might
be handy for the Android port, which, IIUC, currently binds
triple-volume-down to quit, not to enter the debugger.  (I think all
other ports have stable signal handling interfaces, but I may be wrong).

> integers from a signal handler should not be a problem, right?  (In
> retrospect, we should have probably made debug-on-event a function,
> then we could produce that number inside the function, thus avoiding
> the need for watching the variable.  Maybe we should introduce the
> function now and deprecate the variable?)

I've introduced a function, so you can call debug-on-event or set it,
but I haven't deprecated the variable yet, because there's no way to
read the value from Lisp otherwise.

With this change, we still have six (!) debug-on-* variables, and two
additional debug-on-* functions.  They all behave slightly differently.

Is this at all what you meant?  I realize it's incomplete, but it's
probably worth it to do things properly.

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.