diff mbox

[v2] Use SaL symbol name when reporting breakpoint locations

Message ID 20171020185101.1736-1-keiths@redhat.com
State New
Headers show

Commit Message

Keith Seitz Oct. 20, 2017, 6:51 p.m. UTC
This was formerly called "Report call site for inlined functions," but
given the feedback of that patch, I've removed the actual call-site information
from the patch entirely.  That greatly simplifies this patch, highlighting
the fundamental bug/feature that I trying to fix/implement.

--

Currently, "info break" can show some (perhaps) unexpected results when
setting a breakpoint on an inlined function:

(gdb) list
1	#include <stdio.h>
2
3	static inline void foo()
4	{
5	        printf("Hello world\n");
6	}
7
8	int main()
9	{
10	        foo();
11	        return 0;
12	}
13
(gdb) b foo
Breakpoint 1 at 0x400434: file foo.c, line 5.
(gdb) i b
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x0000000000400434 in main at foo.c:5

GDB reported that we understood what "foo" was, but we then report that the
breakpoint is actually set in main. While that is literally true, we can
do a little better.

This is accomplished by copying the symbol for which the breakpoint was set
into the bp_location.  From there, print_breakpoint_location can use this
information to print out symbol information (if available) instead of calling
find_pc_sect_function.

With the patch installed,

(gdb) i b
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x0000000000400434 in foo at foo.c:5

gdb/ChangeLog:

	* breakpoint.c (print_breakpoint_location): Use the symbol saved
	in the bp_location, falling back to find_pc_sect_function when
	needed.
	(add_location_to_breakpoint): Save sal->symbol.
	* breakpoint.h (struct bp_location) <symbol>: New field.
	* symtab.c (find_function_start_sal): Save the symbol into the SaL.
	* symtab.h (struct symtab_and_line) <symbol>: New field.

gdb/testsuite/ChangeLog:

	* gdb.opt/inline-break.exp (break_info_1): New procedure.
	Test "info break" for every inlined function breakpoint.
---
 gdb/breakpoint.c                       |  8 +++-
 gdb/breakpoint.h                       |  5 +++
 gdb/testsuite/gdb.opt/inline-break.exp | 75 ++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 2 deletions(-)

Comments

Pedro Alves Oct. 27, 2017, 2:32 p.m. UTC | #1
On 10/20/2017 07:51 PM, Keith Seitz wrote:

