Why is it so difficult to get a Lisp backtrace?

Why is it so difficult to get a Lisp backtrace?

[Alan Mackenzie]

Hello, Emacs.

(setq debug-on-error t) ought to be useful to get backtraces.  It is,
sometimes, but far from always.

There seem to be three use cases for debug-on-error:
(i) The vast bulk of users, who are barely, if at all, aware of its
  existence.  They will enable it on request from a maintainer to help
  debug a bug.
(ii) Hard-core maintainers, who run with it enabled constantly.  Here
  debug-ignored-errors helps suppress certain unhelpful errors which
  would otherwise occur too often and irritate.
(iii) Those maintainers who run with it disabled, but when an error
  occurs, want to be able to repeat that error and get a backtrace.

This third group of users is poorly catered for by the current collection
of mechanisms.  See also bug #56201, with thanks to Andreas and Lars who
helped me get to the bottom of it.  To be reasonably sure of getting a
backtrace, it seems one needs to do all of the following:
(i) (setq debug-on-error t).
(ii) (setq debug-on-signal t).
(iii) Bind debug-ignored-errors to nil.
(iv) Pray.
(v) Execute the erring command again.

I put it to people that we need a new command or variable not called
something like `backtrace-next-and-I-really-mean-it' which would allow
step (v) to generate the desired backtrace without having to go through
steps (i) to (iv) individually.  The lack of such a command/variable is
making (at least) my Emacs development less pleasant than it might
otherwise be.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Why is it so difficult to get a Lisp backtrace?

[Alan Mackenzie]

Hello, again.

On Sat, Jun 25, 2022 at 13:42:04 +0000, Alan Mackenzie wrote:
> (setq debug-on-error t) ought to be useful to get backtraces.  It is,
> sometimes, but far from always.

> There seem to be three use cases for debug-on-error:
> (i) The vast bulk of users, who are barely, if at all, aware of its
>   existence.  They will enable it on request from a maintainer to help
>   debug a bug.
> (ii) Hard-core maintainers, who run with it enabled constantly.  Here
>   debug-ignored-errors helps suppress certain unhelpful errors which
>   would otherwise occur too often and irritate.
> (iii) Those maintainers who run with it disabled, but when an error
>   occurs, want to be able to repeat that error and get a backtrace.

> This third group of users is poorly catered for by the current collection
> of mechanisms.  See also bug #56201, with thanks to Andreas and Lars who
> helped me get to the bottom of it.  To be reasonably sure of getting a
> backtrace, it seems one needs to do all of the following:
> (i) (setq debug-on-error t).
> (ii) (setq debug-on-signal t).
> (iii) Bind debug-ignored-errors to nil.
> (iv) Pray.
> (v) Execute the erring command again.

> I put it to people that we need a new command or variable not called
> something like `backtrace-next-and-I-really-mean-it' which would allow
> step (v) to generate the desired backtrace without having to go through
> steps (i) to (iv) individually.  The lack of such a command/variable is
> making (at least) my Emacs development less pleasant than it might
> otherwise be.

I mean something like the following, which to a first approximation,
works.  To use it, do M-x debug-next-command and the execute the command
expected to give errors:


;; -*- lexical-binding: t -*-

(defvar debug-next-command-save-debug-on-error nil)
(defvar debug-next-command-save-debug-on-signal nil)
(defvar debug-next-command-save-debug-on-quit nil)
(defvar debug-next-command-save-debug-ignored-errors nil)

(defvar debug-next-command-stay-on-post-command nil)

(defun debug-next-command-end ()
  "Undo the effects of debug-next-command."
  (if debug-next-command-stay-on-post-command
      (setq debug-next-command-stay-on-post-command nil)
    (setq debug-on-error debug-next-command-save-debug-on-error
	  debug-on-signal debug-next-command-save-debug-on-signal
	  debug-on-quit debug-next-command-save-debug-on-quit
	  debug-ignored-errors debug-next-command-save-debug-ignored-errors)
    (remove-hook 'post-command-hook #'debug-next-command-end)))

(defun debug-next-command ()
  "Produce a bactrace in the next command should an error occur."
  (interactive)
  (setq debug-next-command-save-debug-on-error debug-on-error
	debug-next-command-save-debug-on-signal debug-on-signal
	debug-next-command-save-debug-on-quit debug-on-quit
	debug-next-command-save-debug-ignored-errors debug-ignored-errors)
  (setq debug-on-error t
	debug-on-signal t
	debug-on-quit t
	debug-ignored-errors nil)
  (setq debug-next-command-stay-on-post-command t)
  (add-hook 'post-command-hook #'debug-next-command-end))

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Why is it so difficult to get a Lisp backtrace?

[Stefan Kangas]

Alan Mackenzie <acm@muc.de> writes:

> I mean something like the following, which to a first approximation,
> works.  To use it, do M-x debug-next-command and the execute the command
> expected to give errors:

Good idea, but how about just making it into a manual toggle?  I might
need to run more than one command to reproduce a bug.

Re: Why is it so difficult to get a Lisp backtrace?

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

>> This third group of users is poorly catered for by the current collection
>> of mechanisms.  See also bug #56201, with thanks to Andreas and Lars who
>> helped me get to the bottom of it.  To be reasonably sure of getting a
>> backtrace, it seems one needs to do all of the following:
>> (i) (setq debug-on-error t).
>> (ii) (setq debug-on-signal t).
>> (iii) Bind debug-ignored-errors to nil.
>> (iv) Pray.

You probably also need to ensure that inhibit-redisplay is nil at the
point of the error, I think?  (At least I vaguely recall having to
remove some of those bindings to get a backtrace...)

> I mean something like the following, which to a first approximation,
> works.  To use it, do M-x debug-next-command and the execute the command
> expected to give errors:

I like it, but it may not be useful in all contexts -- errors are used
as a programming mechanism here and there (i.e., throwing an error
symbol to be caught by the caller), and you'll get backtraces from these
non-error errors, too, unfortunately.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Why is it so difficult to get a Lisp backtrace?

[Alan Mackenzie]

Hello, Stefan.

On Sat, Jun 25, 2022 at 17:30:30 +0200, Stefan Kangas wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > I mean something like the following, which to a first approximation,
> > works.  To use it, do M-x debug-next-command and the execute the
> > command expected to give errors:

> Good idea, but how about just making it into a manual toggle?  I might
> need to run more than one command to reproduce a bug.

That's another idea.  It has snags, like for example, you set the toggle,
use it, and forget to cancel it.  You then amend one of the controlling
variables (like debug-on-quit) manually, then cancel the toggle, at which
point confusion and irritation happen.

One idea I had was to have an optional numerical argument, saying how
many commands to leave the thing in place for.  Yet another, conflicting,
idea would be to have an optional boolean argument to exclude
debug-on-signal from the mix, so as not to get too many "expected"
backtraces.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Why is it so difficult to get a Lisp backtrace?

[Alan Mackenzie]

Hello, Lars.

On Sat, Jun 25, 2022 at 17:35:02 +0200, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> >> This third group of users is poorly catered for by the current
> >> collection of mechanisms.  See also bug #56201, with thanks to
> >> Andreas and Lars who helped me get to the bottom of it.  To be
> >> reasonably sure of getting a backtrace, it seems one needs to do
> >> all of the following:
> >> (i) (setq debug-on-error t).
> >> (ii) (setq debug-on-signal t).
> >> (iii) Bind debug-ignored-errors to nil.
> >> (iv) Pray.

> You probably also need to ensure that inhibit-redisplay is nil at the
> point of the error, I think?  (At least I vaguely recall having to
> remove some of those bindings to get a backtrace...)

For example, by testing a flag in the backtrace function, and binding
inhibit-redisplay to nil if that flag is set.  This could get quite
complicate quite quickly.

Another thing I'd like to be able to do is get a backtrace when there's
an error in font-locking during redisplay.  That's a whole order of
magnitude difference in complexity.

> > I mean something like the following, which to a first approximation,
> > works.  To use it, do M-x debug-next-command and the execute the
> > command expected to give errors:

> I like it, but it may not be useful in all contexts -- errors are used
> as a programming mechanism here and there (i.e., throwing an error
> symbol to be caught by the caller), and you'll get backtraces from
> these non-error errors, too, unfortunately.

As I suggested in my reply to Stefan, maybe a C-u argument to the
function could be used to exclude (or include) debug-on-signal from the
mix.  Or something like that.

> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Why is it so difficult to get a Lisp backtrace?

[Stefan Kangas]

Alan Mackenzie <acm@muc.de> writes:

> One idea I had was to have an optional numerical argument, saying how
> many commands to leave the thing in place for.  Yet another, conflicting,
> idea would be to have an optional boolean argument to exclude
> debug-on-signal from the mix, so as not to get too many "expected"
> backtraces.

In my personal use, I'd probably put this behind a timer so that it is
disabled again automatically after 30 seconds (or something).  I do
that for `menu-bar-mode', for example, since I also only ever enable
that temporarily for testing.  But that might be too hacky for general
use.

Re: Why is it so difficult to get a Lisp backtrace?

[Eli Zaretskii]

> Date: Sat, 25 Jun 2022 15:59:52 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Another thing I'd like to be able to do is get a backtrace when there's
> an error in font-locking during redisplay.  That's a whole order of
> magnitude difference in complexity.

The only two ways of having that, AFAIK, are:

  . emit the backtrace int *Messages*
  . add the backtrace to delayed-warnings-list

You _cannot_ have a *backtrace* buffer popping up when the error comes
from some Lisp called by redisplay, because we explicitly disable
that, and for a good reason.

Re: Why is it so difficult to get a Lisp backtrace?

[Alan Mackenzie]

Hello, Eli.

On Sat, Jun 25, 2022 at 19:51:59 +0300, Eli Zaretskii wrote:
> > Date: Sat, 25 Jun 2022 15:59:52 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Another thing I'd like to be able to do is get a backtrace when
> > there's an error in font-locking during redisplay.  That's a whole
> > order of magnitude difference in complexity.

> The only two ways of having that, AFAIK, are:

>   . emit the backtrace int *Messages*
>   . add the backtrace to delayed-warnings-list

Why can't the backtrace be generated and stored, and only displayed at a
subsequent redisplay (probably the next one)?

> You _cannot_ have a *backtrace* buffer popping up when the error comes
> from some Lisp called by redisplay, because we explicitly disable
> that, and for a good reason.

It would increase the complexity of redisplay, making it fragile, for no
good reason, yes.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Why is it so difficult to get a Lisp backtrace?

[Eli Zaretskii]

> Date: Sat, 25 Jun 2022 19:45:49 +0000
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > The only two ways of having that, AFAIK, are:
> 
> >   . emit the backtrace int *Messages*
> >   . add the backtrace to delayed-warnings-list
> 
> Why can't the backtrace be generated and stored, and only displayed at a
> subsequent redisplay (probably the next one)?

That's what the second alternative above already does.

Re: Why is it so difficult to get a Lisp backtrace?

[Stefan Monnier]

> This third group of users is poorly catered for by the current collection
> of mechanisms.  See also bug #56201, with thanks to Andreas and Lars who
> helped me get to the bottom of it.  To be reasonably sure of getting a
> backtrace, it seems one needs to do all of the following:
> (i) (setq debug-on-error t).
> (ii) (setq debug-on-signal t).
> (iii) Bind debug-ignored-errors to nil.
> (iv) Pray.
> (v) Execute the erring command again.

As discussed further in the thread another related problem is those
errors which happen in contexts where we can't popup the debugger.

I've been thinking recently that every time an error is raised, we
should stash the backtrace into some fixed-size table.

This way, when an error happens, you don't need to do steps i..v at all,
you just go and consult the recent backtraces.

I'm not sure I'd do that for all those signals which are caught (just like
I'm not sure step ii should be there in your recipe), but I'd definitely
do that for those errors caught by `with-demoted-errors`.


        Stefan

Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Alan Mackenzie]

Hello, Eli.

On Sun, Jun 26, 2022 at 08:20:47 +0300, Eli Zaretskii wrote:
> > Date: Sat, 25 Jun 2022 19:45:49 +0000
> > Cc: larsi@gnus.org, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > The only two ways of having that, AFAIK, are:

> > >   . emit the backtrace int *Messages*
> > >   . add the backtrace to delayed-warnings-list

> > Why can't the backtrace be generated and stored, and only displayed at a
> > subsequent redisplay (probably the next one)?

> That's what the second alternative above already does.

OK, I've got some code.  :-)

What it does, on a fontification error during redisplay, is to write a
backtrace into the *Backtrace* buffer, attempts a pop-to-buffer (which
isn't working, not surprisingly), and aborts the redisplay.

The basic idea is to note the condition-case handler which is scheduled
to handle any fontification error.  In signal_or_quit, if that is the
handler which is about to handle an error, generate a backtrace instead.

To try it out, set the new variable backtrace-on-fontification-error to
non-nil, and tweak some code to generate an error during fontification.

As already noted, the *Backtrace* buffer doesn't appear on the screen,
yet.  Any hints on how to achieve this would be most welcome.  It seems I
could really do with after-redisplay-hook, in which I could call
pop-to-buffer, then cause redisplay to be restarted.

Maybe this code could go into Emacs 29, once it's working properly.



diff --git a/src/dispextern.h b/src/dispextern.h
index c7399ca299..4bf6fb843f 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3406,6 +3406,7 @@ void move_it_in_display_line (struct it *it,
 int partial_line_height (struct it *it_origin);
 bool in_display_vector_p (struct it *);
 int frame_mode_line_height (struct frame *);
+extern struct handler *redisplay_deep_handler;
 extern bool redisplaying_p;
 extern bool help_echo_showing_p;
 extern Lisp_Object help_echo_string, help_echo_window;
diff --git a/src/eval.c b/src/eval.c
index 346dff8bdc..ad5eb9dc7e 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1550,9 +1550,12 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object err,
 						ptrdiff_t nargs,
-						Lisp_Object *args))
+						Lisp_Object *args),
+			   struct handler **handler)
 {
   struct handler *c = push_handler (handlers, CONDITION_CASE);
+  if (handler)
+    *handler = c;
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1812,6 +1815,32 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool keyboard_quit)
       unbind_to (count, Qnil);
     }
 
+  /* If an error is signalled during fontification in redisplay, print
+     a backtrace into the buffer *Backtrace*.  */
+  if (!debugger_called && !NILP (error_symbol)
+      && redisplay_fontifying
+      && backtrace_on_fontification_error
+      && (NILP (clause) || h == redisplay_deep_handler)
+      && NILP (Vinhibit_debugger)
+      && !NILP (Ffboundp (Qdebug_early)))
+    {
+      max_ensure_room (&max_lisp_eval_depth, lisp_eval_depth, 100);
+      specpdl_ref count = SPECPDL_INDEX ();
+      ptrdiff_t counti = specpdl_ref_to_count (count);
+      Lisp_Object backtrace_buffer;
+      max_ensure_room (&max_specpdl_size, counti, 200);
+      backtrace_buffer = Fget_buffer_create (make_string ("*Backtrace*", 11),
+					     Qnil);
+      current_buffer = XBUFFER (backtrace_buffer);
+      Ferase_buffer ();
+      specbind (Qstandard_output, backtrace_buffer);
+      specbind (Qdebugger, Qdebug_early);
+      call_debugger (list2 (Qerror, Fcons (error_symbol, data)));
+      unbind_to (count, Qnil);
+      call1 (intern ("pop-to-buffer"), backtrace_buffer);
+      Fthrow (Qtop_level, Qt);	/* This might not be needed. */
+    }
+
   if (!NILP (clause))
     {
       Lisp_Object unwind_data
@@ -4269,6 +4298,12 @@ Does not apply if quit is handled by a `condition-case'.  */);
   DEFVAR_BOOL ("debug-on-next-call", debug_on_next_call,
 	       doc: /* Non-nil means enter debugger before next `eval', `apply' or `funcall'.  */);
 
+  DEFVAR_BOOL ("backtrace-on-fontification-error", backtrace_on_fontification_error,
+	       doc: /* Non-nil means show a backtrace if a fontification error occurs in redisplay.
+When this happens, it causes the current redisplay to be aborted and a new one
+including the *Backtrace* buffer to be performed.  */);
+  backtrace_on_fontification_error = false;
+
   DEFVAR_BOOL ("debugger-may-continue", debugger_may_continue,
 	       doc: /* Non-nil means debugger may continue execution.
 This is nil when the debugger is called under circumstances where it
diff --git a/src/keyboard.c b/src/keyboard.c
index e62b2e56d3..fdce343d6f 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1878,7 +1878,7 @@ safe_run_hook_funcall (ptrdiff_t nargs, Lisp_Object *args)
   /* Yes, run_hook_with_args works with args in the other order.  */
   internal_condition_case_n (safe_run_hooks_1,
 			     2, ((Lisp_Object []) {args[1], args[0]}),
-			     Qt, safe_run_hooks_error);
+			     Qt, safe_run_hooks_error, NULL);
   return Qnil;
 }
 
diff --git a/src/lisp.h b/src/lisp.h
index 05b0754ff6..16830c7d8e 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4558,7 +4558,7 @@ extern Lisp_Object internal_condition_case_1 (Lisp_Object (*) (Lisp_Object), Lis
 extern Lisp_Object internal_condition_case_2 (Lisp_Object (*) (Lisp_Object, Lisp_Object), Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object (*) (Lisp_Object));
 extern Lisp_Object internal_condition_case_n
     (Lisp_Object (*) (ptrdiff_t, Lisp_Object *), ptrdiff_t, Lisp_Object *,
-     Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *));
+     Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *), struct handler **);
 extern Lisp_Object internal_catch_all (Lisp_Object (*) (void *), void *, Lisp_Object (*) (enum nonlocal_exit, Lisp_Object));
 extern struct handler *push_handler (Lisp_Object, enum handlertype)
   ATTRIBUTE_RETURNS_NONNULL;
