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

[Pedro Alves <pedro@palves.net>]

Hi!

On 2023-03-16 17:37, Andrew Burgess via Gdb-patches wrote:

> The user can still find the number of the breakpoint that triggered
> the initial stop in this line:
> 
>   Error in testing condition for breakpoint 1:
> 
> But there's now only one stop reason reported, the SIGSEGV, which I
> think is much clearer.

That's reasonable.

> @@ -5545,6 +5546,17 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>  	      exception_fprintf (gdb_stderr, ex,
>  				 "Error in testing condition for breakpoint %d:\n",
>  				 b->number);
> +
> +	      /* If the pc value changed as a result of evaluating the
> +		 condition then we probably stopped within an inferior
> +		 function call due to some unexpected stop, e.g. the thread
> +		 hit another breakpoint, or the thread received an
> +		 unexpected signal.  In this case we don't want to also
> +		 print the information about this breakpoint.  */
> +	      CORE_ADDR pc_after_check
> +		= get_frame_pc (get_selected_frame (nullptr));
> +	      if (pc_before_check != pc_after_check)
> +		bs->print = 0;


Not a great fan of this PC heuristic, though.  We can reuse the logic stop_id logic used by normal_stop
to detect whether a hook-stop changed the execution context.  See patch below.  It passes the
whole testsuite cleanly for me (on native, that's all I tested).  Note, it adds
thread_state to struct stop_context, because in run_inferior_call, the thread for which we are
saving the context for is THREAD_RUNNING.  In this particular scenario, probably only comparing
the last stop_id would suffice, but, it doesn't hurt to compare the whole context.  It can
actually be a bit better, in case e.g., the exception was due to the remote target connection
dropping, in which case the current get_selected_frame call would throw, while with this
approach, we'll just detect that the context changed.  (haven't actually tried that scenario,
just going by inspection.)

The other thing that I think we could/should do better is best seen with MI:

Before your change, we had:

 *stopped,reason="breakpoint-hit",disp="keep",bkptno="3",frame={addr="0x0000555555555149",func="func_bp",args=[],file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="32",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="2"
 &"Error in testing condition for breakpoint 2:\n"
 &"The program being debugged stopped while in a function called from GDB.\n"
 &"Evaluation of the expression containing the function\n"
 &"(func_bp) will be abandoned.\n"
 &"When the function is done executing, GDB will silently stop.\n"
 =breakpoint-modified,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x000055555555515d",func="foo",file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="39",thread-groups=["i1"],cond="(func_bp ())",times="1",original-location="infcall-failure.c:39"}
 ~"\n"
 ~"Breakpoint 2, func_bp () at gdb.base/infcall-failure.c:32\n"
 ~"32\t  int res = 0;\t/* Second breakpoint.  */\n"
 *stopped,reason="breakpoint-hit",disp="keep",bkptno="2",frame={addr="0x0000555555555149",func="func_bp",args=[],file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="32",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="2"
 (gdb) 

Note the last *stopped for the breakpoint we tried to evaluate the condition.

While in current master (with your patch merged), we have:

 *stopped,reason="breakpoint-hit",disp="keep",bkptno="3",frame={addr="0x0000555555555149",func="func_bp",args=[],file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="32",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="1"
 &"Error in testing condition for breakpoint 2:\n"
 &"The program being debugged stopped while in a function called from GDB.\n"
 &"Evaluation of the expression containing the function\n"
 &"(func_bp) will be abandoned.\n"
 &"When the function is done executing, GDB will silently stop.\n"
 =breakpoint-modified,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x000055555555515d",func="foo",file="gdb.base/infcall-failure.c",fullname="gdb.base/infcall-failure.c",line="39",thread-groups=["i1"],cond="(func_bp ())",times="1",original-location="infcall-failure.c:39"}
 *stopped                                 <<<<<<<<<<<<< here
 (gdb) 

Note the "*stopped" without any context in the "<<<< here" line.

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.

From e8584146cfbb372214097a519c5a2738a72fbb4d Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 7 Jul 2023 14:21:18 +0100
Subject: [PATCH] stop_context/stop_id instead of PC

Change-Id: I3e70c3fdb95b9bc4996f6c0c18431a3df48cc3a6
---
 gdb/breakpoint.c | 18 ++++++++----------
 gdb/infrun.c     | 30 ++++--------------------------
 gdb/infrun.h     | 27 +++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index da6c8de9d14..f791a74a246 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5535,9 +5535,11 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
 	  else
 	    within_current_scope = false;
 	}
-      CORE_ADDR pc_before_check = get_frame_pc (get_selected_frame (nullptr));
+
       if (within_current_scope)
 	{
+	  stop_context saved_context;
+
 	  try
 	    {
 	      condition_result = breakpoint_cond_eval (cond);
@@ -5548,15 +5550,11 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
 				 "Error in testing condition for breakpoint %d:\n",
 				 b->number);
 
-	      /* If the pc value changed as a result of evaluating the
-		 condition then we probably stopped within an inferior
-		 function call due to some unexpected stop, e.g. the thread
-		 hit another breakpoint, or the thread received an
-		 unexpected signal.  In this case we don't want to also
-		 print the information about this breakpoint.  */
-	      CORE_ADDR pc_after_check
-		= get_frame_pc (get_selected_frame (nullptr));
-	      if (pc_before_check != pc_after_check)
+	      /* If we stopped within an inferior function call,
+		 e.g. the thread hit another breakpoint, or the thread
+		 received an unexpected signal, don't print the
+		 information about this breakpoint.  */
+	      if (saved_context.changed ())
 		bs->print = 0;
 	    }
 	}
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 58da1cef29e..3dd24fccf6e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8755,31 +8755,6 @@ maybe_remove_breakpoints (void)
     }
 }
 
