Re: [PATCHv5 05/11] gdb: don't always print breakpoint location after failed condition check

[Pedro Alves <pedro@palves.net>]

On 2023-07-07 22:18, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
>> On 2023-07-07 16:20, Pedro Alves wrote:
>>> I'd think the second *stopped shouldn't even be there at all, following the
>>> logic used to remove the stop from the CLI.
>>>
>>> Maybe we should try moving or copying this "don't report this stop" logic to
>>> fetch_inferior_event, to avoid calling the second normal_stop at all.
>>> I gave it a quick try, but what I tried without thinking much about it
>>> just hung GDB.  I didn't dig too much as I want to move on to review the rest
>>> of your series.
>>
>> Here's the totally broken naive patch I tried.
>>
>> From e1fde974fa58351de9f6dc0661162d77fc8be078 Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <pedro@palves.net>
>> Date: Fri, 7 Jul 2023 16:10:29 +0100
>> Subject: [PATCH] mi
>>
>> Change-Id: Ie49a36dc67b9584c40daabfffae1f87f6dd98c5c
>> ---
>>  gdb/infrun.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 3dd24fccf6e..ab1fd13abe0 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -4395,6 +4395,8 @@ fetch_inferior_event ()
>>      auto defer_delete_threads
>>        = make_scope_exit (delete_just_stopped_threads_infrun_breakpoints);
>>  
>> +    stop_context saved_context;
>> +
>>      /* Now figure out what to do with the result of the result.  */
>>      handle_inferior_event (&ecs);
>>  
>> @@ -4415,16 +4417,17 @@ fetch_inferior_event ()
>>  	  }
>>  	else
>>  	  {
>> -	    bool should_notify_stop = true;
>>  	    bool proceeded = false;
>>  
>>  	    stop_all_threads_if_all_stop_mode ();
>>  
>>  	    clean_up_just_stopped_threads_fsms (&ecs);
>>  
>> -	    if (thr != nullptr && thr->thread_fsm () != nullptr)
>> -	      should_notify_stop
>> -	       = thr->thread_fsm ()->should_notify_stop ();
>> +	    bool should_notify_stop
>> +		= (!saved_context.changed ()
>> +		   && thr != nullptr
>> +		   && thr->thread_fsm () != nullptr
>> +		   && thr->thread_fsm ()->should_notify_stop ());
> 
> So, I got this working, as in, not locking up GDB.  The problem was that
> thr->thread_fsm() is often nullptr, so we end up never calling
> normal_stop.
> 
> However, I don't think this is going to work out.  There's a couple of
> problems.
> 
> First, at the point fetch_inferior_event is called, there is no thread
> selected, so the ptid and thread captured in saved_context are always
> null_ptid and nullptr respectively.  By the time we call
> saved_context.changed() we do have a thread selected, so the context
> appears to have always changed.

Yeah.

> However, I figured I could work around this be replacing the full
> stop_context with a simple 'int stop_id = get_stop_id ();', I figured
> this might be enough, this will tell me if GDB has notified the user of
> a stop somewhere within the handle_inferior_event code.
> 
> This works fine in the sense that I do now correctly spot that the user
> has been notified of a stop and don't call normal_stop again...
> 
> ... but as a consequence, GDB now doesn't realise that is has stopped,
> and doesn't display a prompt back to the user.
> 
> I'm going to have to leave this for the weekend now, but will continue
> looking at this next week.  I've included my WIP hack below.
> 
> Would be interested on whether you see any problems with just using
> get_stop_id() to determine if the user has already seen a stop or not? 

I don't see a problem offhand.