Re: [PATCHv5 01/11] gdb: include breakpoint number in testing condition error message

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> Hi,
>
> On 2023-03-16 17:36, Andrew Burgess via Gdb-patches wrote:
>
>> So, in this commit, in preparation for the later commits, I propose to
>> change the 'Error in testing breakpoint condition:' line to this:
>> 
>>   Error in testing condition for breakpoint NUMBER:
>> 
>> where NUMBER will be filled in as appropriate.  Here's the first
>> example with the updated error:
>> 
>>   (gdb) break foo if (*(int *) 0) == 0
>>   Breakpoint 1 at 0x40111e: file bpcond.c, line 11.
>>   (gdb) r
>>   Starting program: /tmp/bpcond
>>   Error in testing condition for breakpoint 1:
>>   Cannot access memory at address 0x0
>> 
>>   Breakpoint 1, foo () at bpcond.c:11
>>   11	  int a = 32;
>>   (gdb)
>> 
>
> Reading this, I was thinking that we should print the breakpoint location number
> as well.  We do print it when presenting breakpoint stops nowadays, like:
>
>  (top-gdb) b printf
>  Breakpoint 3 at 0xee680 (2 locations)
>  (top-gdb) r
>  ...
>  Breakpoint 3.1, ui_file::printf ...
>  ...
>
> Did you consider that and decided against?  I didn't find any mention of it in previous
> discussions.

No, this was just an oversight, and would I think be a great
improvement.

The patch below implements this idea.  Let me know what you think.

Thanks,
Andrew

---

commit ead767dcb1f5023ed1ce98001672a63cc3737bdb
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri Jul 7 16:36:26 2023 +0100

    gdb: include location number in breakpoint error message
    
    This commit improves the output of this previous commit:
    
      commit 2dc3457a454a35d0617dc1f9cc1db77468471f95
      Date:   Fri Oct 14 13:22:55 2022 +0100
    
          gdb: include breakpoint number in testing condition error message
    
    The earlier commit extended the error message:
    
      Error in testing breakpoint condition:
    
    to include the breakpoint number, e.g.:
    
      Error in testing breakpoint condition 3:
    
    This commit extends takes this further, and includes the location
    number if the breakpoint has multiple locations, so we might now see:
    
      Error in testing breakpoint condition 3.2:
    
    Just as with how GDB reports a normal breakpoint stop, if a breakpoint
    only has a single location then the location number is not included,
    this keeps things nice and consistent.
    
    I've extended one of the tests to cover the new functionality.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index da6c8de9d14..d898167b7e1 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5544,9 +5544,17 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
 	    }
 	  catch (const gdb_exception_error &ex)
 	    {
-	      exception_fprintf (gdb_stderr, ex,
-				 "Error in testing condition for breakpoint %d:\n",
-				 b->number);
+	      int locno = bpstat_locno (bs);
+	      if (locno != 0)
+		exception_fprintf
+		  (gdb_stderr, ex,
+		   "Error in testing condition for breakpoint %d.%d:\n",
+		   b->number, locno);
+	      else
+		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
diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.c b/gdb/testsuite/gdb.base/bp-cond-failure.c
index 2a9974b47ce..8c226edd8b4 100644
--- a/gdb/testsuite/gdb.base/bp-cond-failure.c
+++ b/gdb/testsuite/gdb.base/bp-cond-failure.c
@@ -15,16 +15,26 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-int
+static inline int __attribute__((__always_inline__))
 foo ()
 {
-  return 0;	/* Breakpoint here.  */
+  return 0;	/* Multi-location breakpoint here.  */
+}
+
+static int __attribute__((noinline))
+bar ()
+{
+  int res = foo ();	/* Single-location breakpoint here.  */
+
+  return res;
 }
 
 int
 main ()
 {
-  int res = foo ();
+  int res = bar ();
+
+  res = foo ();
 
   return res;
 }
diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.exp b/gdb/testsuite/gdb.base/bp-cond-failure.exp
index cb572203772..bc7d8d13b9f 100644
--- a/gdb/testsuite/gdb.base/bp-cond-failure.exp
+++ b/gdb/testsuite/gdb.base/bp-cond-failure.exp
@@ -44,10 +44,7 @@ if { [is_address_zero_readable] } {
     return
 }
 
-# Where the breakpoint will be placed.
-set bp_line [gdb_get_line_number "Breakpoint here"]
-
-proc run_test { cond_eval access_type } {
+proc run_test { cond_eval access_type lineno nloc } {
     clean_restart ${::binfile}
 
     if { ![runto_main] } {
@@ -59,17 +56,23 @@ proc run_test { cond_eval access_type } {
     }
 
     # Setup the conditional breakpoint and record its number.
-    gdb_breakpoint "${::srcfile}:${::bp_line} if (*(${access_type} *) 0) == 0"
+    gdb_breakpoint "${::srcfile}:${lineno} if (*(${access_type} *) 0) == 0"
     set bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*"]
 
+    if { $nloc > 1 } {
+	set bp_num_pattern "${bp_num}.1"
+    } else {
+	set bp_num_pattern "${bp_num}"
+    }
+
     gdb_test "continue" \
 	[multi_line \
 	     "Continuing\\." \
-	     "Error in testing condition for breakpoint ${bp_num}:" \
+	     "Error in testing condition for breakpoint ${bp_num_pattern}:" \
 	     "Cannot access memory at address 0x0" \
 	     "" \
-	     "Breakpoint ${bp_num}, foo \\(\\) at \[^\r\n\]+:${::bp_line}" \
-	     "${::decimal}\\s+\[^\r\n\]+Breakpoint here\[^\r\n\]+"]
+	     "Breakpoint ${bp_num_pattern}, \(foo\|bar\) \\(\\) at \[^\r\n\]+:${lineno}" \
+	     "${::decimal}\\s+\[^\r\n\]+ breakpoint here\\. \[^\r\n\]+"]
 }
 
 # If we're using a remote target then conditions could be evaulated
@@ -97,8 +100,19 @@ gdb_test_multiple "show breakpoint condition-evaluation" "" {
     }
 }
 
+# Where the breakpoint will be placed.
+set bp_line_multi_loc [gdb_get_line_number "Multi-location breakpoint here"]
+set bp_line_single_loc [gdb_get_line_number "Single-location breakpoint here"]
+
+
+
 foreach_with_prefix access_type { "char" "short" "int" "long long" } {
     foreach_with_prefix cond_eval $cond_eval_modes {
-	run_test $cond_eval $access_type
+	with_test_prefix "multi-loc" {
+	    run_test $cond_eval $access_type $bp_line_multi_loc 2
+	}
+	with_test_prefix "single-loc" {
+	    run_test $cond_eval $access_type $bp_line_single_loc 1
+	}
     }
 }