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

[Andrew Burgess <aburgess@redhat.com>]

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.

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? 

Thanks,
Andrew

>  
>  	    if (should_notify_stop)
>  	      {
>
> base-commit: c0c3bb70f2f13e07295041cdf24a4d2997fe99a4
> prerequisite-patch-id: c39ea2983f5abc3fac4664e7ea78420409d3df86
> -- 
> 2.34.1


---

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3dd24fccf6e..83c5c6c51d6 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);
 
+    int stop_id = get_stop_id ();
+
     /* Now figure out what to do with the result of the result.  */
     handle_inferior_event (&ecs);
 
@@ -4415,7 +4420,7 @@ fetch_inferior_event ()
 	  }
 	else
 	  {
-	    bool should_notify_stop = true;
+	    bool should_notify_stop = stop_id == get_stop_id ();
 	    bool proceeded = false;
 
 	    stop_all_threads_if_all_stop_mode ();
@@ -4424,7 +4435,8 @@ fetch_inferior_event ()
 
 	    if (thr != nullptr && thr->thread_fsm () != nullptr)
 	      should_notify_stop
-	       = thr->thread_fsm ()->should_notify_stop ();
+		= (should_notify_stop
+		   && thr->thread_fsm ()->should_notify_stop ());
 
 	    if (should_notify_stop)
 	      {