Re: [PATCH v4 3/4] [gdb/cli] Allow source highlighting to be interrupted (continued)

[Pedro Alves <pedro@palves.net>]

On 2023-10-18 18:11, Tom de Vries wrote:
> In PR cli/30934, a problem is reported where gdb becomes unresponsive for
> 2m13s because the GNU source-highlight library is taking a lot of time to
> annotate the source:
> ...
> $ gdb -q a.out -ex "b 56"
> Reading symbols from a.out...
> Breakpoint 1 at 0x400e2a: file test.cpp, line 56.
> (gdb) run
> Starting program: /data/vries/gdb/a.out
> 
> Breakpoint 1, Solution::numOfSubarrays () at test.cpp:56
> <... wait for it ...>
> 56	        return (int) t;
> (gdb)
> ...
> 
> This is due to a problem in the library [1], for which I've posted a
> patch [2], which brings down the annotation time to 3s.
> 
> However, GDB should not become unresponsive due to such a problem.
> 
> The previous patch allows us to interrupt a list command during highlighting
> by either SIGTERM or SIGINT.
> 
> In this case, we can use SIGTERM to interrupt the run command, but not SIGINT.
> 
> This is due to the fact that the infrun_quit_handler is still active.
> 
> The purpose of infrun_quit_handler is to:
> - if !target_terminal::is_ours, pass ^C to the inferior, and
> - if target_terminal::is_ours, ignore ^C.  If gdb is executing say a continue
>   in the background, it generates events and we don't want to interrupt the
>   handling of these events.
> 
> In this case however, the reason target_terminal::is_ours is not because
> there's background execution, but because normal_stop has decided that we have
> stopped execution, and it's preparing to hand back control to GDB.
> 
> Fix this by switching back to the default_quit_handler during notify_normal_stop.

Hmm.  This worries me.  GDB is preparing to hand back control, but it isn't
yet done doing that.  E.g., at the bottom of normal_stop, after notifying the
interpreters, we handle breakpoint auto delete.  And then the caller of normal_stop
still has run control work to do which we better not skip.  By switching to
the default_quit_handler in normal_stop, we risk a Ctrl-C running into a QUIT
and resulting in skipping all that code.  Similarly, imagine if the MI "normal_stop"
observer runs into a QUIT and throws due to a Ctrl-C, and that makes GDB not print
the *stopped async record or some other bit of important info for the frontend.
That would leave the frontend out of sync.

Also, overriding the overridden quit_handler is a bit of code smell.  We can't
be sure that the right handler is the default_quit_handler.  Better would be to force
the infrun_quit_handler override scoped_restore dtor (e.g., by wrapping that
scoped_restore in an optional, and passing down a reference to that so we could
clear the optional.)  But that would likely look not-so-pretty.

It just seems way safer to do this, to drop this patch.  This means not using QUIT in
the next patch after all...  I will reply to that patch next.

Pedro Alves