From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.devel Subject: Re: Redisplay hook error backtraces Date: Wed, 13 Jul 2022 18:41:01 +0000 Message-ID: References: <83o7yg9f0w.fsf@gnu.org> <83r1396lvr.fsf@gnu.org> <83edz87ivz.fsf@gnu.org> <83r1375m6x.fsf@gnu.org> <837d4hw5to.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="16772"; mail-complaints-to="usenet@ciao.gmane.io" Cc: larsi@gnus.org, monnier@iro.umontreal.ca, emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Jul 13 20:42:34 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oBhJi-0004BG-CV for ged-emacs-devel@m.gmane-mx.org; Wed, 13 Jul 2022 20:42:34 +0200 Original-Received: from localhost ([::1]:42800 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oBhJg-0003PW-MY for ged-emacs-devel@m.gmane-mx.org; Wed, 13 Jul 2022 14:42:32 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:43306) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oBhIM-0002Jz-ON for emacs-devel@gnu.org; Wed, 13 Jul 2022 14:41:10 -0400 Original-Received: from colin.muc.de ([193.149.48.1]:31560 helo=mail.muc.de) by eggs.gnu.org with smtp (Exim 4.90_1) (envelope-from ) id 1oBhIK-00086x-NM for emacs-devel@gnu.org; Wed, 13 Jul 2022 14:41:10 -0400 Original-Received: (qmail 45978 invoked by uid 3782); 13 Jul 2022 18:41:07 -0000 Original-Received: from acm.muc.de (p4fe15b73.dip0.t-ipconnect.de [79.225.91.115]) (using STARTTLS) by colin.muc.de (tmda-ofmipd) with ESMTP; Wed, 13 Jul 2022 20:41:07 +0200 Original-Received: (qmail 10181 invoked by uid 1000); 13 Jul 2022 18:41:01 -0000 Content-Disposition: inline In-Reply-To: <837d4hw5to.fsf@gnu.org> X-Submission-Agent: TMDA/1.3.x (Ph3nix) X-Primary-Address: acm@muc.de Received-SPF: pass client-ip=193.149.48.1; envelope-from=acm@muc.de; helo=mail.muc.de X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:292109 Archived-At: 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 , > > emacs-devel@gnu.org, acm@muc.de > > From: Alan Mackenzie > > 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).