> +
> +    if {$source != ""} {
> +	set source "/$source:$line"

I suspect this '/' breaks with remote host testing.  See e.g.,:
https://sourceware.org/ml/gdb-patches/2013-04/msg00088.html

Otherwise LGTM.

Thanks,
Pedro Alves
Keith Seitz Oct. 27, 2017, 5:59 p.m. UTC | #2
On 10/27/2017 07:32 AM, Pedro Alves wrote:
> On 10/20/2017 07:51 PM, Keith Seitz wrote:
> 
>> +
>> +    if {$source != ""} {
>> +	set source "/$source:$line"
> 
> I suspect this '/' breaks with remote host testing.  See e.g.,:
> https://sourceware.org/ml/gdb-patches/2013-04/msg00088.html

Thank you for pointing that out. I did not remember that. [I really need to set up a remote host testing environment here!]

It makes no difference to the test if the '/' is in there (when `source' is added to the final regexp return by this procedure, it is preceded with ".*", so it makes no difference). As in the referenced message/patch, I've removed that character.

> Otherwise LGTM.

Pushed with that change, thank you again!

Keith
diff mbox

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 32ceea7c9b..2ede1cef96 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5956,8 +5956,11 @@  print_breakpoint_location (struct breakpoint *b,
     uiout->field_string ("what", event_location_to_string (b->location.get ()));
   else if (loc && loc->symtab)
     {
-      struct symbol *sym 
-	= find_pc_sect_function (loc->address, loc->section);
+      const struct symbol *sym = loc->symbol;
+
+      if (sym == NULL)
+	sym = find_pc_sect_function (loc->address, loc->section);
+
       if (sym)
 	{
 	  uiout->text ("in ");
@@ -8742,6 +8745,7 @@  add_location_to_breakpoint (struct breakpoint *b,
   loc->gdbarch = loc_gdbarch;
   loc->line_number = sal->line;
   loc->symtab = sal->symtab;
+  loc->symbol = sal->symbol;
 
   set_breakpoint_location_function (loc,
 				    sal->explicit_pc || sal->explicit_line);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 2b80ed9f46..2cc235c3a8 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -486,6 +486,11 @@  public:
      to find the corresponding source file name.  */
 
   struct symtab *symtab = NULL;
+
+  /* The symbol found by the location parser, if any.  This may be used to
+     ascertain when an event location was set at a different location than
+     the one originally selected by parsing, e.g., inlined symbols.  */
+  const struct symbol *symbol = NULL;
 };
 
 /* The possible return values for print_bpstat, print_it_normal,
diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp
index 7be3a34dbc..ec9ee74ec1 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -24,6 +24,62 @@  if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
     return -1
 }
 
+# Return a string that may be used to match the output of "info break NUM".
+#
+# Optional arguments:
+#
+# source    - the name of the source file
+# func      - the name of the function
+# disp      - the event disposition
+# enabled   - enable state
+# locs      - number of locations
+# line      - source line number (ignored without -source)
+
+proc break_info_1 {num args} {
+    global decimal
+
+    # Column delimiter
+    set c {[\t ]+}
+
+    # Row delimiter
+    set end {[\r\n \t]+}
+
+    # Table header
+    set header "[join [list Num Type Disp Enb Address What] ${c}]"
+
+    # Get/configure any optional parameters.
+    parse_args [list {source ""} {func ".*"} {disp "keep"} \
+		    {enabled "y"} {locs 1} [list line $decimal] \
+		    {type "breakpoint"}]
+
+    if {$source != ""} {
+	set source "/$source:$line"
+    }
+
+    # Result starts with the standard header.
+    set result "$header${end}"
+
+    # Set up for multi-location breakpoint marker.
+    if {$locs == 1} {
+	set multi ".*"
+    } else {
+	set multi "<MULTIPLE>${end}"
+    }
+    append result "[join [list $num $type $disp $enabled $multi] $c]"
+
+    # Add location info.
+    for {set i 1} {$i <= $locs} {incr i} {
+	if {$locs > 1} {
+	    append result "[join [list $num.$i $enabled] $c].*"
+	}
+
+	#  Add function/source file info.
+	append result "in $func at .*$source${end}"
+    }
+
+    return $result
+}
+
 #
 # func1 is a static inlined function that is called once.
 # The result should be a single-location breakpoint.
@@ -111,3 +167,22 @@  gdb_test "print func1" \
 #
 gdb_test "print func2" \
     "\\\$.* = {int \\(int\\)} .* <func2>"
+
+# Test that "info break" reports the location of the breakpoints "inside"
+# the inlined functions
+
+set results(1) [break_info_1 1 -source $srcfile -func "func1"]
+set results(2) [break_info_1 2 -locs 2 -source $srcfile -func "func2"]
+set results(3) [break_info_1 3 -source $srcfile -func "func3b"]
+set results(4) [break_info_1 4 -locs 2 -source $srcfile -func "func4b"]
+set results(5) [break_info_1 5 -locs 2 -source $srcfile -func "func5b"]
+set results(6) [break_info_1 6 -locs 3 -source $srcfile -func "func6b"]
+set results(7) [break_info_1 7 -locs 2 -source $srcfile -func "func7b"]
+set results(8) [break_info_1 8 -locs 3 -source $srcfile -func "func8b"]
+
+for {set i 1} {$i <= [array size results]} {incr i} {
+    send_log "Expecting: $results($i)\n"
+    gdb_test "info break $i" $results($i)
+}
+
+unset -nocomplain results