Package: emacs;
Reported by: Ihor Radchenko <yantar92 <at> posteo.net>
Date: Fri, 17 Jan 2025 14:35:02 UTC
Severity: normal
Found in version 31.0.50
Done: Pip Cet <pipcet <at> protonmail.com>
Bug is archived. No further changes may be made.
Message #73 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 13:37:18 +0000
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Sun, 19 Jan 2025 09:44:26 +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:
>>
>> > 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.