-/* The execution context that just caused a normal stop.  */
-
-struct stop_context
-{
-  stop_context ();
-
-  DISABLE_COPY_AND_ASSIGN (stop_context);
-
-  bool changed () const;
-
-  /* The stop ID.  */
-  ULONGEST stop_id;
-
-  /* The event PTID.  */
-
-  ptid_t ptid;
-
-  /* If stopp for a thread event, this is the thread that caused the
-     stop.  */
-  thread_info_ref thread;
-
-  /* The inferior that caused the stop.  */
-  int inf_num;
-};
-
 /* Initializes a new stop context.  If stopped for a thread event, this
    takes a strong reference to the thread.  */
 
@@ -8794,7 +8769,10 @@ stop_context::stop_context ()
       /* Take a strong reference so that the thread can't be deleted
 	 yet.  */
       thread = thread_info_ref::new_reference (inferior_thread ());
+      thread_state = thread->state;
     }
+  else
+    thread_state = THREAD_EXITED;
 }
 
 /* Return true if the current context no longer matches the saved stop
@@ -8807,7 +8785,7 @@ stop_context::changed () const
     return true;
   if (inf_num != current_inferior ()->num)
     return true;
-  if (thread != nullptr && thread->state != THREAD_STOPPED)
+  if (thread != nullptr && thread->state != thread_state)
     return true;
   if (get_stop_id () != stop_id)
     return true;
diff --git a/gdb/infrun.h b/gdb/infrun.h
index a343d27f72d..b5bd00f5561 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -416,5 +416,32 @@ struct scoped_enable_commit_resumed
   bool m_prev_enable_commit_resumed;
 };
 
+/* The execution context that just caused a normal stop.  */
+
+struct stop_context
+{
+  stop_context ();
+
+  DISABLE_COPY_AND_ASSIGN (stop_context);
+
+  bool changed () const;
+
+  /* The stop ID.  */
+  ULONGEST stop_id;
+
+  /* The event PTID.  */
+  ptid_t ptid;
+
+  /* If stopped for a thread event, this is the thread that caused the
+     stop.  */
+  thread_info_ref thread;
+
+  /* If stopped for a thread event, this is the state of the thread
+     that caused the stop.  */
+  enum thread_state thread_state;
+
+  /* The inferior that caused the stop.  */
+  int inf_num;
+};
 
 #endif /* INFRUN_H */

base-commit: c0c3bb70f2f13e07295041cdf24a4d2997fe99a4
-- 
2.34.1