Re: Redisplay hook error backtraces

[Alan Mackenzie <acm@muc.de>]

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).