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

[Tom de Vries <tdevries@suse.de>]

On 10/20/23 21:16, Pedro Alves wrote:
> 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.

Ack, dropped in v5 ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203505.html ).

Thanks,
- Tom