diff --git a/src/xdisp.c b/src/xdisp.c
index 90809ac3ab..6b2d09efb2 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -615,6 +615,9 @@ fill_column_indicator_column (struct it *it, int char_width)
 
 bool noninteractive_need_newline;
 
+/* The handler structure which will catch errors in fontification code.  */
+struct handler *redisplay_deep_handler;
+
 /* True means print newline to message log before next message.  */
 
 static bool message_log_need_newline;
@@ -3013,7 +3016,8 @@ safe__call (bool inhibit_quit, ptrdiff_t nargs, Lisp_Object func, va_list ap)
       /* Use Qt to ensure debugger does not run,
 	 so there is no possibility of wanting to redisplay.  */
       val = internal_condition_case_n (Ffuncall, nargs, args, Qt,
-				       safe_eval_handler);
+				       safe_eval_handler,
+				       &redisplay_deep_handler);
       val = SAFE_FREE_UNBIND_TO (count, val);
     }
 
@@ -4320,6 +4324,7 @@ handle_fontified_prop (struct it *it)
 
       val = Vfontification_functions;
       specbind (Qfontification_functions, Qnil);
+      specbind (Qredisplay_fontifying, Qt);
 
       eassert (it->end_charpos == ZV);
 
@@ -26431,7 +26436,7 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 						   Flength (elt),
 						   props,
 						   elt}),
-					       Qt, safe_eval_handler);
+					       Qt, safe_eval_handler, NULL);
 		    /* Add this item to mode_line_proptrans_alist.  */
 		    mode_line_proptrans_alist
 		      = Fcons (Fcons (elt, props),
@@ -35704,6 +35709,7 @@ be let-bound around code that needs to disable messages temporarily. */);
   DEFSYM (QCfile, ":file");
   DEFSYM (Qfontified, "fontified");
   DEFSYM (Qfontification_functions, "fontification-functions");
+  DEFSYM (Qredisplay_fontifying, "redisplay-fontifying");
 
   /* Name of the symbol which disables Lisp evaluation in 'display'
      properties.  This is used by enriched.el.  */
@@ -36221,6 +36227,10 @@ fontified regions the property `fontified'.  */);
   Vfontification_functions = Qnil;
   Fmake_variable_buffer_local (Qfontification_functions);
 
+  DEFVAR_BOOL ("redisplay-fontifying", redisplay_fontifying,
+    doc: /* Non-nil when fontification invoked from redisplay is in progress.  */);
+  redisplay_fontifying = false;
+
   DEFVAR_BOOL ("unibyte-display-via-language-environment",
                unibyte_display_via_language_environment,
     doc: /* Non-nil means display unibyte text according to language environment.
diff --git a/src/xfns.c b/src/xfns.c
index 1372809da6..dc18cfb4ae 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -3405,7 +3405,7 @@ x_xim_text_to_utf8_unix (XIMText *text, ptrdiff_t *length)
   waiting_for_input = false;
   arg = make_mint_ptr (&data);
   internal_condition_case_n (x_xim_text_to_utf8_unix_1, 1, &arg,
-			     Qt, x_xim_text_to_utf8_unix_2);
+			     Qt, x_xim_text_to_utf8_unix_2, NULL);
   waiting_for_input = was_waiting_for_input_p;
 
   *length = coding.produced;


-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> What it does, on a fontification error during redisplay, is to write a
> backtrace into the *Backtrace* buffer, attempts a pop-to-buffer (which
> isn't working, not surprisingly), and aborts the redisplay.

Sounds promising -- it's very difficult to debug fontification errors
currently, so this would be very welcome.

> As already noted, the *Backtrace* buffer doesn't appear on the screen,
> yet.  Any hints on how to achieve this would be most welcome.

Is `inhibit-redisplay' non-nil at this point?  In that case, `debug'
will just exit immediately...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Eli Zaretskii]

> Date: Mon, 27 Jun 2022 19:48:45 +0000
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > > The only two ways of having that, AFAIK, are:
> 
> > > >   . emit the backtrace int *Messages*
> > > >   . add the backtrace to delayed-warnings-list
> 
> > > Why can't the backtrace be generated and stored, and only displayed at a
> > > subsequent redisplay (probably the next one)?
> 
> > That's what the second alternative above already does.
> 
> OK, I've got some code.  :-)

Sigh.  I suggested two alternatives, but you effectively
ignored/rejected both of them.  Why?

> What it does, on a fontification error during redisplay, is to write a
> backtrace into the *Backtrace* buffer, attempts a pop-to-buffer (which
> isn't working, not surprisingly), and aborts the redisplay.

I don't understand why you needed to do it this way.

First, what's wrong with the two ways I suggested?  They are simple to
implement, use existing infrastructure, don't require any changes in
the APIs, and are safe.  Why didn't you use one of them?

Second, if you _must_ roll out something new (but please explain why),
why do you need a special handler? why not slightly modify the
existing handler, safe_eval_handler?  Doing that would have avoided
the need to change existing APIs.

Third, why are you singling out the fontifications? they are not the
only source of Lisp errors in code called by the display engine.
Moreover, handle_fontified_prop is not the only way of calling
expensive Lisp code that deals with faces.

Forth, why on Earth do you want to abort redisplay??  Once you
collected the backtrace, there should be no need in such drastic
measures.  I've just finished adding a feature that _really_ needs to
abort redisplay, and I can tell you: it's NOT safe!  It can easily
crash Emacs within a few commands, and your patch didn't do what I
needed to do to avoid that.  You just throw to top-level from the
bowels of the display code, assuming that it's okay.  Well, it isn't,
not if handle_fontified_prop is called from redisplay_internal and its
subroutines!

So basically, I'm extremely confused, to say the least, by the
proposed changes and by your rationale, and am not happy at all with
installing them.

> As already noted, the *Backtrace* buffer doesn't appear on the screen,
> yet.  Any hints on how to achieve this would be most welcome.

Any reasons you'd want that?  Once the buffer exists, why do you need
to actually pop it up (and thus potentially disrupt the very
window/frame configuration which you are trying to debug)?

> It seems I could really do with after-redisplay-hook, in which I
> could call pop-to-buffer, then cause redisplay to be restarted.

I think you misunderstand what such a hook could do, and why it could
be useful, if at all, in the first place.  If all you want is to cause
the main loop perform some command after redisplay cycle ends, we
already have several mechanisms in place for doing that.

But all that is secondary.  We should first agree on the correct
design and implementation of the main part, which is how to handle
these errors for collecting Lisp backtrace information.

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Alan Mackenzie]

Hello, Eli.

On Tue, Jun 28, 2022 at 14:57:44 +0300, Eli Zaretskii wrote:
> > Date: Mon, 27 Jun 2022 19:48:45 +0000
> > Cc: larsi@gnus.org, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > > > The only two ways of having that, AFAIK, are:

> > > > >   . emit the backtrace int *Messages*
> > > > >   . add the backtrace to delayed-warnings-list

> > > > Why can't the backtrace be generated and stored, and only displayed at a
> > > > subsequent redisplay (probably the next one)?

> > > That's what the second alternative above already does.

> > OK, I've got some code.  :-)

> Sigh.  I suggested two alternatives, but you effectively
> ignored/rejected both of them.  Why?

*Messages* just doesn't feel the right place to put a backtrace.

delayed-warnings-list consists of a list of structured lists, rather than
the vanilla text output by debug-early.  So the backtrace would need some
massaging to fit into delayed-warnings-list.  Also these delayed warnings
get displayed between post-command-hook and redisplay, which is not an
optimal time to display a backtrace generated in redisplay.

However, the display of the backtrace was not the difficult bit in
writing this code - that was triggering the backtrace when there was an
error signalled, and only then.

> > What it does, on a fontification error during redisplay, is to write a
> > backtrace into the *Backtrace* buffer, attempts a pop-to-buffer (which
> > isn't working, not surprisingly), and aborts the redisplay.

> I don't understand why you needed to do it this way.

I copied it from the debug-on-error backtrace.  It's just my first try at
getting the thing displayed.  I'm not at all experienced in creating and
displaying windows.

> First, what's wrong with the two ways I suggested?  They are simple to
> implement, use existing infrastructure, don't require any changes in
> the APIs, and are safe.  Why didn't you use one of them?

See above.

> Second, if you _must_ roll out something new (but please explain why),
> why do you need a special handler? why not slightly modify the
> existing handler, safe_eval_handler?  Doing that would have avoided
> the need to change existing APIs.

That way hadn't occurred to me.  It would surely be better than
yesterday's patch.

> Third, why are you singling out the fontifications? they are not the
> only source of Lisp errors in code called by the display engine.
> Moreover, handle_fontified_prop is not the only way of calling
> expensive Lisp code that deals with faces.

All that's true, yet fontification errors are an important source of
difficult to debug errors.  We could certainly extend the new backtrace
mechanism to handle these other types of bug, too.

> Forth, why on Earth do you want to abort redisplay??  Once you
> collected the backtrace, there should be no need in such drastic
> measures.

There's really no need for this, thinking about it.  Given that the
condition-case has handled the error, there's nothing stopping the
redisplay from continuing successfully on the other visible windows.

> I've just finished adding a feature that _really_ needs to abort
> redisplay, and I can tell you: it's NOT safe!  It can easily crash
> Emacs within a few commands, and your patch didn't do what I needed to
> do to avoid that.  You just throw to top-level from the bowels of the
> display code, assuming that it's okay.  Well, it isn't, not if
> handle_fontified_prop is called from redisplay_internal and its
> subroutines!

OK.

> So basically, I'm extremely confused, to say the least, by the
> proposed changes and by your rationale, and am not happy at all with
> installing them.

My patch was more a proof of concept, a demonstration that a backtrace
from within redisplay is possible.  It wasn't intended to be finished
code, and clearly isn't in that state, yet.

> > As already noted, the *Backtrace* buffer doesn't appear on the screen,
> > yet.  Any hints on how to achieve this would be most welcome.

> Any reasons you'd want that?  Once the buffer exists, why do you need
> to actually pop it up (and thus potentially disrupt the very
> window/frame configuration which you are trying to debug)?

A debug-on-error backtrace gets displayed immediately, so it would help
consistency.  If the window where the error occurred isn't occupying its
entire frame, there should be room to put the backtrace somewhere else, I
think.  I'm not at all experienced with things like pop-to-buffer, but
they're supposed to be very flexible.

> > It seems I could really do with after-redisplay-hook, in which I
> > could call pop-to-buffer, then cause redisplay to be restarted.

> I think you misunderstand what such a hook could do, and why it could
> be useful, if at all, in the first place.  If all you want is to cause
> the main loop perform some command after redisplay cycle ends, we
> already have several mechanisms in place for doing that.

That's what I want, yes, but couldn't find any such mechanism anywhere
when I looked.

> But all that is secondary.  We should first agree on the correct
> design and implementation of the main part, which is how to handle
> these errors for collecting Lisp backtrace information.

Yes.  How about what you suggested above, namely creating the backtrace
in save_eval_handler rather than signal_or_quit, at least as a second
attempt.  Though I don't think this would handle errors in other Lisp
called from redisplay.

Thanks for such a full review and helpful criticism.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Eli Zaretskii]

> Date: Tue, 28 Jun 2022 17:28:38 +0000
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > > > >   . emit the backtrace int *Messages*
> > > > > >   . add the backtrace to delayed-warnings-list
> 
> > > > > Why can't the backtrace be generated and stored, and only displayed at a
> > > > > subsequent redisplay (probably the next one)?
> 
> > > > That's what the second alternative above already does.
> 
> > > OK, I've got some code.  :-)
> 
> > Sigh.  I suggested two alternatives, but you effectively
> > ignored/rejected both of them.  Why?
> 
> *Messages* just doesn't feel the right place to put a backtrace.

That's where we currently log all the errors during redisplay, since
Emacs 21.1.  So why this one is different?

But you _really_ dislike *Messages*, then put the stuff into another
buffer, like *backtrace-in-redisplay* or somesuch.

> delayed-warnings-list consists of a list of structured lists, rather than
> the vanilla text output by debug-early.

Why is it a problem?  You could again stash the backtrace somewhere
and only show the name of that buffer in the warning text.  Using
delayed-warnings-list makes sure the problem is not missed.

> Also these delayed warnings get displayed between post-command-hook
> and redisplay, which is not an optimal time to display a backtrace
> generated in redisplay.

The buffer with warnings pops up very soon after redisplay, which is
more-or-less exactly what you wanted?

> > Third, why are you singling out the fontifications? they are not the
> > only source of Lisp errors in code called by the display engine.
> > Moreover, handle_fontified_prop is not the only way of calling
> > expensive Lisp code that deals with faces.
> 
> All that's true, yet fontification errors are an important source of
> difficult to debug errors.  We could certainly extend the new backtrace
> mechanism to handle these other types of bug, too.

It makes very little sense to me to create infrastructure that caters
only to your current and immediate problem.  If we introduce such a
feature, it should work with any Lisp called from redisplay.

> > > As already noted, the *Backtrace* buffer doesn't appear on the screen,
> > > yet.  Any hints on how to achieve this would be most welcome.
> 
> > Any reasons you'd want that?  Once the buffer exists, why do you need
> > to actually pop it up (and thus potentially disrupt the very
> > window/frame configuration which you are trying to debug)?
> 
> A debug-on-error backtrace gets displayed immediately, so it would help
> consistency.  If the window where the error occurred isn't occupying its
> entire frame, there should be room to put the backtrace somewhere else, I
> think.  I'm not at all experienced with things like pop-to-buffer, but
> they're supposed to be very flexible.

I think showing a delayed-warning with the text that mentions the
buffer with the backtrace is completely adequate.  It stops short of
popping the window with a backtrace itself, but that could be added
later, if necessary, perhaps as a feature of delayed-warning.

> > > It seems I could really do with after-redisplay-hook, in which I
> > > could call pop-to-buffer, then cause redisplay to be restarted.
> 
> > I think you misunderstand what such a hook could do, and why it could
> > be useful, if at all, in the first place.  If all you want is to cause
> > the main loop perform some command after redisplay cycle ends, we
> > already have several mechanisms in place for doing that.
> 
> That's what I want, yes, but couldn't find any such mechanism anywhere
> when I looked.

One thing that comes to mind is a timer.  Another thing is pushing
something onto unread-command-events.  I'm sure there are more.

> Yes.  How about what you suggested above, namely creating the backtrace
> in save_eval_handler rather than signal_or_quit, at least as a second
> attempt.  Though I don't think this would handle errors in other Lisp
> called from redisplay.

It will, because any Lisp we call goes though safe_eval.  The only
reason why your original proposal didn't was because it bound the
variable which enables this only in handle_fontified_prop.  But we
don't need this flag if the logic to decide what to do with the error
will be in save_eval_handler.

And you need to consider one more factor: code from display engine,
including fontification via handle_fontified_prop, is called also from
user commands, not via redisplay_internal.  For example, commands like
C-n and C-v invoke display code.  Errors signaled there _can_ be left
to go to top-level and probably could call the debugger.  To test
whether any given code was called from redisplay_internal, test the
value of the variable redisplaying_p.

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Alan Mackenzie]

Hello, Eli.

On Tue, Jun 28, 2022 at 21:17:04 +0300, Eli Zaretskii wrote:
> > Date: Tue, 28 Jun 2022 17:28:38 +0000
> > Cc: larsi@gnus.org, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > > > > >   . emit the backtrace int *Messages*
> > > > > > >   . add the backtrace to delayed-warnings-list

> > > > > > Why can't the backtrace be generated and stored, and only displayed at a
> > > > > > subsequent redisplay (probably the next one)?

> > > > > That's what the second alternative above already does.

> > > > OK, I've got some code.  :-)

> > > Sigh.  I suggested two alternatives, but you effectively
> > > ignored/rejected both of them.  Why?

> > *Messages* just doesn't feel the right place to put a backtrace.

> That's where we currently log all the errors during redisplay, since
> Emacs 21.1.  So why this one is different?

It's not a single line, or a small number of lines.  It could potentially
be a stack overflow error, generating a large backtrace, which might have
more lines than message-log-max.

> But you _really_ dislike *Messages*, then put the stuff into another
> buffer, like *backtrace-in-redisplay* or somesuch.

I'm currently using the buffer *Backtrace*.  Is there anything wrong with
that?

> > delayed-warnings-list consists of a list of structured lists, rather than
> > the vanilla text output by debug-early.

> Why is it a problem?  You could again stash the backtrace somewhere
> and only show the name of that buffer in the warning text.  Using
> delayed-warnings-list makes sure the problem is not missed.

> > Also these delayed warnings get displayed between post-command-hook
> > and redisplay, which is not an optimal time to display a backtrace
> > generated in redisplay.

> The buffer with warnings pops up very soon after redisplay, which is
> more-or-less exactly what you wanted?

OK, I've never used it.  On reading the documentation, I expected it to
wait until after the next command.

> > > Third, why are you singling out the fontifications? they are not the
> > > only source of Lisp errors in code called by the display engine.
> > > Moreover, handle_fontified_prop is not the only way of calling
> > > expensive Lisp code that deals with faces.

> > All that's true, yet fontification errors are an important source of
> > difficult to debug errors.  We could certainly extend the new backtrace
> > mechanism to handle these other types of bug, too.

> It makes very little sense to me to create infrastructure that caters
> only to your current and immediate problem.  If we introduce such a
> feature, it should work with any Lisp called from redisplay.

OK.  It shouldn't actually be much more difficult to keep the new
facility generic.

> > > > As already noted, the *Backtrace* buffer doesn't appear on the screen,
> > > > yet.  Any hints on how to achieve this would be most welcome.

> > > Any reasons you'd want that?  Once the buffer exists, why do you need
> > > to actually pop it up (and thus potentially disrupt the very
> > > window/frame configuration which you are trying to debug)?

> > A debug-on-error backtrace gets displayed immediately, so it would help
> > consistency.  If the window where the error occurred isn't occupying its
> > entire frame, there should be room to put the backtrace somewhere else, I
> > think.  I'm not at all experienced with things like pop-to-buffer, but
> > they're supposed to be very flexible.

> I think showing a delayed-warning with the text that mentions the
> buffer with the backtrace is completely adequate.  It stops short of
> popping the window with a backtrace itself, but that could be added
> later, if necessary, perhaps as a feature of delayed-warning.

I think that would be a good compromise.

> > > > It seems I could really do with after-redisplay-hook, in which I
> > > > could call pop-to-buffer, then cause redisplay to be restarted.

> > > I think you misunderstand what such a hook could do, and why it could
> > > be useful, if at all, in the first place.  If all you want is to cause
> > > the main loop perform some command after redisplay cycle ends, we
> > > already have several mechanisms in place for doing that.

> > That's what I want, yes, but couldn't find any such mechanism anywhere
> > when I looked.

> One thing that comes to mind is a timer.  Another thing is pushing
> something onto unread-command-events.  I'm sure there are more.

OK, yes, maybe a timer would be the thing.

> > Yes.  How about what you suggested above, namely creating the backtrace
> > in save_eval_handler rather than signal_or_quit, at least as a second
> > attempt.  Though I don't think this would handle errors in other Lisp
> > called from redisplay.

Now I remember why I created the backtrace in signal_or_quit - it needs
to be done before the stack gets unwound, which happens later in
signal_or_quit.  On return to save_eval_handler is just too late for
this.

> It will, because any Lisp we call goes though safe_eval.  The only
> reason why your original proposal didn't was because it bound the
> variable which enables this only in handle_fontified_prop.  But we
> don't need this flag if the logic to decide what to do with the error
> will be in save_eval_handler.

Then the mechanism I've implemented, namely to set redisplay_deep_handler
to the handler which would handle an error, could be made to serve for
other sorts of Lisp code.  Probably I'll need to rename some variables
whose name contains "fontification" and suchlike.

> And you need to consider one more factor: code from display engine,
> including fontification via handle_fontified_prop, is called also from
> user commands, not via redisplay_internal.  For example, commands like
> C-n and C-v invoke display code.  Errors signaled there _can_ be left
> to go to top-level and probably could call the debugger.  To test
> whether any given code was called from redisplay_internal, test the
> value of the variable redisplaying_p.

OK, this is a further complication.  But it would be good to get to the
debugger if possible, rather than just having a "historical" backtrace.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Eli Zaretskii]

> Date: Tue, 28 Jun 2022 19:31:59 +0000
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > *Messages* just doesn't feel the right place to put a backtrace.
> 
> > That's where we currently log all the errors during redisplay, since
> > Emacs 21.1.  So why this one is different?
> 
> It's not a single line, or a small number of lines.  It could potentially
> be a stack overflow error, generating a large backtrace, which might have
> more lines than message-log-max.

We could temporarily bind message-log-max to a larger value, if that's
the problem.  But I really doubt that something like this could be an
issue in practice.

> > But you _really_ dislike *Messages*, then put the stuff into another
> > buffer, like *backtrace-in-redisplay* or somesuch.
> 
> I'm currently using the buffer *Backtrace*.  Is there anything wrong with
> that?

Not that I see, no.

> > The buffer with warnings pops up very soon after redisplay, which is
> > more-or-less exactly what you wanted?
> 
> OK, I've never used it.  On reading the documentation, I expected it to
> wait until after the next command.

That's because the ELisp reference manual describes it from the POV of
setting up the warning as part of some command.  And even then, it
says:

     The value of this variable is a list of warnings to be displayed
     after the current command has finished.

"Current command", not "next command".

If you add a warning to the list inside redisplay, it will pop up
after redisplay returns.

> Now I remember why I created the backtrace in signal_or_quit - it needs
> to be done before the stack gets unwound, which happens later in
> signal_or_quit.  On return to save_eval_handler is just too late for
> this.

That's orthogonal, I think?  You can collect the data inside
signal_or_quit, and the signal handler then only needs to handle the
error gracefully after it adds the warning.

> > It will, because any Lisp we call goes though safe_eval.  The only
> > reason why your original proposal didn't was because it bound the
> > variable which enables this only in handle_fontified_prop.  But we
> > don't need this flag if the logic to decide what to do with the error
> > will be in save_eval_handler.
> 
> Then the mechanism I've implemented, namely to set redisplay_deep_handler
> to the handler which would handle an error, could be made to serve for
> other sorts of Lisp code.

It could, but I see no reason for having a new handler.

> > And you need to consider one more factor: code from display engine,
> > including fontification via handle_fontified_prop, is called also from
> > user commands, not via redisplay_internal.  For example, commands like
> > C-n and C-v invoke display code.  Errors signaled there _can_ be left
> > to go to top-level and probably could call the debugger.  To test
> > whether any given code was called from redisplay_internal, test the
> > value of the variable redisplaying_p.
> 
> OK, this is a further complication.  But it would be good to get to the
> debugger if possible, rather than just having a "historical" backtrace.

If the display code is called from "normal" commands, you can.

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Stefan Monnier]

>> That's where we currently log all the errors during redisplay, since
>> Emacs 21.1.  So why this one is different?
>
> It's not a single line, or a small number of lines.  It could potentially
> be a stack overflow error, generating a large backtrace, which might have
> more lines than message-log-max.

How 'bout stashing the backtrace data in a var (without turning it into
text yet; that should always be quick, safe, and painless) and then
adding a button in the *Messages* (or *Warnings*) next to the error
itself such that pressing the buttong shows you the actual backtrace?

We could even try and get fancy: instead of only stashing the
`backtrace-frames`, we could also stash a copy of the specpdl so we
could bring up a *Backtrace* buffer which isn't quite "live" but can
still be used to some extent to see the values of local vars (and
`current-buffer`) in the different activation frames.

BTW, in order to debug fontification errors, we also have
`jit-locak-debug-mode` which postpones the jit/font-lock execution from
within redisplay to "just a bit later" such that it can use the
debugger.  IIRC it still has some rough edges in some cases, but in
theory it should be possible to make this work such that you can (for
example) Edebug `font-lock.el` itself.


        Stefan

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  larsi@gnus.org,  emacs-devel@gnu.org
> Date: Thu, 30 Jun 2022 16:34:23 -0400
> 
> BTW, in order to debug fontification errors, we also have
> `jit-locak-debug-mode` which postpones the jit/font-lock execution from
> within redisplay to "just a bit later" such that it can use the
> debugger.  IIRC it still has some rough edges in some cases, but in
> theory it should be possible to make this work such that you can (for
> example) Edebug `font-lock.el` itself.

It would be nice to have this documented in the ELisp manual.  I can
easily add the jit-lock-debug-mode variable there, but from what you
write, there seems to be more wisdom to go with its usage.

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Juri Linkov]

>> BTW, in order to debug fontification errors, we also have
>> `jit-locak-debug-mode` which postpones the jit/font-lock execution from
>> within redisplay to "just a bit later" such that it can use the
>> debugger.  IIRC it still has some rough edges in some cases, but in
>> theory it should be possible to make this work such that you can (for
>> example) Edebug `font-lock.el` itself.
>
> It would be nice to have this documented in the ELisp manual.  I can
> easily add the jit-lock-debug-mode variable there, but from what you
> write, there seems to be more wisdom to go with its usage.

Also would be nice to introduce a "log level" with possible values
Fatal, Error, Warn, Info, Debug, Trace.  Then depending on the
customized value decide whether print the complete backtrace to
the *Messages* buffer, or only the error message, or nothing at all.

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  acm@muc.de,  larsi@gnus.org,
>  emacs-devel@gnu.org
> Date: Fri, 01 Jul 2022 18:34:01 +0300
> 
> >> BTW, in order to debug fontification errors, we also have
> >> `jit-locak-debug-mode` which postpones the jit/font-lock execution from
> >> within redisplay to "just a bit later" such that it can use the
> >> debugger.  IIRC it still has some rough edges in some cases, but in
> >> theory it should be possible to make this work such that you can (for
> >> example) Edebug `font-lock.el` itself.
> >
> > It would be nice to have this documented in the ELisp manual.  I can
> > easily add the jit-lock-debug-mode variable there, but from what you
> > write, there seems to be more wisdom to go with its usage.
> 
> Also would be nice to introduce a "log level" with possible values
> Fatal, Error, Warn, Info, Debug, Trace.  Then depending on the
> customized value decide whether print the complete backtrace to
> the *Messages* buffer, or only the error message, or nothing at all.

I don't necessarily object, but I'm asking myself who'd want less than
the complete information.  And I don't find any good answer; it seems
to me that whenever one gets the less detailed report, one will
immediately want the most detailed one.

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Juri Linkov]

>> Also would be nice to introduce a "log level" with possible values
>> Fatal, Error, Warn, Info, Debug, Trace.  Then depending on the
>> customized value decide whether print the complete backtrace to
>> the *Messages* buffer, or only the error message, or nothing at all.
>
> I don't necessarily object, but I'm asking myself who'd want less than
> the complete information.  And I don't find any good answer; it seems
> to me that whenever one gets the less detailed report, one will
> immediately want the most detailed one.

More information is better than less, indeed.  Then in case
someone worries about flooding the Messages buffer, maybe at least
hide the complete information, then provide a button to unhide it.

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Stefan Monnier]

>> BTW, in order to debug fontification errors, we also have
>> `jit-locak-debug-mode` which postpones the jit/font-lock execution from
>> within redisplay to "just a bit later" such that it can use the
>> debugger.  IIRC it still has some rough edges in some cases, but in
>> theory it should be possible to make this work such that you can (for
>> example) Edebug `font-lock.el` itself.
>
> It would be nice to have this documented in the ELisp manual.  I can
> easily add the jit-lock-debug-mode variable there, but from what you
> write, there seems to be more wisdom to go with its usage.

No, there's no extra wisdom.  It's just that it hasn't been used much,
so it probably needs refinement.  Bug reports would be quite welcome to
figure out what needs to be improved.


        Stefan

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: acm@muc.de,  larsi@gnus.org,  emacs-devel@gnu.org
> Date: Fri, 01 Jul 2022 15:26:26 -0400
> 
> >> BTW, in order to debug fontification errors, we also have
> >> `jit-locak-debug-mode` which postpones the jit/font-lock execution from
> >> within redisplay to "just a bit later" such that it can use the
> >> debugger.  IIRC it still has some rough edges in some cases, but in
> >> theory it should be possible to make this work such that you can (for
> >> example) Edebug `font-lock.el` itself.
> >
> > It would be nice to have this documented in the ELisp manual.  I can
> > easily add the jit-lock-debug-mode variable there, but from what you
> > write, there seems to be more wisdom to go with its usage.
> 
> No, there's no extra wisdom.

Then what did you mean by "some rough edges in some cases"?

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Alan Mackenzie]

Hello, Stefan.

On Thu, Jun 30, 2022 at 16:34:23 -0400, Stefan Monnier wrote:
> >> That's where we currently log all the errors during redisplay, since
> >> Emacs 21.1.  So why this one is different?

> > It's not a single line, or a small number of lines.  It could potentially
> > be a stack overflow error, generating a large backtrace, which might have
> > more lines than message-log-max.

> How 'bout stashing the backtrace data in a var (without turning it into
> text yet; that .....

Does "that" mean the stashing or the turning into text?

> .... should always be quick, safe, and painless) and then adding a
> button in the *Messages* (or *Warnings*) next to the error itself such
> that pressing the buttong shows you the actual backtrace?

I'm not sure the stashing of the backtrace data is a good idea.  It's
likely to be BIG (see above), and it's fragile - the only place to
record it is in signal_or_quit, before it gets discarded by a call to
unwind_to_catch.  The need to know its precise format is obviated by the
C function mapbacktrace, so we might as well just dump out the backtrace
as text in *Backtrace* at the first opportunity.  There's nothing time
critical about the said dumping.

> We could even try and get fancy: instead of only stashing the
> `backtrace-frames`, we could also stash a copy of the specpdl so we
> could bring up a *Backtrace* buffer which isn't quite "live" but can
> still be used to some extent to see the values of local vars (and
> `current-buffer`) in the different activation frames.

I think that's very ambitious.  Surely the time for that is when the
basic backtrace functionality is working, and the need for something
more sopisticated is perceived.

> BTW, in order to debug fontification errors, we also have
> `jit-locak-debug-mode` which postpones the jit/font-lock execution from
> within redisplay to "just a bit later" such that it can use the
> debugger.  IIRC it still has some rough edges in some cases, but in
> theory it should be possible to make this work such that you can (for
> example) Edebug `font-lock.el` itself.

It does work, though, doesn't it?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Stefan Monnier]

Alan Mackenzie [2022-07-02 20:27:32] wrote:
> Hello, Stefan.
> On Thu, Jun 30, 2022 at 16:34:23 -0400, Stefan Monnier wrote:
>> >> That's where we currently log all the errors during redisplay, since
>> >> Emacs 21.1.  So why this one is different?
>
>> > It's not a single line, or a small number of lines.  It could potentially
>> > be a stack overflow error, generating a large backtrace, which might have
>> > more lines than message-log-max.
>> How 'bout stashing the backtrace data in a var (without turning it into
>> text yet; that .....
> Does "that" mean the stashing or the turning into text?

It meant "stashing".

>> .... should always be quick, safe, and painless) and then adding a
>> button in the *Messages* (or *Warnings*) next to the error itself such
>> that pressing the buttong shows you the actual backtrace?
> I'm not sure the stashing of the backtrace data is a good idea.  It's
> likely to be BIG (see above), and it's fragile

I don't see the "above" that explains why you think it's going to
be BIG.  Neither do I understand why you think it would be fragile?

> - the only place to record it is in signal_or_quit, before it gets
> discarded by a call to unwind_to_catch.  The need to know its precise
> format is obviated by the C function mapbacktrace, so we might as well
> just dump out the backtrace as text in *Backtrace* at the first
> opportunity.  There's nothing time critical about the said dumping.

As I mentioned in another message, I think it would make sense to
"always" stash backtraces when we get an unhandled error, and then to
offer a way to investigate these "post mortem".  Think of it as Emacs's
equivalent of Unix's core dumps.

>> We could even try and get fancy: instead of only stashing the
>> `backtrace-frames`, we could also stash a copy of the specpdl so we
>> could bring up a *Backtrace* buffer which isn't quite "live" but can
>> still be used to some extent to see the values of local vars (and
>> `current-buffer`) in the different activation frames.
> I think that's very ambitious.

I don't think it's should be terribly hard.  We already support multiple
specpdl and switching between them for the purpose of threads, so most
of the delicate part of the code is already written.

>> BTW, in order to debug fontification errors, we also have
>> `jit-locak-debug-mode` which postpones the jit/font-lock execution from
>> within redisplay to "just a bit later" such that it can use the
>> debugger.  IIRC it still has some rough edges in some cases, but in
>> theory it should be possible to make this work such that you can (for
>> example) Edebug `font-lock.el` itself.
>
> It does work, though, doesn't it?

Yes.


        Stefan

Re: Fontification error backtrace [Was: Why is it so difficult to get a Lisp backtrace?]

[Stefan Monnier]

> Then what did you mean by "some rough edges in some cases"?

I have a vague recollection of bumping into problems where
font-lock would keep being invoked while we're inside the debugger,
making it hard to see what happens in what order.


        Stefan

Redisplay hook error bactraces [Was: Fontification error backtrace]

[Alan Mackenzie]

Hello, Eli.

I've amended the patch I previously submitted considerably.  In
particular, it will generate a backtrace for a Lisp error in (almost) any
hook called from redisplay.  The exception is
redisplay-end-trigger-functions, which is obsolete, being used
(hopefully) only by the obsolete lazy-lock.

To use it, set the variable backtrace-on-redisplay-error to t.
Optionally, configure redisplay-last-error, which determines which
backtrace(s) to keep, should a command cause more than one.

In detail....

On Wed, Jun 29, 2022 at 22:00:54 +0300, Eli Zaretskii wrote:
> > Date: Tue, 28 Jun 2022 19:31:59 +0000
> > Cc: larsi@gnus.org, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

[ .... ]

> > I'm currently using the buffer *Backtrace*.  Is there anything wrong
> > with that?

> Not that I see, no.

OK, *Backtrace* it is.

> > > The buffer with warnings pops up very soon after redisplay, which is
> > > more-or-less exactly what you wanted?

> > OK, I've never used it.  On reading the documentation, I expected it to
> > wait until after the next command.

> That's because the ELisp reference manual describes it from the POV of
> setting up the warning as part of some command.  And even then, it
> says:

>      The value of this variable is a list of warnings to be displayed
>      after the current command has finished.

> "Current command", not "next command".

> If you add a warning to the list inside redisplay, it will pop up
> after redisplay returns.

I've tried to use it, but the list of warnings does indeed wait until
after the next command before being displayed.  I think the command loop
calls the functions for it between post-command-hook and redisplay.
Maybe the command loop could call it after redisplay as well.

> > Now I remember why I created the backtrace in signal_or_quit - it needs
> > to be done before the stack gets unwound, which happens later in
> > signal_or_quit.  On return to save_eval_handler is just too late for
> > this.

> That's orthogonal, I think?  You can collect the data inside
> signal_or_quit, and the signal handler then only needs to handle the
> error gracefully after it adds the warning.

The "handling" of the error is to allow the redisplay to continue, just
as though no backtrace were generated.  Since there's no possibility of
user interaction or a recursive redisplay, that should be OK.

> > > It will, because any Lisp we call goes though safe_eval.  The only
> > > reason why your original proposal didn't was because it bound the
> > > variable which enables this only in handle_fontified_prop.  But we
> > > don't need this flag if the logic to decide what to do with the error
> > > will be in save_eval_handler.

> > Then the mechanism I've implemented, namely to set redisplay_deep_handler
> > to the handler which would handle an error, could be made to serve for
> > other sorts of Lisp code.

> It could, but I see no reason for having a new handler.

Sorry, I didn't express that well.  That variable redisplay_deep_handler
merely records the handler that a condition-case might use.  In
signal_or_quit, if the handler about to be used matches
redisplay_deep_handler, then a backtrace gets generated.

> > > And you need to consider one more factor: code from display engine,
> > > including fontification via handle_fontified_prop, is called also from
> > > user commands, not via redisplay_internal.  For example, commands like
> > > C-n and C-v invoke display code.  Errors signaled there _can_ be left
> > > to go to top-level and probably could call the debugger.  To test
> > > whether any given code was called from redisplay_internal, test the
> > > value of the variable redisplaying_p.

> > OK, this is a further complication.  But it would be good to get to the
> > debugger if possible, rather than just having a "historical" backtrace.

> If the display code is called from "normal" commands, you can.

I haven't attended to this possibility, yet, but I don't foresee it as
being particularly difficult or time-consuming.

I also still need to write documentation for the new code.

Anyhow, here's the current state of the patch, which still clearly isn't
finished:



diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index df919fd715..f5f52a2076 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -265,6 +265,10 @@ minibuffer-prompt-properties--setter
 	     (debug-ignored-errors debug (repeat (choice symbol regexp)))
 	     (debug-on-quit debug boolean)
 	     (debug-on-signal debug boolean)
+             (redisplay-last-error debug
+                                   (choice (const :tag "First backtrace only" nil)
+                                           (const :tag "Last backtrace only" t)
+                                           (const :tag "All backtraces" 'many)))
              (debugger-stack-frame-as-list debugger boolean "26.1")
 	     ;; fileio.c
 	     (delete-by-moving-to-trash auto-save boolean "23.1")
diff --git a/src/eval.c b/src/eval.c
index 141d2546f0..50982980cb 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -57,6 +57,12 @@ Lisp_Object Vrun_hooks;
 /* FIXME: We should probably get rid of this!  */
 Lisp_Object Vsignaling_function;
 
+/* The handler structure which will catch errors in Lisp hooks called
+   from redisplay.  We do not use it for this; we compare it with the
+   handler which is about to be used in signal_or_quit, and if it
+   matches, cause a backtrace to be generated.  */
+static struct handler *redisplay_deep_handler;
+
 /* These would ordinarily be static, but they need to be visible to GDB.  */
 bool backtrace_p (union specbinding *) EXTERNALLY_VISIBLE;
 Lisp_Object *backtrace_args (union specbinding *) EXTERNALLY_VISIBLE;
@@ -333,7 +339,8 @@ call_debugger (Lisp_Object arg)
   /* Interrupting redisplay and resuming it later is not safe under
      all circumstances.  So, when the debugger returns, abort the
      interrupted redisplay by going back to the top-level.  */
-  if (debug_while_redisplaying)
+  if (debug_while_redisplaying
+      && !EQ (Vdebugger, Qdebug_early))
     Ftop_level ();
 
   return unbind_to (count, val);
@@ -1550,14 +1557,18 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object err,
 						ptrdiff_t nargs,
-						Lisp_Object *args))
+						Lisp_Object *args),
+			   bool note_handler)
 {
   struct handler *c = push_handler (handlers, CONDITION_CASE);
+  if (note_handler)
+    redisplay_deep_handler = c;
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
       clobbered_eassert (handlerlist == c);
       handlerlist = handlerlist->next;
+      redisplay_deep_handler = NULL;
       return hfun (val, nargs, args);
     }
   else
@@ -1565,6 +1576,7 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
       Lisp_Object val = bfun (nargs, args);
       eassert (handlerlist == c);
       handlerlist = c->next;
+      redisplay_deep_handler = NULL;
       return val;
     }
 }
@@ -1697,6 +1709,11 @@ quit (void)
   return signal_or_quit (Qquit, Qnil, true);
 }
 
+/* Has an error in redisplay giving rise to a backtrace occurred as
+   yet in the current command?  This gets reset in the command
+   loop.  */
+bool backtrace_yet = false;
+
 /* Signal an error, or quit.  ERROR_SYMBOL and DATA are as with Fsignal.
    If KEYBOARD_QUIT, this is a quit; ERROR_SYMBOL should be
    Qquit and DATA should be Qnil, and this function may return.
@@ -1812,6 +1829,42 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool keyboard_quit)
       unbind_to (count, Qnil);
     }
 
+  /* If an error is signalled during a Lisp hook in redisplay, write a
+     backtrace into the buffer *Backtrace*.  */
+  if (!debugger_called && !NILP (error_symbol)
+      && redisplay_lisping
+      && backtrace_on_redisplay_lisp_error
+      && (!backtrace_yet || !NILP (Vredisplay_last_error))
+      && (NILP (clause) || h == redisplay_deep_handler)
+      && NILP (Vinhibit_debugger)
+      && !NILP (Ffboundp (Qdebug_early)))
+    {
+      max_ensure_room (&max_lisp_eval_depth, lisp_eval_depth, 100);
+      specpdl_ref count = SPECPDL_INDEX ();
+      ptrdiff_t counti = specpdl_ref_to_count (count);
+      AUTO_STRING (backtrace, "*Backtrace*");
+      Lisp_Object backtrace_buffer;
+      AUTO_STRING (gap, "\n\n\n\n"); /* Separates backtraces in *Backtrace* */
+      Lisp_Object delayed_warning;
+      max_ensure_room (&max_specpdl_size, counti, 200);
+      backtrace_buffer = Fget_buffer_create (backtrace, Qnil);
+      current_buffer = XBUFFER (backtrace_buffer);
+      if (!backtrace_yet
+	  || EQ (Vredisplay_last_error, Qt))
+	Ferase_buffer ();
+      else if (!EQ (Vredisplay_last_error, Qt))
+	Finsert (1, &gap);
+      backtrace_yet = true;
+      specbind (Qstandard_output, backtrace_buffer);
+      specbind (Qdebugger, Qdebug_early);
+      call_debugger (list2 (Qerror, Fcons (error_symbol, data)));
+      unbind_to (count, Qnil);
+      delayed_warning = make_string ("Error in a redisplay Lisp hook.  See buffer *Backtrace*", 55);
+
+      Vdelayed_warnings_list = Fcons (list2 (Qerror, delayed_warning),
+				      Vdelayed_warnings_list);
+    }
+
   if (!NILP (clause))
     {
       Lisp_Object unwind_data
@@ -4274,6 +4327,11 @@ Does not apply if quit is handled by a `condition-case'.  */);
   DEFVAR_BOOL ("debug-on-next-call", debug_on_next_call,
 	       doc: /* Non-nil means enter debugger before next `eval', `apply' or `funcall'.  */);
 
+  DEFVAR_BOOL ("backtrace-on-redisplay-lisp-error", backtrace_on_redisplay_lisp_error,
+	       doc: /* Non-nil means create a backtrace if a lisp error occurs in a redisplay hook.
+The backtrace is written to buffer *Backtrace*.  */);
+  backtrace_on_redisplay_lisp_error = false;
+
   DEFVAR_BOOL ("debugger-may-continue", debugger_may_continue,
 	       doc: /* Non-nil means debugger may continue execution.
 This is nil when the debugger is called under circumstances where it
@@ -4314,6 +4372,13 @@ message upon encountering an unhandled error, without showing
 the Lisp backtrace.  */);
   backtrace_on_error_noninteractive = true;
 
+  DEFVAR_LISP ("redisplay-last-error", Vredisplay_last_error,
+	       doc: /* Handling of *Backtrace* when several redisplay errors occur in one command.
+If nil (default), we write only the backtrace from the command's first error.
+If t, we save only the backtrace from the last error in the command.
+If any other value, we write backtraces for all errors in the command.  */);
+  Vredisplay_last_error = Qnil;
+
   /* The value of num_nonmacro_input_events as of the last time we
    started to enter the debugger.  If we decide to enter the debugger
    again when this is still equal to num_nonmacro_input_events, then we
diff --git a/src/keyboard.c b/src/keyboard.c
index c729d5dfb3..d95dc404c8 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1330,6 +1330,7 @@ command_loop_1 (void)
 	display_malloc_warning ();
 
       Vdeactivate_mark = Qnil;
+      backtrace_yet = false;
 
       /* Don't ignore mouse movements for more than a single command
 	 loop.  (This flag is set in xdisp.c whenever the tool bar is
@@ -1823,12 +1824,18 @@ adjust_point_for_property (ptrdiff_t last_pt, bool modified)
 }
 
 /* Subroutine for safe_run_hooks: run the hook, which is ARGS[1].  */
-
 static Lisp_Object
 safe_run_hooks_1 (ptrdiff_t nargs, Lisp_Object *args)
 {
-  eassert (nargs == 2);
-  return call0 (args[1]);
+  eassert (nargs >= 2 && nargs <= 4);
+  switch (nargs) {
+  case 2:
+    return call0 (args[1]);
+  case 3:
+    return call1 (args[1], args[2]);
+  default:
+    return call2 (args[1], args[2], args[3]);
+  }
 }
 
 /* Subroutine for safe_run_hooks: handle an error by clearing out the function
@@ -1837,7 +1844,7 @@ safe_run_hooks_1 (ptrdiff_t nargs, Lisp_Object *args)
 static Lisp_Object
 safe_run_hooks_error (Lisp_Object error, ptrdiff_t nargs, Lisp_Object *args)
 {
-  eassert (nargs == 2);
+  eassert (nargs >= 2 && nargs <= 4);
   AUTO_STRING (format, "Error in %s (%S): %S");
   Lisp_Object hook = args[0];
   Lisp_Object fun = args[1];
@@ -1877,7 +1884,7 @@ safe_run_hook_funcall (ptrdiff_t nargs, Lisp_Object *args)
   /* Yes, run_hook_with_args works with args in the other order.  */
   internal_condition_case_n (safe_run_hooks_1,
 			     2, ((Lisp_Object []) {args[1], args[0]}),
-			     Qt, safe_run_hooks_error);
+			     Qt, safe_run_hooks_error, false);
   return Qnil;
 }
 
@@ -1895,6 +1902,49 @@ safe_run_hooks (Lisp_Object hook)
   unbind_to (count, Qnil);
 }
 
+static Lisp_Object
+safe_run_hook_with_backtrace_funcall (ptrdiff_t nargs, Lisp_Object *args)
+{
+  Lisp_Object args_out [4];
+
+  eassert (nargs >= 2 && nargs <= 4);
+  args_out [0] = args[1];
+  args_out [1] = args[0];
+  if (nargs >= 3) args_out [2] = args [2];
+  if (nargs == 4) args_out [3] = args [3];
+  /* Yes, run_hook_with_args works with args in the other order.  */
+  internal_condition_case_n (safe_run_hooks_1,
+			     nargs, args_out,
+			     Qt, safe_run_hooks_error, true);
+  return Qnil;
+}
+
+/* Like safe_run_hooks, but in the event of a lisp error in a hook,
+   output a backtrace, should Emacs currently be configured for
+   this (backtrace-on-redisplay-lisp-error is non-nil).  */
+void
+safe_run_hooks_with_backtrace (Lisp_Object hook)
+{
+  specpdl_ref count = SPECPDL_INDEX ();
+
+  specbind (Qinhibit_quit, Qt);
+  run_hook_with_args (2, ((Lisp_Object []) {hook, hook}),
+		      safe_run_hook_with_backtrace_funcall);
+  unbind_to (count, Qnil);
+}
+
+void
+safe_run_hooks_2_with_backtrace (Lisp_Object hook, Lisp_Object arg1,
+				 Lisp_Object arg2)
+{
+  specpdl_ref count = SPECPDL_INDEX ();
+
+  specbind (Qinhibit_quit, Qt);
+  run_hook_with_args (4, ((Lisp_Object []) {hook, hook, arg1, arg2}),
+		      safe_run_hook_with_backtrace_funcall);
+  unbind_to (count, Qnil);
+}
+
 \f
 /* Nonzero means polling for input is temporarily suppressed.  */
 
diff --git a/src/lisp.h b/src/lisp.h
index dc496cc165..2227349d04 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4526,6 +4526,7 @@ extern Lisp_Object Vrun_hooks;
 extern Lisp_Object Vsignaling_function;
 extern Lisp_Object inhibit_lisp_code;
 extern bool signal_quit_p (Lisp_Object);
+extern bool backtrace_yet;
 
 /* To run a normal hook, use the appropriate function from the list below.
    The calling convention:
@@ -4562,7 +4563,7 @@ extern Lisp_Object internal_condition_case_1 (Lisp_Object (*) (Lisp_Object), Lis
 extern Lisp_Object internal_condition_case_2 (Lisp_Object (*) (Lisp_Object, Lisp_Object), Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object (*) (Lisp_Object));
 extern Lisp_Object internal_condition_case_n
     (Lisp_Object (*) (ptrdiff_t, Lisp_Object *), ptrdiff_t, Lisp_Object *,
-     Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *));
+     Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *), bool);
 extern Lisp_Object internal_catch_all (Lisp_Object (*) (void *), void *, Lisp_Object (*) (enum nonlocal_exit, Lisp_Object));
 extern struct handler *push_handler (Lisp_Object, enum handlertype)
   ATTRIBUTE_RETURNS_NONNULL;
@@ -4823,6 +4824,9 @@ extern bool detect_input_pending (void);
 extern bool detect_input_pending_ignore_squeezables (void);
 extern bool detect_input_pending_run_timers (bool);
 extern void safe_run_hooks (Lisp_Object);
+extern void safe_run_hooks_with_backtrace (Lisp_Object);
+extern void safe_run_hooks_2_with_backtrace (Lisp_Object, Lisp_Object,
+					     Lisp_Object);
 extern void cmd_error_internal (Lisp_Object, const char *);
 extern Lisp_Object command_loop_2 (Lisp_Object);
 extern Lisp_Object read_menu_command (void);
diff --git a/src/xdisp.c b/src/xdisp.c
index f205327cc3..5c4afa4a8d 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -3022,7 +3022,7 @@ safe__call (bool inhibit_quit, ptrdiff_t nargs, Lisp_Object func, va_list ap)
       /* Use Qt to ensure debugger does not run,
 	 so there is no possibility of wanting to redisplay.  */
       val = internal_condition_case_n (Ffuncall, nargs, args, Qt,
-				       safe_eval_handler);
+				       safe_eval_handler, true);
       val = SAFE_FREE_UNBIND_TO (count, val);
     }
 
@@ -4332,6 +4332,7 @@ handle_fontified_prop (struct it *it)
 
       val = Vfontification_functions;
       specbind (Qfontification_functions, Qnil);
+      specbind (Qredisplay_lisping, Qt);
 
       eassert (it->end_charpos == ZV);
 
@@ -12628,6 +12629,7 @@ set_message (Lisp_Object string)
     {
       specpdl_ref count = SPECPDL_INDEX ();
       specbind (Qinhibit_quit, Qt);
+      specbind (Qredisplay_lisping, Qt);
       message = safe_call1 (Vset_message_function, string);
       unbind_to (count, Qnil);
 
@@ -12705,7 +12707,8 @@ clear_message (bool current_p, bool last_displayed_p)
         {
           specpdl_ref count = SPECPDL_INDEX ();
           specbind (Qinhibit_quit, Qt);
-          preserve = safe_call (1, Vclear_message_function);
+	  specbind (Qredisplay_lisping, Qt);
+	  preserve = safe_call (1, Vclear_message_function);
           unbind_to (count, Qnil);
         }
 
@@ -13289,6 +13292,7 @@ prepare_menu_bars (void)
 {
   bool all_windows = windows_or_buffers_changed || update_mode_lines;
   bool some_windows = REDISPLAY_SOME_P ();
+  specpdl_ref count = SPECPDL_INDEX ();
 
   if (FUNCTIONP (Vpre_redisplay_function))
     {
@@ -13306,7 +13310,9 @@ prepare_menu_bars (void)
 		windows = Fcons (this, windows);
 	    }
 	}
+      specbind (Qredisplay_lisping, Qt);
       safe__call1 (true, Vpre_redisplay_function, windows);
+      unbind_to (count, Qnil);
     }
 
   /* Update all frame titles based on their buffer names, etc.  We do
@@ -13463,12 +13469,11 @@ update_menu_bar (struct frame *f, bool save_match_data, bool hooks_run)
 
 	  if (!hooks_run)
 	    {
-	      /* Run the Lucid hook.  */
-	      safe_run_hooks (Qactivate_menubar_hook);
+	      specbind (Qredisplay_lisping, Qt);
 
 	      /* If it has changed current-menubar from previous value,
 		 really recompute the menu-bar from the value.  */
-	      safe_run_hooks (Qmenu_bar_update_hook);
+	      safe_run_hooks_with_backtrace (Qmenu_bar_update_hook);
 
 	      hooks_run = true;
 	    }
@@ -17915,8 +17920,9 @@ run_window_scroll_functions (Lisp_Object window, struct text_pos startp)
     {
       specpdl_ref count = SPECPDL_INDEX ();
       specbind (Qinhibit_quit, Qt);
-      run_hook_with_args_2 (Qwindow_scroll_functions, window,
-			    make_fixnum (CHARPOS (startp)));
+      specbind (Qredisplay_lisping, Qt);
+      safe_run_hooks_2_with_backtrace
+	(Qwindow_scroll_functions, window, make_fixnum (CHARPOS (startp)));
       unbind_to (count, Qnil);
       SET_TEXT_POS_FROM_MARKER (startp, w->start);
       /* In case the hook functions switch buffers.  */
@@ -19419,6 +19425,7 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 	 now actually do it.  */
       if (new_vpos >= 0)
 	{
+	  specpdl_ref count = SPECPDL_INDEX ();
 	  struct glyph_row *row;
 
 	  row = MATRIX_FIRST_TEXT_ROW (w->desired_matrix);
@@ -19444,7 +19451,9 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 	     propagated its info to `w' anyway.  */
 	  w->redisplay = false;
 	  XBUFFER (w->contents)->text->redisplay = false;
+	  specbind (Qredisplay_lisping, Qt);
 	  safe__call1 (true, Vpre_redisplay_function, Fcons (window, Qnil));
+	  unbind_to (count, Qnil);
 
 	  if (w->redisplay || XBUFFER (w->contents)->text->redisplay
 	      || ((EQ (Vdisplay_line_numbers, Qrelative)
@@ -26544,7 +26553,7 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 						   Flength (elt),
 						   props,
 						   elt}),
-					       Qt, safe_eval_handler);
+					       Qt, safe_eval_handler, false);
 		    /* Add this item to mode_line_proptrans_alist.  */
 		    mode_line_proptrans_alist
 		      = Fcons (Fcons (elt, props),
@@ -35848,6 +35857,7 @@ be let-bound around code that needs to disable messages temporarily. */);
   DEFSYM (QCfile, ":file");
   DEFSYM (Qfontified, "fontified");
   DEFSYM (Qfontification_functions, "fontification-functions");
+  DEFSYM (Qredisplay_lisping, "redisplay-lisping");
 
   /* Name of the symbol which disables Lisp evaluation in 'display'
      properties.  This is used by enriched.el.  */
@@ -36365,6 +36375,10 @@ fontified regions the property `fontified'.  */);
   Vfontification_functions = Qnil;
   Fmake_variable_buffer_local (Qfontification_functions);
 
+  DEFVAR_BOOL ("redisplay-lisping", redisplay_lisping,
+    doc: /* Non-nil when a Lisp hook call from redisplay is in progress.  */);
+  redisplay_lisping = false;
+
   DEFVAR_BOOL ("unibyte-display-via-language-environment",
                unibyte_display_via_language_environment,
     doc: /* Non-nil means display unibyte text according to language environment.
diff --git a/src/xfns.c b/src/xfns.c
index 331f22763e..b026605053 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -3405,7 +3405,7 @@ x_xim_text_to_utf8_unix (XIMText *text, ptrdiff_t *length)
   waiting_for_input = false;
   arg = make_mint_ptr (&data);
   internal_condition_case_n (x_xim_text_to_utf8_unix_1, 1, &arg,
-			     Qt, x_xim_text_to_utf8_unix_2);
+			     Qt, x_xim_text_to_utf8_unix_2, false);
   waiting_for_input = was_waiting_for_input_p;
 
   *length = coding.produced;


-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error bactraces [Was: Fontification error backtrace]

[Eli Zaretskii]

> Date: Tue, 12 Jul 2022 19:48:49 +0000
> Cc: larsi@gnus.org, Stefan Monnier <monnier@iro.umontreal.ca>,
>   emacs-devel@gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> To use it, set the variable backtrace-on-redisplay-error to t.

There's no such variable in the patch.

> Optionally, configure redisplay-last-error, which determines which
> backtrace(s) to keep, should a command cause more than one.

Why do we need two variables for that? why not a single variable that
is not a boolean?

> > > OK, I've never used it.  On reading the documentation, I expected it to
> > > wait until after the next command.
> 
> > That's because the ELisp reference manual describes it from the POV of
> > setting up the warning as part of some command.  And even then, it
> > says:
> 
> >      The value of this variable is a list of warnings to be displayed
> >      after the current command has finished.
> 
> > "Current command", not "next command".
> 
> > If you add a warning to the list inside redisplay, it will pop up
> > after redisplay returns.
> 
> I've tried to use it, but the list of warnings does indeed wait until
> after the next command before being displayed.

I think it's a terminology issue: how do you define "next command" in
this context?

I'm telling you: we have code in the display engine that uses delayed
warnings, and the warnings pop up right away, not after one more
command.

> > > Now I remember why I created the backtrace in signal_or_quit - it needs
> > > to be done before the stack gets unwound, which happens later in
> > > signal_or_quit.  On return to save_eval_handler is just too late for
> > > this.
> 
> > That's orthogonal, I think?  You can collect the data inside
> > signal_or_quit, and the signal handler then only needs to handle the
> > error gracefully after it adds the warning.
> 
> The "handling" of the error is to allow the redisplay to continue, just
> as though no backtrace were generated.  Since there's no possibility of
> user interaction or a recursive redisplay, that should be OK.

I don't understand this response.  It seems to be unrelated to what I
wrote.

> > > Then the mechanism I've implemented, namely to set redisplay_deep_handler
> > > to the handler which would handle an error, could be made to serve for
> > > other sorts of Lisp code.
> 
> > It could, but I see no reason for having a new handler.
> 
> Sorry, I didn't express that well.  That variable redisplay_deep_handler
> merely records the handler that a condition-case might use.  In
> signal_or_quit, if the handler about to be used matches
> redisplay_deep_handler, then a backtrace gets generated.

??? You use a handler as a flag?  Then why not just use a simple flag
variable?

> +  /* If an error is signalled during a Lisp hook in redisplay, write a
> +     backtrace into the buffer *Backtrace*.  */
> +  if (!debugger_called && !NILP (error_symbol)
> +      && redisplay_lisping
> +      && backtrace_on_redisplay_lisp_error
> +      && (!backtrace_yet || !NILP (Vredisplay_last_error))
> +      && (NILP (clause) || h == redisplay_deep_handler)
> +      && NILP (Vinhibit_debugger)
> +      && !NILP (Ffboundp (Qdebug_early)))

Why do we need so many conditions and flags?  Why do we need a new
variable redisplay_lisping, when we already have redisplaying_p?  Why
do you need to compare both redisplay_deep_handler and
backtrace_on_redisplay_lisp_error?  And what is the test of
backtrace_yet about?

I still hope this could be done more elegantly and with fewer changes
to infrastructure.

Thanks.

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Eli.

On Wed, Jul 13, 2022 at 15:33:07 +0300, Eli Zaretskii wrote:
> > Date: Tue, 12 Jul 2022 19:48:49 +0000
> > Cc: larsi@gnus.org, Stefan Monnier <monnier@iro.umontreal.ca>,
> >   emacs-devel@gnu.org, acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

> > To use it, set the variable backtrace-on-redisplay-error to t.

> There's no such variable in the patch.

Apologies.  I meant backtrace-on-redisplay-lisp-error.

> > Optionally, configure redisplay-last-error, which determines which
> > backtrace(s) to keep, should a command cause more than one.

> Why do we need two variables for that? why not a single variable that
> is not a boolean?

I was anticipating setting backtrace-on-redisplay-lisp-error
"dynamically" in debug-next-command, the command I introduced right at
the start of this thread ~3 weeks ago, and which would set all possible
flags to maximise the chance of getting a backtrace on the next command.
Such flags really need to be just nil/t.  Additionally,
redisplay-last-error allows a bit of configuration.

But I suppose if this is too many variables, we could cut one of them
out.

[ .... ]

> > > If you add a warning to the list inside redisplay, it will pop up
> > > after redisplay returns.

> > I've tried to use it, but the list of warnings does indeed wait until
> > after the next command before being displayed.

> I think it's a terminology issue: how do you define "next command" in
> this context?

I set up jit-lock-functions to give an error when a C-Mode buffer is
scrolled.  I enable the new mechanism.  I scroll the buffer.  I then see
the expected error in the echo area.  I type a random key.  This now
opens a window with *Warnings* in it, the sole line there being the error
I've just generated.  This is on a Linux console.

> I'm telling you: we have code in the display engine that uses delayed
> warnings, and the warnings pop up right away, not after one more
> command.

There is something different between what I am doing and what the
existing code in the display engine does.

But this doesn't seem the most pressing of problems with my patch.  Not
yet.

> > > > Now I remember why I created the backtrace in signal_or_quit - it
> > > > needs to be done before the stack gets unwound, which happens
> > > > later in signal_or_quit.  On return to save_eval_handler is just
> > > > too late for this.

> > > That's orthogonal, I think?  You can collect the data inside
> > > signal_or_quit, and the signal handler then only needs to handle
> > > the error gracefully after it adds the warning.

> > The "handling" of the error is to allow the redisplay to continue,
> > just as though no backtrace were generated.  Since there's no
> > possibility of user interaction or a recursive redisplay, that should
> > be OK.

> I don't understand this response.  It seems to be unrelated to what I
> wrote.

The backtrace is generated directly from signal_or_quit, so there is no
further collection of data to be done here; it's written to the buffer
*Backtrace*.  There is then no further handling to be done[*], except to
issue a delayed warning.  Maybe this could be better done in
internal_condition_case_n, thus taking some code lines out of
signal_or_quit.

[*] There's still the condition-case handling to be done, which would
have been needed regardless of the backtrace.

> > > > Then the mechanism I've implemented, namely to set
> > > > redisplay_deep_handler to the handler which would handle an
> > > > error, could be made to serve for other sorts of Lisp code.

> > > It could, but I see no reason for having a new handler.

> > Sorry, I didn't express that well.  That variable redisplay_deep_handler
> > merely records the handler that a condition-case might use.  In
> > signal_or_quit, if the handler about to be used matches
> > redisplay_deep_handler, then a backtrace gets generated.

> ??? You use a handler as a flag?  Then why not just use a simple flag
> variable?

The problem is that other signals go through the same C code in
signal_and_quit, for example "smaller" condition-cases.  We do not want
to generate backtraces for these routine condition-cases which simply get
handled by their Lisp code.  redisplay_deep_handler records the "deepest"
pending condition-case, and only if the variable `h' matches that deepest
condition case do we have an error that we want to output a backtrace for.

> > +  /* If an error is signalled during a Lisp hook in redisplay, write a
> > +     backtrace into the buffer *Backtrace*.  */
> > +  if (!debugger_called && !NILP (error_symbol)
> > +      && redisplay_lisping
> > +      && backtrace_on_redisplay_lisp_error
> > +      && (!backtrace_yet || !NILP (Vredisplay_last_error))
> > +      && (NILP (clause) || h == redisplay_deep_handler)
> > +      && NILP (Vinhibit_debugger)
> > +      && !NILP (Ffboundp (Qdebug_early)))

> Why do we need so many conditions and flags?  Why do we need a new
> variable redisplay_lisping, when we already have redisplaying_p?

Thanks for asking that.  redisplay_lisping is actually redundant, since
the state it records (being in a running Lisp hook) is trivially
reflected in other variables.  So I've removed it from the code.

> Why do you need to compare both redisplay_deep_handler and
> backtrace_on_redisplay_lisp_error?

r_d_h checks whether we've actually got a pertinent Lisp error (see
above).  b_o_r_l_e is a toggle set by the user to enable the feature.

> And what is the test of backtrace_yet about?

When a backtrace is being generated, it first erases *Backtrace*.  We
only want to do that for the first backtrace generated by the current
command.  So backtrace_yet records whether we've already generated a BT
in this command.  It is reset to false in the command loop.

This ensures that the user sees that first backtrace, which is
likely to be the most interesting one.  Unless she has configured
backtrace-last-error to do something different.

As an alternative to this complicated configuration, perhaps we could
just erase *Backtrace* for the first BT, then write any further BTs to
*Backtrace*, too.

> I still hope this could be done more elegantly and with fewer changes
> to infrastructure.

You mean, all the changes in eval.c and keyboard.c?  I think the changes
to internal_condition_case_n are essential to the patch, and I honestly
don't think it can be done much more elegantly, but I'm open to
suggestions.

Thanks for reviewing the patch (again) so thoroughly.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Wed, 13 Jul 2022 18:41:01 +0000
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> The problem is that other signals go through the same C code in
> signal_and_quit, for example "smaller" condition-cases.  We do not want
> to generate backtraces for these routine condition-cases which simply get
> handled by their Lisp code.  redisplay_deep_handler records the "deepest"
> pending condition-case, and only if the variable `h' matches that deepest
> condition case do we have an error that we want to output a backtrace for.

I still don't think I understand why testing redisplaying_p and the
new optional variable would not be enough.

> > And what is the test of backtrace_yet about?
> 
> When a backtrace is being generated, it first erases *Backtrace*.  We
> only want to do that for the first backtrace generated by the current
> command.  So backtrace_yet records whether we've already generated a BT
> in this command.  It is reset to false in the command loop.
> 
> This ensures that the user sees that first backtrace, which is
> likely to be the most interesting one.  Unless she has configured
> backtrace-last-error to do something different.
> 
> As an alternative to this complicated configuration, perhaps we could
> just erase *Backtrace* for the first BT, then write any further BTs to
> *Backtrace*, too.

Either that, or erase it every time.  I think anything more complex is
over-engineered.

> > I still hope this could be done more elegantly and with fewer changes
> > to infrastructure.
> 
> You mean, all the changes in eval.c and keyboard.c?  I think the changes
> to internal_condition_case_n are essential to the patch, and I honestly
> don't think it can be done much more elegantly, but I'm open to
> suggestions.

Can we discuss how to implement it without introducing a special
handler and without adding new safe_run_hooks_* functions?

Re: Redisplay hook error bactraces [Was: Fontification error backtrace]

[Stefan Monnier]

> +  /* If an error is signalled during a Lisp hook in redisplay, write a
> +     backtrace into the buffer *Backtrace*.  */
> +  if (!debugger_called && !NILP (error_symbol)
> +      && redisplay_lisping
> +      && backtrace_on_redisplay_lisp_error
> +      && (!backtrace_yet || !NILP (Vredisplay_last_error))
> +      && (NILP (clause) || h == redisplay_deep_handler)
> +      && NILP (Vinhibit_debugger)
> +      && !NILP (Ffboundp (Qdebug_early)))
> +    {
> +      max_ensure_room (&max_lisp_eval_depth, lisp_eval_depth, 100);
> +      specpdl_ref count = SPECPDL_INDEX ();
> +      ptrdiff_t counti = specpdl_ref_to_count (count);
> +      AUTO_STRING (backtrace, "*Backtrace*");
> +      Lisp_Object backtrace_buffer;
> +      AUTO_STRING (gap, "\n\n\n\n"); /* Separates backtraces in *Backtrace* */

I really think that printing the backtrace right when the error is
signaled is a bad idea.  Better just copy the whole specpdl (or just the
backtrace part of it) and leave the buffer manipulation and printing for
later when we're back in a normal&sane context.

It would be worthwhile doing all this work in the context of the error
if there was an upside such as the ability to do the debugging "live",
but since this is for the case where we'll be doing a "post-mortem
debugging" anyway, we're much better off doing as little as possible in
the context of the error: too many things can go wrong when running in
the context of the error, it's asking for trouble.


        Stefan

Re: Redisplay hook error bactraces [Was: Fontification error backtrace]

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  larsi@gnus.org,  emacs-devel@gnu.org
> Date: Wed, 13 Jul 2022 15:13:41 -0400
> 
> I really think that printing the backtrace right when the error is
> signaled is a bad idea.  Better just copy the whole specpdl (or just the
> backtrace part of it) and leave the buffer manipulation and printing for
> later when we're back in a normal&sane context.

I agree.

Re: Redisplay hook error bactraces [Was: Fontification error backtrace]

[Alan Mackenzie]

Hello, Stefan.

On Wed, Jul 13, 2022 at 15:13:41 -0400, Stefan Monnier wrote:
> > +  /* If an error is signalled during a Lisp hook in redisplay, write a
> > +     backtrace into the buffer *Backtrace*.  */
> > +  if (!debugger_called && !NILP (error_symbol)
> > +      && redisplay_lisping
> > +      && backtrace_on_redisplay_lisp_error
> > +      && (!backtrace_yet || !NILP (Vredisplay_last_error))
> > +      && (NILP (clause) || h == redisplay_deep_handler)
> > +      && NILP (Vinhibit_debugger)
> > +      && !NILP (Ffboundp (Qdebug_early)))
> > +    {
> > +      max_ensure_room (&max_lisp_eval_depth, lisp_eval_depth, 100);
> > +      specpdl_ref count = SPECPDL_INDEX ();
> > +      ptrdiff_t counti = specpdl_ref_to_count (count);
> > +      AUTO_STRING (backtrace, "*Backtrace*");
> > +      Lisp_Object backtrace_buffer;
> > +      AUTO_STRING (gap, "\n\n\n\n"); /* Separates backtraces in *Backtrace* */

> I really think that printing the backtrace right when the error is
> signaled is a bad idea.  Better just copy the whole specpdl (or just the
> backtrace part of it) and leave the buffer manipulation and printing for
> later when we're back in a normal&sane context.

What you suggest would imply a _lot_ of coding to make it happen.  Does
there exist anywhere any support for copyging a specpdl, and somehow
making a lisp object out of it?  Why do you think that copying the
specpdl would be less work or less risky than directly generating a
backtrace, as the current code does?

The advantage of directly generating the backtrace is that all the
infrastructure is already there.  We've got mapbacktrace which
encapsulates the formats of the specpdl.  And debug-early.el, whose two
functions total 60 lines (including doc strings and comments) and don't
use any other Lisp whatsoever.

> It would be worthwhile doing all this work in the context of the error
> if there was an upside such as the ability to do the debugging "live",
> but since this is for the case where we'll be doing a "post-mortem
> debugging" anyway, we're much better off doing as little as possible in
> the context of the error: too many things can go wrong when running in
> the context of the error, it's asking for trouble.

Again, why is copying the pdl less risky than generating the backtrace?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Eli.

On Wed, Jul 13, 2022 at 22:00:14 +0300, Eli Zaretskii wrote:
> > Date: Wed, 13 Jul 2022 18:41:01 +0000
> > Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > The problem is that other signals go through the same C code in
> > signal_and_quit, for example "smaller" condition-cases.  We do not want
> > to generate backtraces for these routine condition-cases which simply get
> > handled by their Lisp code.  redisplay_deep_handler records the "deepest"
> > pending condition-case, and only if the variable `h' matches that deepest
> > condition case do we have an error that we want to output a backtrace for.

> I still don't think I understand why testing redisplaying_p and the
> new optional variable would not be enough.

We've got to distinguish between the signals we want to generate
backtraces for and those we don't.  redisplaying_p is not relevant to
that, I think.  For example, we don't want to generate a backtrace for a
"failed" evaluation of the code generated by `c-safe'.

> > > And what is the test of backtrace_yet about?

> > When a backtrace is being generated, it first erases *Backtrace*.  We
> > only want to do that for the first backtrace generated by the current
> > command.  So backtrace_yet records whether we've already generated a BT
> > in this command.  It is reset to false in the command loop.

> > This ensures that the user sees that first backtrace, which is
> > likely to be the most interesting one.  Unless she has configured
> > backtrace-last-error to do something different.

> > As an alternative to this complicated configuration, perhaps we could
> > just erase *Backtrace* for the first BT, then write any further BTs to
> > *Backtrace*, too.

> Either that, or erase it every time.  I think anything more complex is
> over-engineered.

OK, I'll get rid of the config variable, and just dump every backtrace to
*Backtrace*.  That will get rid of quite a few lines of code.

> > > I still hope this could be done more elegantly and with fewer changes
> > > to infrastructure.

> > You mean, all the changes in eval.c and keyboard.c?  I think the changes
> > to internal_condition_case_n are essential to the patch, and I honestly
> > don't think it can be done much more elegantly, but I'm open to
> > suggestions.

> Can we discuss how to implement it without introducing a special
> handler and without adding new safe_run_hooks_* functions?

OK.  Perhaps with extra optional arguments, that kind of thing?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error bactraces [Was: Fontification error backtrace]

[Stefan Monnier]

> What you suggest would imply a _lot_ of coding to make it happen.  Does
> there exist anywhere any support for copyging a specpdl, and somehow
> making a lisp object out of it?  Why do you think that copying the
> specpdl would be less work or less risky than directly generating a
> backtrace, as the current code does?

I don't think it needs to be much coding.

Stashing the full specpdl would be more work, admittedly, but turning
the specpdl into a list via something like `backtrace-frames` is trivial
(we could even easily move `backtrace-frames` to C if we wanted to be
paranoid about running ELisp at that point).

> The advantage of directly generating the backtrace is that all the
> infrastructure is already there.  We've got mapbacktrace which
> encapsulates the formats of the specpdl.  And debug-early.el, whose two
> functions total 60 lines (including doc strings and comments) and don't
> use any other Lisp whatsoever.

That potentially runs a crapload of code via various hooks, many of
which can be influenced by variables dynamically bound in the context of
the error.

Moving the bulk of the work to a normal context means we can be much
more relaxed about using "fully featured" code, so there'll be much less
resistance to over-engineering :-)


        Stefan

Re: Redisplay hook error backtraces

[Stefan Monnier]

>> I still don't think I understand why testing redisplaying_p and the
>> new optional variable would not be enough.
>
> We've got to distinguish between the signals we want to generate
> backtraces for and those we don't.  redisplaying_p is not relevant to
> that, I think.  For example, we don't want to generate a backtrace for a
> "failed" evaluation of the code generated by `c-safe'.

BTW, `c-safe` is called `ignore-errors` in standard ELisp since
Emacs-23 (before that it was provided by `cl`).


        Stefan

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Wed, 13 Jul 2022 20:12:42 +0000
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > I still don't think I understand why testing redisplaying_p and the
> > new optional variable would not be enough.
> 
> We've got to distinguish between the signals we want to generate
> backtraces for and those we don't.  redisplaying_p is not relevant to
> that, I think.  For example, we don't want to generate a backtrace for a
> "failed" evaluation of the code generated by `c-safe'.

You want to distinguish errors inside condition-case?  If they are not
already marked in some way that allows you to do so, I think it's
better to do it the other way around: bind some variable in
internal_lisp_condition_case before invoking the body.

> > > You mean, all the changes in eval.c and keyboard.c?  I think the changes
> > > to internal_condition_case_n are essential to the patch, and I honestly
> > > don't think it can be done much more elegantly, but I'm open to
> > > suggestions.
> 
> > Can we discuss how to implement it without introducing a special
> > handler and without adding new safe_run_hooks_* functions?
> 
> OK.  Perhaps with extra optional arguments, that kind of thing?

Maybe, I don't know.  I still think the job of signal_or_quit and
safe_run_hooks is almost the same when you want to collect backtrace,
so too many differences strike me as unexpected.

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Eli.

On Thu, Jul 14, 2022 at 08:12:51 +0300, Eli Zaretskii wrote:
> > Date: Wed, 13 Jul 2022 20:12:42 +0000
> > Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > I still don't think I understand why testing redisplaying_p and the
> > > new optional variable would not be enough.

> > We've got to distinguish between the signals we want to generate
> > backtraces for and those we don't.  redisplaying_p is not relevant to
> > that, I think.  For example, we don't want to generate a backtrace
> > for a "failed" evaluation of the code generated by `c-safe'.

> You want to distinguish errors inside condition-case?

More, distinguish the different condition-cases in which errors might
occur.

> If they are not already marked in some way that allows you to do so, I
> think it's better to do it the other way around: bind some variable in
> internal_lisp_condition_case before invoking the body.

OK, I think I see what you're getting at.  But this would mean binding
that variable at _every_ condition_case, and needing some way of deciding
which way to bind it (an extra argument in every use of
internal_lisp_condition_case, perhaps?).  Why would this be better than
the way I have already implemented?

> > > > You mean, all the changes in eval.c and keyboard.c?  I think the
> > > > changes to internal_condition_case_n are essential to the patch,
> > > > and I honestly don't think it can be done much more elegantly,
> > > > but I'm open to suggestions.

> > > Can we discuss how to implement it without introducing a special
> > > handler and without adding new safe_run_hooks_* functions?

I think we need the new function safe_run_hooks_2_with_backtrace (see
below), since there is currently no "safe" function for hooks with two
arguments.  But some of the other ones could disappear (see below).

> > OK.  Perhaps with extra optional arguments, that kind of thing?

> Maybe, I don't know.  I still think the job of signal_or_quit and
> safe_run_hooks is almost the same when you want to collect backtrace,
> so too many differences strike me as unexpected.

OK, I have an idea.  I restore the variable redisplay_lisping back into
the code (I took it out last night), binding it to true (or Qt?) at every
place in xdisp.c where redisplay calls a Lisp hook.  I then test that
variable in internal_condition_case_n in place of having the extra bool
argument to that function.

That would then get rid of the new functions
safe_run_hooks_with_backtrace_funcall and safe_run_hooks_with_backtrace.
We could also rename safe_run_hooks_2_with_backtrace by removing
"_with_backtrace" from the name.

What do you think?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Thu, 14 Jul 2022 09:01:16 +0000
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > You want to distinguish errors inside condition-case?
> 
> More, distinguish the different condition-cases in which errors might
> occur.

What else is exposed to Lisp?  We are talking about catching Lisp
errors only, right?

> > > > Can we discuss how to implement it without introducing a special
> > > > handler and without adding new safe_run_hooks_* functions?
> 
> I think we need the new function safe_run_hooks_2_with_backtrace (see
> below), since there is currently no "safe" function for hooks with two
> arguments.  But some of the other ones could disappear (see below).

What is the second argument, and why do we need it?

> OK, I have an idea.  I restore the variable redisplay_lisping back into
> the code (I took it out last night), binding it to true (or Qt?) at every
> place in xdisp.c where redisplay calls a Lisp hook.

These all go through a single function, so there's just one place to
do that.

> I then test that variable in internal_condition_case_n in place of
> having the extra bool argument to that function.
> 
> That would then get rid of the new functions
> safe_run_hooks_with_backtrace_funcall and safe_run_hooks_with_backtrace.
> We could also rename safe_run_hooks_2_with_backtrace by removing
> "_with_backtrace" from the name.
> 
> What do you think?

Sounds good, but let's see the code.

And I still would like you to explore Stefan's suggestion, since doing
too much non-trivial Lisp stuff in signal_or_quit is better avoided.

Thanks.

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Eli.

On Thu, Jul 14, 2022 at 12:10:13 +0300, Eli Zaretskii wrote:
> > Date: Thu, 14 Jul 2022 09:01:16 +0000
> > Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > You want to distinguish errors inside condition-case?

> > More, distinguish the different condition-cases in which errors might
> > occur.

> What else is exposed to Lisp?

I don't understand this question.

> We are talking about catching Lisp errors only, right?

Yes.

> > > > > Can we discuss how to implement it without introducing a
> > > > > special handler and without adding new safe_run_hooks_*
> > > > > functions?

> > I think we need the new function safe_run_hooks_2_with_backtrace (see
> > below), since there is currently no "safe" function for hooks with
> > two arguments.  But some of the other ones could disappear (see
> > below).

> What is the second argument, and why do we need it?

There are three arguments, the non-standard hook, and the two arguments
which will be fed to that hook.

Currently, that hook (window-scroll-functions) is called with
run_hook_with_args_2, which doesn't have a "safe" in its functionality,
which we need.  My change is to use safe_run_hooks_2 (a new function)
instead.

> > OK, I have an idea.  I restore the variable redisplay_lisping back
> > into the code (I took it out last night), binding it to true (or Qt?)
> > at every place in xdisp.c where redisplay calls a Lisp hook.

> These all go through a single function, so there's just one place to
> do that.

I disagree.  There are seven places, for the seven different Lisp hooks
currently called from redisplay.

> > I then test that variable in internal_condition_case_n in place of
> > having the extra bool argument to that function.

Done.

> > That would then get rid of the new functions
> > safe_run_hooks_with_backtrace_funcall and safe_run_hooks_with_backtrace.
> > We could also rename safe_run_hooks_2_with_backtrace by removing
> > "_with_backtrace" from the name.

Also done.

> > What do you think?

> Sounds good, but let's see the code.

See below.

> And I still would like you to explore Stefan's suggestion, since doing
> too much non-trivial Lisp stuff in signal_or_quit is better avoided.

I still don't understand this.  The Lisp in debug-early.el is about as
trivial as you can get whilst still doing anything at all.  It doesn't
use any other Lisp libraries, only the C primitives.  (At least, that was
the case, and could and should be restored.)

But can we talk about this in the other branch of this thread which
already goes into some detail about this, please?

Here's the current version of the patch:



diff --git a/src/dispextern.h b/src/dispextern.h
index ca7834dec5..af490f574e 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3410,6 +3410,7 @@ int partial_line_height (struct it *it_origin);
 bool in_display_vector_p (struct it *);
 int frame_mode_line_height (struct frame *);
 extern bool redisplaying_p;
+extern bool redisplay_lisping;
 extern bool display_working_on_window_p;
 extern void unwind_display_working_on_window (void);
 extern bool help_echo_showing_p;
diff --git a/src/eval.c b/src/eval.c
index 141d2546f0..edadf64434 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -57,6 +57,12 @@ Lisp_Object Vrun_hooks;
 /* FIXME: We should probably get rid of this!  */
 Lisp_Object Vsignaling_function;
 
+/* The handler structure which will catch errors in Lisp hooks called
+   from redisplay.  We do not use it for this; we compare it with the
+   handler which is about to be used in signal_or_quit, and if it
+   matches, cause a backtrace to be generated.  */
+static struct handler *redisplay_deep_handler;
+
 /* These would ordinarily be static, but they need to be visible to GDB.  */
 bool backtrace_p (union specbinding *) EXTERNALLY_VISIBLE;
 Lisp_Object *backtrace_args (union specbinding *) EXTERNALLY_VISIBLE;
@@ -333,7 +339,8 @@ call_debugger (Lisp_Object arg)
   /* Interrupting redisplay and resuming it later is not safe under
      all circumstances.  So, when the debugger returns, abort the
      interrupted redisplay by going back to the top-level.  */
-  if (debug_while_redisplaying)
+  if (debug_while_redisplaying
+      && !EQ (Vdebugger, Qdebug_early))
     Ftop_level ();
 
   return unbind_to (count, val);
@@ -1553,11 +1560,14 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 						Lisp_Object *args))
 {
   struct handler *c = push_handler (handlers, CONDITION_CASE);
+  if (redisplay_lisping)
+    redisplay_deep_handler = c;
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
       clobbered_eassert (handlerlist == c);
       handlerlist = handlerlist->next;
+      redisplay_deep_handler = NULL;
       return hfun (val, nargs, args);
     }
   else
@@ -1565,6 +1575,7 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
       Lisp_Object val = bfun (nargs, args);
       eassert (handlerlist == c);
       handlerlist = c->next;
+      redisplay_deep_handler = NULL;
       return val;
     }
 }
@@ -1697,6 +1708,11 @@ quit (void)
   return signal_or_quit (Qquit, Qnil, true);
 }
 
+/* Has an error in redisplay giving rise to a backtrace occurred as
+   yet in the current command?  This gets reset in the command
+   loop.  */
+bool backtrace_yet = false;
+
 /* Signal an error, or quit.  ERROR_SYMBOL and DATA are as with Fsignal.
    If KEYBOARD_QUIT, this is a quit; ERROR_SYMBOL should be
    Qquit and DATA should be Qnil, and this function may return.
@@ -1812,6 +1828,39 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool keyboard_quit)
       unbind_to (count, Qnil);
     }
 
+  /* If an error is signalled during a Lisp hook in redisplay, write a
+     backtrace into the buffer *Backtrace*.  */
+  if (!debugger_called && !NILP (error_symbol)
+      && backtrace_on_redisplay_lisp_error
+      && (NILP (clause) || h == redisplay_deep_handler)
+      && NILP (Vinhibit_debugger)
+      && !NILP (Ffboundp (Qdebug_early)))
+    {
+      max_ensure_room (&max_lisp_eval_depth, lisp_eval_depth, 100);
+      specpdl_ref count = SPECPDL_INDEX ();
+      ptrdiff_t counti = specpdl_ref_to_count (count);
+      AUTO_STRING (backtrace, "*Backtrace*");
+      Lisp_Object backtrace_buffer;
+      AUTO_STRING (gap, "\n\n\n\n"); /* Separates backtraces in *Backtrace* */
+      Lisp_Object delayed_warning;
+      max_ensure_room (&max_specpdl_size, counti, 200);
+      backtrace_buffer = Fget_buffer_create (backtrace, Qnil);
+      current_buffer = XBUFFER (backtrace_buffer);
+      if (!backtrace_yet) /* Are we on the first backtrace of the command?  */
+	Ferase_buffer ();
+      else
+	Finsert (1, &gap);
+      backtrace_yet = true;
+      specbind (Qstandard_output, backtrace_buffer);
+      specbind (Qdebugger, Qdebug_early);
+      call_debugger (list2 (Qerror, Fcons (error_symbol, data)));
+      unbind_to (count, Qnil);
+      delayed_warning = make_string ("Error in a redisplay Lisp hook.  See buffer *Backtrace*", 55);
+
+      Vdelayed_warnings_list = Fcons (list2 (Qerror, delayed_warning),
+				      Vdelayed_warnings_list);
+    }
+
   if (!NILP (clause))
     {
       Lisp_Object unwind_data
@@ -4274,6 +4323,11 @@ Does not apply if quit is handled by a `condition-case'.  */);
   DEFVAR_BOOL ("debug-on-next-call", debug_on_next_call,
 	       doc: /* Non-nil means enter debugger before next `eval', `apply' or `funcall'.  */);
 
+  DEFVAR_BOOL ("backtrace-on-redisplay-lisp-error", backtrace_on_redisplay_lisp_error,
+	       doc: /* Non-nil means create a backtrace if a lisp error occurs in a redisplay hook.
+The backtrace is written to buffer *Backtrace*.  */);
+  backtrace_on_redisplay_lisp_error = false;
+
   DEFVAR_BOOL ("debugger-may-continue", debugger_may_continue,
 	       doc: /* Non-nil means debugger may continue execution.
 This is nil when the debugger is called under circumstances where it
diff --git a/src/keyboard.c b/src/keyboard.c
index c729d5dfb3..e886f571ce 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1330,6 +1330,7 @@ command_loop_1 (void)
 	display_malloc_warning ();
 
       Vdeactivate_mark = Qnil;
+      backtrace_yet = false;
 
       /* Don't ignore mouse movements for more than a single command
 	 loop.  (This flag is set in xdisp.c whenever the tool bar is
@@ -1823,12 +1824,18 @@ adjust_point_for_property (ptrdiff_t last_pt, bool modified)
 }
 
 /* Subroutine for safe_run_hooks: run the hook, which is ARGS[1].  */
-
 static Lisp_Object
 safe_run_hooks_1 (ptrdiff_t nargs, Lisp_Object *args)
 {
-  eassert (nargs == 2);
-  return call0 (args[1]);
+  eassert (nargs >= 2 && nargs <= 4);
+  switch (nargs) {
+  case 2:
+    return call0 (args[1]);
+  case 3:
+    return call1 (args[1], args[2]);
+  default:
+    return call2 (args[1], args[2], args[3]);
+  }
 }
 
 /* Subroutine for safe_run_hooks: handle an error by clearing out the function
@@ -1837,7 +1844,7 @@ safe_run_hooks_1 (ptrdiff_t nargs, Lisp_Object *args)
 static Lisp_Object
 safe_run_hooks_error (Lisp_Object error, ptrdiff_t nargs, Lisp_Object *args)
 {
-  eassert (nargs == 2);
+  eassert (nargs >= 2 && nargs <= 4);
   AUTO_STRING (format, "Error in %s (%S): %S");
   Lisp_Object hook = args[0];
   Lisp_Object fun = args[1];
@@ -1895,6 +1902,17 @@ safe_run_hooks (Lisp_Object hook)
   unbind_to (count, Qnil);
 }
 
+void
+safe_run_hooks_2 (Lisp_Object hook, Lisp_Object arg1, Lisp_Object arg2)
+{
+  specpdl_ref count = SPECPDL_INDEX ();
+
+  specbind (Qinhibit_quit, Qt);
+  run_hook_with_args (4, ((Lisp_Object []) {hook, hook, arg1, arg2}),
+		      safe_run_hook_funcall);
+  unbind_to (count, Qnil);
+}
+
 \f
 /* Nonzero means polling for input is temporarily suppressed.  */
 
diff --git a/src/lisp.h b/src/lisp.h
index dc496cc165..7caf5e2745 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4526,6 +4526,7 @@ extern Lisp_Object Vrun_hooks;
 extern Lisp_Object Vsignaling_function;
 extern Lisp_Object inhibit_lisp_code;
 extern bool signal_quit_p (Lisp_Object);
+extern bool backtrace_yet;
 
 /* To run a normal hook, use the appropriate function from the list below.
    The calling convention:
@@ -4823,6 +4824,7 @@ extern bool detect_input_pending (void);
 extern bool detect_input_pending_ignore_squeezables (void);
 extern bool detect_input_pending_run_timers (bool);
 extern void safe_run_hooks (Lisp_Object);
+extern void safe_run_hooks_2 (Lisp_Object, Lisp_Object, Lisp_Object);
 extern void cmd_error_internal (Lisp_Object, const char *);
 extern Lisp_Object command_loop_2 (Lisp_Object);
 extern Lisp_Object read_menu_command (void);
diff --git a/src/xdisp.c b/src/xdisp.c
index f205327cc3..092b1f2be2 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -1030,6 +1030,11 @@ static struct glyph_slice null_glyph_slice = { 0, 0, 0, 0 };
 
 bool redisplaying_p;
 
+/* True if a call to a Lisp hook from redisplay is currently in
+   progress.  */
+
+bool redisplay_lisping = false;
+
 /* True while some display-engine code is working on layout of some
    window.
 
@@ -3137,6 +3142,12 @@ CHECK_WINDOW_END (struct window *w)
 #endif
 }
 
+static void
+unwind_redisplay_lisping (int arg)
+{
+  redisplay_lisping = arg;
+}
+
 /***********************************************************************
 		       Iterator initialization
  ***********************************************************************/
@@ -4332,6 +4343,8 @@ handle_fontified_prop (struct it *it)
 
       val = Vfontification_functions;
       specbind (Qfontification_functions, Qnil);
+      record_unwind_protect_int (unwind_redisplay_lisping, redisplay_lisping);
+      redisplay_lisping = true;
 
       eassert (it->end_charpos == ZV);
 
@@ -12628,6 +12641,8 @@ set_message (Lisp_Object string)
     {
       specpdl_ref count = SPECPDL_INDEX ();
       specbind (Qinhibit_quit, Qt);
+      record_unwind_protect_int (unwind_redisplay_lisping, redisplay_lisping);
+      redisplay_lisping = true;
       message = safe_call1 (Vset_message_function, string);
       unbind_to (count, Qnil);
 
@@ -12705,6 +12720,8 @@ clear_message (bool current_p, bool last_displayed_p)
         {
           specpdl_ref count = SPECPDL_INDEX ();
           specbind (Qinhibit_quit, Qt);
+	  record_unwind_protect_int (unwind_redisplay_lisping, redisplay_lisping);
+	  redisplay_lisping = true;
           preserve = safe_call (1, Vclear_message_function);
           unbind_to (count, Qnil);
         }
@@ -13289,6 +13306,7 @@ prepare_menu_bars (void)
 {
   bool all_windows = windows_or_buffers_changed || update_mode_lines;
   bool some_windows = REDISPLAY_SOME_P ();
+  specpdl_ref count = SPECPDL_INDEX ();
 
   if (FUNCTIONP (Vpre_redisplay_function))
     {
@@ -13306,7 +13324,10 @@ prepare_menu_bars (void)
 		windows = Fcons (this, windows);
 	    }
 	}
+      record_unwind_protect_int (unwind_redisplay_lisping, redisplay_lisping);
+      redisplay_lisping = true;
       safe__call1 (true, Vpre_redisplay_function, windows);
+      unbind_to (count, Qnil);
     }
 
   /* Update all frame titles based on their buffer names, etc.  We do
@@ -13463,8 +13484,8 @@ update_menu_bar (struct frame *f, bool save_match_data, bool hooks_run)
 
 	  if (!hooks_run)
 	    {
-	      /* Run the Lucid hook.  */
-	      safe_run_hooks (Qactivate_menubar_hook);
+	      record_unwind_protect_int (unwind_redisplay_lisping, redisplay_lisping);
+	      redisplay_lisping = true;
 
 	      /* If it has changed current-menubar from previous value,
 		 really recompute the menu-bar from the value.  */
@@ -17915,8 +17936,10 @@ run_window_scroll_functions (Lisp_Object window, struct text_pos startp)
     {
       specpdl_ref count = SPECPDL_INDEX ();
       specbind (Qinhibit_quit, Qt);
-      run_hook_with_args_2 (Qwindow_scroll_functions, window,
-			    make_fixnum (CHARPOS (startp)));
+      record_unwind_protect_int (unwind_redisplay_lisping, redisplay_lisping);
+      redisplay_lisping = true;
+      safe_run_hooks_2
+	(Qwindow_scroll_functions, window, make_fixnum (CHARPOS (startp)));
       unbind_to (count, Qnil);
       SET_TEXT_POS_FROM_MARKER (startp, w->start);
       /* In case the hook functions switch buffers.  */
@@ -19419,6 +19442,7 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 	 now actually do it.  */
       if (new_vpos >= 0)
 	{
+	  specpdl_ref count = SPECPDL_INDEX ();
 	  struct glyph_row *row;
 
 	  row = MATRIX_FIRST_TEXT_ROW (w->desired_matrix);
@@ -19444,7 +19468,10 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 	     propagated its info to `w' anyway.  */
 	  w->redisplay = false;
 	  XBUFFER (w->contents)->text->redisplay = false;
+	  record_unwind_protect_int (unwind_redisplay_lisping, redisplay_lisping);
+	  redisplay_lisping = true;
 	  safe__call1 (true, Vpre_redisplay_function, Fcons (window, Qnil));
+	  unbind_to (count, Qnil);
 
 	  if (w->redisplay || XBUFFER (w->contents)->text->redisplay
 	      || ((EQ (Vdisplay_line_numbers, Qrelative)
@@ -36882,6 +36909,7 @@ init_xdisp (void)
   }
 
   help_echo_showing_p = false;
+  redisplay_lisping = false;
 }
 
 #ifdef HAVE_WINDOW_SYSTEM




> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Thu, 14 Jul 2022 13:42:18 +0000
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > > You want to distinguish errors inside condition-case?
> 
> > > More, distinguish the different condition-cases in which errors might
> > > occur.
> 
> > What else is exposed to Lisp?
> 
> I don't understand this question.

There's just one condition-case available to Lisp code, AFAICT, so
why isn't it enough to distinguish condition-case from any other
callers of internal_condition_case* ?

> > > I think we need the new function safe_run_hooks_2_with_backtrace (see
> > > below), since there is currently no "safe" function for hooks with
> > > two arguments.  But some of the other ones could disappear (see
> > > below).
> 
> > What is the second argument, and why do we need it?
> 
> There are three arguments, the non-standard hook, and the two arguments
> which will be fed to that hook.
> 
> Currently, that hook (window-scroll-functions) is called with
> run_hook_with_args_2, which doesn't have a "safe" in its functionality,
> which we need.  My change is to use safe_run_hooks_2 (a new function)
> instead.

OK, sounds good.

> > > OK, I have an idea.  I restore the variable redisplay_lisping back
> > > into the code (I took it out last night), binding it to true (or Qt?)
> > > at every place in xdisp.c where redisplay calls a Lisp hook.
> 
> > These all go through a single function, so there's just one place to
> > do that.
> 
> I disagree.  There are seven places, for the seven different Lisp hooks
> currently called from redisplay.

Aren't they all go through safe_call?  Which seven places are you
talking about?

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Eli.

On Thu, Jul 14, 2022 at 16:59:57 +0300, Eli Zaretskii wrote:
> > Date: Thu, 14 Jul 2022 13:42:18 +0000
> > Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > > > You want to distinguish errors inside condition-case?

> > > > More, distinguish the different condition-cases in which errors
> > > > might occur.

> > > What else is exposed to Lisp?

> > I don't understand this question.

> There's just one condition-case available to Lisp code, AFAICT, so why
> isn't it enough to distinguish condition-case from any other callers of
> internal_condition_case* ?

I think I understand what you're saying, now.  That a condition-case
called from Lisp will not use any of the internal_condition_case*
functions.  So we could just assume that if i_c_case* triggers, it must be
one of the hooks we're interested in that called it.

I don't think that's right.  It might well be that Lisp code calls a C
primitive function that itself uses an internal_condition_case.  In this
scenario, we would not want to generate a backtrace for that nested
internal_condition_case.

[ .... ]

> > > > OK, I have an idea.  I restore the variable redisplay_lisping
> > > > back into the code (I took it out last night), binding it to true
> > > > (or Qt?) at every place in xdisp.c where redisplay calls a Lisp
> > > > hook.

> > > These all go through a single function, so there's just one place to
> > > do that.

> > I disagree.  There are seven places, for the seven different Lisp hooks
> > currently called from redisplay.

> Aren't they all go through safe_call?

They do, yes, but so do other things that we don't want to engage the
backtrace mechanism for.

> Which seven places are you talking about?

1. handle_fontified_prop;
2. set_message;
3. clear_message;
4. prepare_menu_bars (near the top);
5. update_menu_bar (line ~54);
6. run_window_scroll_functions;
7. redisplay_window (line ~415).

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Stefan Monnier]

>> Aren't they all go through safe_call?
>
> They do, yes, but so do other things that we don't want to engage the
> backtrace mechanism for.

Actually, I'm not sure that's the case.  Basically the question is
whether `safe_call` is used because we don't care about any errors that
might be thrown (akin to ignore-errors`) or whether we do care but don't
want to let them escape and ruin our lunch (akin to
`with-demoted-errors`).

Being in redisplay is usually a good reason why we don't want to let
errors escape, even we do care about those errors (and hence we need
to use a mechanism like the one you're implementing in order to debug
those problems), but there can be other such cases that also use
`safe_call` and where we would also want to use your new code.


        Stefan

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Thu, 14 Jul 2022 16:07:33 +0000
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > There's just one condition-case available to Lisp code, AFAICT, so why
> > isn't it enough to distinguish condition-case from any other callers of
> > internal_condition_case* ?
> 
> I think I understand what you're saying, now.  That a condition-case
> called from Lisp will not use any of the internal_condition_case*
> functions.  So we could just assume that if i_c_case* triggers, it must be
> one of the hooks we're interested in that called it.
> 
> I don't think that's right.  It might well be that Lisp code calls a C
> primitive function that itself uses an internal_condition_case.  In this
> scenario, we would not want to generate a backtrace for that nested
> internal_condition_case.

And we won't, because redisplaying_p won't be set.

> > > I disagree.  There are seven places, for the seven different Lisp hooks
> > > currently called from redisplay.
> 
> > Aren't they all go through safe_call?
> 
> They do, yes, but so do other things that we don't want to engage the
> backtrace mechanism for.
> 
> > Which seven places are you talking about?
> 
> 1. handle_fontified_prop;
> 2. set_message;
> 3. clear_message;
> 4. prepare_menu_bars (near the top);
> 5. update_menu_bar (line ~54);
> 6. run_window_scroll_functions;
> 7. redisplay_window (line ~415).

These either go through safe__call, or through run_hook_with_args.  I
think the latter group should be converted to call safe__call and its
friends.  And then you will have what I described: all those calls go
through a single gate.

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Eli.

On Thu, Jul 14, 2022 at 20:09:02 +0300, Eli Zaretskii wrote:
> > Date: Thu, 14 Jul 2022 16:07:33 +0000
> > Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > There's just one condition-case available to Lisp code, AFAICT, so why
> > > isn't it enough to distinguish condition-case from any other callers of
> > > internal_condition_case* ?

> > I think I understand what you're saying, now.  That a condition-case
> > called from Lisp will not use any of the internal_condition_case*
> > functions.  So we could just assume that if i_c_case* triggers, it must be
> > one of the hooks we're interested in that called it.

> > I don't think that's right.  It might well be that Lisp code calls a C
> > primitive function that itself uses an internal_condition_case.  In this
> > scenario, we would not want to generate a backtrace for that nested
> > internal_condition_case.

> And we won't, because redisplaying_p won't be set.

I don't understand.  If redisplay is called normally, it will go through
redisplay_internal which sets redisplaying_p to true.  And it will stay
true till the end of redisplay_internal, unless one of two special things
happen: (i) We enter the debugger; (ii) there's a recursive edit.

I don't see how the new backtrace facility can get any useful information
out of redisplaying_p; if there were a nested internal_condition_case*,
redisplaying_p would still be true, surely?

What am I missing?

> > > > I disagree.  There are seven places, for the seven different Lisp hooks
> > > > currently called from redisplay.

> > > Aren't they all go through safe_call?

> > They do, yes, but so do other things that we don't want to engage the
> > backtrace mechanism for.

> > > Which seven places are you talking about?

> > 1. handle_fontified_prop;
> > 2. set_message;
> > 3. clear_message;
> > 4. prepare_menu_bars (near the top);
> > 5. update_menu_bar (line ~54);
> > 6. run_window_scroll_functions;
> > 7. redisplay_window (line ~415).

> These either go through safe__call, or through run_hook_with_args.  I
> think the latter group should be converted to call safe__call and its
> friends.  And then you will have what I described: all those calls go
> through a single gate.

Maybe tomorrow!

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Stefan.

On Thu, Jul 14, 2022 at 12:18:51 -0400, Stefan Monnier wrote:
> >> Aren't they all go through safe_call?

> > They do, yes, but so do other things that we don't want to engage the
> > backtrace mechanism for.

> Actually, I'm not sure that's the case.  Basically the question is
> whether `safe_call` is used because we don't care about any errors that
> might be thrown (akin to ignore-errors`) or whether we do care but don't
> want to let them escape and ruin our lunch (akin to
> `with-demoted-errors`).

> Being in redisplay is usually a good reason why we don't want to let
> errors escape, even we do care about those errors (and hence we need
> to use a mechanism like the one you're implementing in order to debug
> those problems), but there can be other such cases that also use
> `safe_call` and where we would also want to use your new code.

There are approximately 38 calls to something like safe_call in the C
sources, about half of which are in xdisp.c.  That's a lot of scope for
irritation if non-events continually swamp the real errors in redisplay's
Lisp hooks.

It feels like we would want some mechanism to filter out "some" safe_call
calls, and things are getting complicated fast.  The scope of the
exercise has already expanded considerably, from just
fontification-functions to all redisplay hooks.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Stefan Monnier]

> It feels like we would want some mechanism to filter out "some"
> safe_call calls, and things are getting complicated fast.  The scope
> of the exercise has already expanded considerably, from just
> fontification-functions to all redisplay hooks.

Remember what I'm pushing for: a trace of recent silenced backtraces.


        Stefan

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Thu, 14 Jul 2022 19:47:25 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, larsi@gnus.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > Being in redisplay is usually a good reason why we don't want to let
> > errors escape, even we do care about those errors (and hence we need
> > to use a mechanism like the one you're implementing in order to debug
> > those problems), but there can be other such cases that also use
> > `safe_call` and where we would also want to use your new code.
> 
> There are approximately 38 calls to something like safe_call in the C
> sources, about half of which are in xdisp.c.  That's a lot of scope for
> irritation if non-events continually swamp the real errors in redisplay's
> Lisp hooks.
> 
> It feels like we would want some mechanism to filter out "some" safe_call
> calls, and things are getting complicated fast.  The scope of the
> exercise has already expanded considerably, from just
> fontification-functions to all redisplay hooks.

Alan, you are again over-engineering things.  Assuming that safe__call
is the only interesting gateway for Lisp errors during redisplay
should be good enough, if you in addition test redisplaying_p.

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, everybody.

I know that there are technical issues in the code still to be thrashed
out, but the new backtrace facility needs documentation.  I've written
some and a proposed patch (for debugging.texi and NEWS) is below.

I've come to see that `backtrace-on-redisplay-lisp-error', although
accurate, is too much of a mouthful, and I think it needs shortening.
Would anybody object to me just calling this variable `debug-redisplay'?

Also, I think that writing the backtrace into *Backtrace* is a bad idea,
now.  Users expect such a buffer to have active facilities, whereas the
new backtrace is just a dead dump.  Is there any objection to me instead
calling the buffer something like "*Redisplay-trace*"?



diff --git a/doc/lispref/debugging.texi b/doc/lispref/debugging.texi
index 058c931954..3f9c9fea50 100644
--- a/doc/lispref/debugging.texi
+++ b/doc/lispref/debugging.texi
@@ -77,6 +77,7 @@ Debugger
 
 @menu
 * Error Debugging::       Entering the debugger when an error happens.
+* Debugging Redisplay::   Getting backtraces from redisplay hook errors.
 * Infinite Loops::        Stopping and debugging a program that doesn't exit.
 * Function Debugging::    Entering it when a certain function is called.
 * Variable Debugging::    Entering it when a variable is modified.
@@ -105,6 +106,10 @@ Error Debugging
 (The command @code{toggle-debug-on-error} provides an easy way to do
 this.)
 
+Note that, for technical reasons, you cannot use the facilities
+defined in this subsection to debug errors in hooks the redisplay code
+has invoked.  @xref{Debugging Redisplay} for help with these.
+
 @defopt debug-on-error
 This variable determines whether the debugger is called when an error
 is signaled and not handled.  If @code{debug-on-error} is @code{t},
@@ -213,6 +218,52 @@ Error Debugging
 bypasses the @code{condition-case} which normally catches errors in the
 init file.
 
+@node Debugging Redisplay
+@subsection Debugging Redisplay Hooks
+@cindex redisplay hooks
+@cindex debugging redisplay hooks
+
+When a Lisp error occurs in a hook function which redisplay has
+invoked, you cannot use Emacs's usual debugging mechanisms, for
+technical reasons.  This subsection describes how to get a backtrace
+from such an error, information which should be helpful in debugging
+it.
+
+Note that if you have had such an error, the error handling will
+likely have removed the function causing it from its hook.  You will
+thus need to reinitialize that hook somehow, perhaps with
+@code{add-hook}, to be able to replay the bug.
+
+The hooks which these directions apply to are the following:
+@enumerate
+@item
+@code{fontification-functions} (@pxref{Auto Faces}).
+@item
+@code{set-message-function} (@pxref{Displaying Messages}).
+@item
+@code{clear-message-function} (@pxref{Displaying Messages}).
+@item
+@code{pre-redisplay-function} (@pxref{Forcing Redisplay}).
+@item
+@code{pre-redisplay-functions} (@pxref{Forcing Redisplay}).
+@item
+@code{menu-bar-update-hook} (@pxref{Menu Bar}).
+@item
+@code{window-scroll-functions} (@pxref{Window Hooks}).
+@end enumerate
+
+To generate a backtrace in these circumstances, set the variable
+@code{backtrace-on-redisplay-lisp-error} to non-@code{nil}.  When the
+error occurs, Emacs will dump the backtrace to the buffer
+@file{*Backtrace*}.  This buffer is not automatically displayed in a
+window; you will need to display it yourself, with a command such as
+@code{switch-to-buffer-other-frame} (@key{C-x 5 b}).
+
+@defvar backtrace-on-redisplay-lisp-error
+Set this variable to non-@code{nil} to enable the generation of a
+backtrace when errors occur in any of the above hooks.
+@end defvar
+
 @node Infinite Loops
 @subsection Debugging Infinite Loops
 @cindex infinite loops
diff --git a/etc/NEWS b/etc/NEWS
index c2900b0bc4..0196d55306 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1227,6 +1227,13 @@ When invoked with a non-zero prefix argument, as in 'C-u C-x C-e',
 this command will pop up a new buffer and show the full pretty-printed
 value there.
 
++++
+*** It is now possible to generate a backtrace from errors in redisplay
+hooks.  To do this, set the new variable
+'backtrace-on-redisplay-lisp-error' to a non-nil value.  The backtrace
+will be written to buffer *Backtrace*, but this buffer will not be
+automatically displayed in a window.
+
 ** Compile
 
 +++


-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Fri, 15 Jul 2022 18:18:14 +0000
> Cc: monnier@iro.umontreal.ca, larsi@gnus.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> I've come to see that `backtrace-on-redisplay-lisp-error', although
> accurate, is too much of a mouthful, and I think it needs shortening.
> Would anybody object to me just calling this variable `debug-redisplay'?

Too general, IMO.  How about backtrace-on-redisplay-errors?

> Also, I think that writing the backtrace into *Backtrace* is a bad idea,
> now.  Users expect such a buffer to have active facilities, whereas the
> new backtrace is just a dead dump.  Is there any objection to me instead
> calling the buffer something like "*Redisplay-trace*"?

No objections, but I think you are again over-engineering things.

> +Note that, for technical reasons, you cannot use the facilities
> +defined in this subsection to debug errors in hooks the redisplay code
> +has invoked.  @xref{Debugging Redisplay} for help with these.
                                           ^
A comma missing there.

> +@node Debugging Redisplay
> +@subsection Debugging Redisplay Hooks
> +@cindex redisplay hooks

This @cindex should be "redisplay hooks, debugging", because it
doesn't talk about the hooks in general.

> +When a Lisp error occurs in a hook function which redisplay has
> +invoked, you cannot use Emacs's usual debugging mechanisms, for
> +technical reasons.  This subsection describes how to get a backtrace
> +from such an error, information which should be helpful in debugging
> +it.

This talks about hooks, which restricts the scope of the facility: we
want to be able to collect backtraces from any Lisp called by the
display code.  For example, :eval forms in the mode line, Lisp code
called by fontification-functions, etc.  So the text ebove needs to be
revised with this in mind.

> +The hooks which these directions apply to are the following:

I don't want us to have a list that looks like it's an exhaustive one.
maintaining such a list is a maintenance burden we are better without.
Please give a couple of examples and change the text to the effect
that these are just examples.

> +*** It is now possible to generate a backtrace from errors in redisplay
> +hooks.  To do this, set the new variable

The heading line should be a full sentence.

Thanks.

Re: Redisplay hook error backtraces

[Eli Zaretskii]

> Date: Thu, 14 Jul 2022 19:33:09 +0000
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > I think I understand what you're saying, now.  That a condition-case
> > > called from Lisp will not use any of the internal_condition_case*
> > > functions.  So we could just assume that if i_c_case* triggers, it must be
> > > one of the hooks we're interested in that called it.
> 
> > > I don't think that's right.  It might well be that Lisp code calls a C
> > > primitive function that itself uses an internal_condition_case.  In this
> > > scenario, we would not want to generate a backtrace for that nested
> > > internal_condition_case.
> 
> > And we won't, because redisplaying_p won't be set.
> 
> I don't understand.  If redisplay is called normally, it will go through
> redisplay_internal which sets redisplaying_p to true.  And it will stay
> true till the end of redisplay_internal, unless one of two special things
> happen: (i) We enter the debugger; (ii) there's a recursive edit.
> 
> I don't see how the new backtrace facility can get any useful information
> out of redisplaying_p; if there were a nested internal_condition_case*,
> redisplaying_p would still be true, surely?
> 
> What am I missing?

My point is that redisplaying_p tells you the code which runs was
invoked from redisplay, and that allows you to distinguish calls to
internal_condition_case* that are of interest (i.e. should produce a
backtrace in the *Backtrace* buffer) from those which aren't of
interest.  Which was the issue you raised against my suggestion to
rely on the fact that Lisp only ever uses a single form of
internal_condition_case* functions.

To recap: this sub-thread started when you said you don't want to
cause all calls to internal_condition_case* to generate backtrace in
this new way.  To which I suggested to bind a variable in
internal_condition_case that would prevent generation of backtrace,
relying on redisplaying_p to tell us when the other
internal_condition_case* functions are called outside of redisplay.

Re: Redisplay hook error backtraces

[Alan Mackenzie]

Hello, Eli.

On Sat, Jul 16, 2022 at 09:12:48 +0300, Eli Zaretskii wrote:
> > Date: Thu, 14 Jul 2022 19:33:09 +0000
> > Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > > I think I understand what you're saying, now.  That a condition-case
> > > > called from Lisp will not use any of the internal_condition_case*
> > > > functions.  So we could just assume that if i_c_case* triggers, it must be
> > > > one of the hooks we're interested in that called it.

> > > > I don't think that's right.  It might well be that Lisp code calls a C
> > > > primitive function that itself uses an internal_condition_case.  In this
> > > > scenario, we would not want to generate a backtrace for that nested
> > > > internal_condition_case.

> > > And we won't, because redisplaying_p won't be set.

> > I don't understand.  If redisplay is called normally, it will go through
> > redisplay_internal which sets redisplaying_p to true.  And it will stay
> > true till the end of redisplay_internal, unless one of two special things
> > happen: (i) We enter the debugger; (ii) there's a recursive edit.

> > I don't see how the new backtrace facility can get any useful information
> > out of redisplaying_p; if there were a nested internal_condition_case*,
> > redisplaying_p would still be true, surely?

> > What am I missing?

> My point is that redisplaying_p tells you the code which runs was
> invoked from redisplay, and that allows you to distinguish calls to
> internal_condition_case* that are of interest (i.e. should produce a
> backtrace in the *Backtrace* buffer) from those which aren't of
> interest.  Which was the issue you raised against my suggestion to
> rely on the fact that Lisp only ever uses a single form of
> internal_condition_case* functions.

> To recap: this sub-thread started when you said you don't want to
> cause all calls to internal_condition_case* to generate backtrace in
> this new way.  To which I suggested to bind a variable in
> internal_condition_case that would prevent generation of backtrace,
> relying on redisplaying_p to tell us when the other
> internal_condition_case* functions are called outside of redisplay.

OK.  I'm a little concerned that we'll be getting too many backtraces.
But my "solution" to that wouldn't have done any good anyhow.

I've got rid of redisplay_lisping, renamed backtrace_.... to
backtrace-on-redisplay-error (without the s), and the patch, somewhat
smaller than before, is below.

I'll amend the documentation later.



diff --git a/src/eval.c b/src/eval.c
index 141d2546f0..2940f33512 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -57,6 +57,12 @@ Lisp_Object Vrun_hooks;
 /* FIXME: We should probably get rid of this!  */
 Lisp_Object Vsignaling_function;
 
+/* The handler structure which will catch errors in Lisp hooks called
+   from redisplay.  We do not use it for this; we compare it with the
+   handler which is about to be used in signal_or_quit, and if it
+   matches, cause a backtrace to be generated.  */
+static struct handler *redisplay_deep_handler;
+
 /* These would ordinarily be static, but they need to be visible to GDB.  */
 bool backtrace_p (union specbinding *) EXTERNALLY_VISIBLE;
 Lisp_Object *backtrace_args (union specbinding *) EXTERNALLY_VISIBLE;
@@ -246,6 +252,7 @@ init_eval (void)
   lisp_eval_depth = 0;
   /* This is less than the initial value of num_nonmacro_input_events.  */
   when_entered_debugger = -1;
+  redisplay_deep_handler = NULL;
 }
 
 /* Ensure that *M is at least A + B if possible, or is its maximum
@@ -333,7 +340,8 @@ call_debugger (Lisp_Object arg)
   /* Interrupting redisplay and resuming it later is not safe under
      all circumstances.  So, when the debugger returns, abort the
      interrupted redisplay by going back to the top-level.  */
-  if (debug_while_redisplaying)
+  if (debug_while_redisplaying
+      && !EQ (Vdebugger, Qdebug_early))
     Ftop_level ();
 
   return unbind_to (count, val);
@@ -1552,12 +1560,16 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 						ptrdiff_t nargs,
 						Lisp_Object *args))
 {
+  struct handler *old_deep = redisplay_deep_handler;
   struct handler *c = push_handler (handlers, CONDITION_CASE);
+  if (redisplaying_p)
+    redisplay_deep_handler = c;
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
       clobbered_eassert (handlerlist == c);
       handlerlist = handlerlist->next;
+      redisplay_deep_handler = old_deep;
       return hfun (val, nargs, args);
     }
   else
@@ -1565,6 +1577,7 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
       Lisp_Object val = bfun (nargs, args);
       eassert (handlerlist == c);
       handlerlist = c->next;
+      redisplay_deep_handler = old_deep;
       return val;
     }
 }
@@ -1697,6 +1710,11 @@ quit (void)
   return signal_or_quit (Qquit, Qnil, true);
 }
 
+/* Has an error in redisplay giving rise to a backtrace occurred as
+   yet in the current command?  This gets reset in the command
+   loop.  */
+bool backtrace_yet = false;
+
 /* Signal an error, or quit.  ERROR_SYMBOL and DATA are as with Fsignal.
    If KEYBOARD_QUIT, this is a quit; ERROR_SYMBOL should be
    Qquit and DATA should be Qnil, and this function may return.
@@ -1812,6 +1830,40 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool keyboard_quit)
       unbind_to (count, Qnil);
     }
 
+  /* If an error is signalled during a Lisp hook in redisplay, write a
+     backtrace into the buffer *Redisplay-trace*.  */
+  if (!debugger_called && !NILP (error_symbol)
+      && backtrace_on_redisplay_error
+      && (NILP (clause) || h == redisplay_deep_handler)
+      && NILP (Vinhibit_debugger)
+      && !NILP (Ffboundp (Qdebug_early)))
+    {
+      max_ensure_room (&max_lisp_eval_depth, lisp_eval_depth, 100);
+      specpdl_ref count = SPECPDL_INDEX ();
+      ptrdiff_t counti = specpdl_ref_to_count (count);
+      AUTO_STRING (redisplay_trace, "*Redisplay_trace*");
+      Lisp_Object redisplay_trace_buffer;
+      AUTO_STRING (gap, "\n\n\n\n"); /* Separates things in *Redisplay-trace* */
+      Lisp_Object delayed_warning;
+      max_ensure_room (&max_specpdl_size, counti, 200);
+      redisplay_trace_buffer = Fget_buffer_create (redisplay_trace, Qnil);
+      current_buffer = XBUFFER (redisplay_trace_buffer);
+      if (!backtrace_yet) /* Are we on the first backtrace of the command?  */
+	Ferase_buffer ();
+      else
+	Finsert (1, &gap);
+      backtrace_yet = true;
+      specbind (Qstandard_output, redisplay_trace_buffer);
+      specbind (Qdebugger, Qdebug_early);
+      call_debugger (list2 (Qerror, Fcons (error_symbol, data)));
+      unbind_to (count, Qnil);
+      delayed_warning = make_string
+	("Error in a redisplay Lisp hook.  See buffer *Redisplay_trace*", 61);
+
+      Vdelayed_warnings_list = Fcons (list2 (Qerror, delayed_warning),
+				      Vdelayed_warnings_list);
+    }
+
   if (!NILP (clause))
     {
       Lisp_Object unwind_data
@@ -4274,6 +4326,11 @@ Does not apply if quit is handled by a `condition-case'.  */);
   DEFVAR_BOOL ("debug-on-next-call", debug_on_next_call,
 	       doc: /* Non-nil means enter debugger before next `eval', `apply' or `funcall'.  */);
 
+  DEFVAR_BOOL ("backtrace-on-redisplay-error", backtrace_on_redisplay_error,
+	       doc: /* Non-nil means create a backtrace if a lisp error occurs in redisplay.
+The backtrace is written to buffer *Redisplay-trace*.  */);
+  backtrace_on_redisplay_error = false;
+
   DEFVAR_BOOL ("debugger-may-continue", debugger_may_continue,
 	       doc: /* Non-nil means debugger may continue execution.
 This is nil when the debugger is called under circumstances where it
diff --git a/src/keyboard.c b/src/keyboard.c
index 2863058d63..6b2757f5b1 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1330,6 +1330,7 @@ command_loop_1 (void)
 	display_malloc_warning ();
 
       Vdeactivate_mark = Qnil;
+      backtrace_yet = false;
 
       /* Don't ignore mouse movements for more than a single command
 	 loop.  (This flag is set in xdisp.c whenever the tool bar is
@@ -1837,7 +1838,7 @@ safe_run_hooks_1 (ptrdiff_t nargs, Lisp_Object *args)
 static Lisp_Object
 safe_run_hooks_error (Lisp_Object error, ptrdiff_t nargs, Lisp_Object *args)
 {
-  eassert (nargs == 2);
+  eassert (nargs >= 2 && nargs <= 4);
   AUTO_STRING (format, "Error in %s (%S): %S");
   Lisp_Object hook = args[0];
   Lisp_Object fun = args[1];
@@ -1895,6 +1896,17 @@ safe_run_hooks (Lisp_Object hook)
   unbind_to (count, Qnil);
 }
 
+void
+safe_run_hooks_2 (Lisp_Object hook, Lisp_Object arg1, Lisp_Object arg2)
+{
+  specpdl_ref count = SPECPDL_INDEX ();
+
+  specbind (Qinhibit_quit, Qt);
+  run_hook_with_args (4, ((Lisp_Object []) {hook, hook, arg1, arg2}),
+		      safe_run_hook_funcall);
+  unbind_to (count, Qnil);
+}
+
 \f
 /* Nonzero means polling for input is temporarily suppressed.  */
 
diff --git a/src/lisp.h b/src/lisp.h
index dc496cc165..7caf5e2745 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4526,6 +4526,7 @@ extern Lisp_Object Vrun_hooks;
 extern Lisp_Object Vsignaling_function;
 extern Lisp_Object inhibit_lisp_code;
 extern bool signal_quit_p (Lisp_Object);
+extern bool backtrace_yet;
 
 /* To run a normal hook, use the appropriate function from the list below.
    The calling convention:
@@ -4823,6 +4824,7 @@ extern bool detect_input_pending (void);
 extern bool detect_input_pending_ignore_squeezables (void);
 extern bool detect_input_pending_run_timers (bool);
 extern void safe_run_hooks (Lisp_Object);
+extern void safe_run_hooks_2 (Lisp_Object, Lisp_Object, Lisp_Object);
 extern void cmd_error_internal (Lisp_Object, const char *);
 extern Lisp_Object command_loop_2 (Lisp_Object);
 extern Lisp_Object read_menu_command (void);
diff --git a/src/xdisp.c b/src/xdisp.c
index 1940d16a01..086875410c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -17915,8 +17915,8 @@ run_window_scroll_functions (Lisp_Object window, struct text_pos startp)
     {
       specpdl_ref count = SPECPDL_INDEX ();
       specbind (Qinhibit_quit, Qt);
-      run_hook_with_args_2 (Qwindow_scroll_functions, window,
-			    make_fixnum (CHARPOS (startp)));
+      safe_run_hooks_2
+	(Qwindow_scroll_functions, window, make_fixnum (CHARPOS (startp)));
       unbind_to (count, Qnil);
       SET_TEXT_POS_FROM_MARKER (startp, w->start);
       /* In case the hook functions switch buffers.  */


-- 
Alan Mackenzie (Nuremberg, Germany).