[2/2] "break LINENO/*ADDRESS", inline functions and "info break" output

Message ID 20180628145035.24713-2-palves@redhat.com
State New
Headers show
Series
  • [1/2] Fix running to breakpoint set in inline function by lineno/address
Related show

Commit Message

Pedro Alves June 28, 2018, 2:50 p.m.
While experimenting with the previous patch, I noticed this inconsistency
in GDB's output:

  (gdb) b 32
  Breakpoint 1 at 0x40062f: file inline-break.c, line 32.                  (1)
  (gdb) r
  ....
  Breakpoint 1, func1 (x=1) at inline-break.c:32                           (2)
  32        return x * 23; /* break here */
  (gdb) info breakpoints
  Num     Type           Disp Enb Address    What
  1       breakpoint     keep y   0x40062f   in main at inline-break.c:32  (3)
	  breakpoint already hit 1 time
  (gdb)

Notice that when the breakpoint as set, GDB showed "inline-break.c,
line 32" (1), the same line number that was specified in the command.

When we run to the breakpoint, we present the stop at the same line
number, and correctly show "func1" as the function name (2).

But in "info break" output (3), notice that we say "in main", not "in
func1".

The same thing happens if you set a breakpoint by address.  I.e.:

  (gdb) b *0x40062f
  Breakpoint 2 at 0x40062f: file inline-break.c, line 32.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   0x000000000040062f in main at inline-break.c:32
   (gdb) r
   ....
  Breakpoint 2, func1 (x=1) at inline-break.c:32
  32        return x * 23; /* break here */

The problem is that the breakpoints were set at an inline function,
but when we set such a breakpoint by line number or address, we don't
record the functions symbol in the sal, and as consequence the
breakpoint location does not have an associated symbol either.

Then, in print_breakpoint_location, if the location does not have a
symbol, we call find_pc_sect_function to find one, and this is what
finds "main", because find_pc_sect_function uses
block_linkage_function:

  /* Return the symbol for the function which contains a specified
     lexical block, described by a struct block BL.  The return value
     will not be an inlined function; the containing function will be
     returned instead.  */

  struct symbol *
  block_linkage_function (const struct block *bl)

To fix this, this commit adds an alternative to find_pc_sect_function
that uses block_containing_function instead:

  /* Return the symbol for the function which contains a specified
     block, described by a struct block BL.  The return value will be
     the closest enclosing function, which might be an inline
     function.  */

  struct symbol *
  block_containing_function (const struct block *bl)

(It seems odd to me that block_linkage_function says "the CONTAINING
function will be returned", and then block_containing_function says it
returns "the closest enclosing function".  Something seems reversed
here.  Still, I've kept the same nomenclature and copied the comments,
so that at least there's consistency.  Maybe we should fix that up
somehow.)

Then I wondered, why make print_breakpoint_location look up the symbol
every time it is called, instead of just always storing the symbol
when the location is created, since the location already stores the
symbol in some cases.  So to find which cases might be missing setting
the symbol in the sal which is used to create the breakpoint location,
I added an assertion to print_breakpoint_location, and ran the
testsuite.  That caught a few places, unsurprisingly:

 - setting a breakpoint by line number
 - setting a breapoint by address
 - ifunc resolving

Those are all fixed by this commit.  I decided not to add the
assertion to block_linkage_function and leave the existing "if (sym)"
check in place, because it's plausible that we have symtabs with line
info but no symbols.  I.e., that would not be a GDB bug, but
a peculiarity of debug info input.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* blockframe.c (find_pc_sect_containing_function): New function.
	* breakpoint.c (print_breakpoint_location): Don't call
	find_pc_sect_function.
	* linespec.c (create_sals_line_offset): Record the location's
	symbol in the sal.
	* linespec.c (convert_address_location_to_sals): Fill in sal's
	symbol with find_pc_sect_containing_function.
	* symtab.c (find_function_start_sal): Rename to ...
	(find_function_start_sal_1): ... this.
	(find_function_start_sal): Reimplement as wrapper around
	find_function_start_sal_1, and use
	find_pc_sect_containing_function to fill in the sal's symbol.
	(find_function_start_sal(symbol*, bool)): Adjust.
	* symtab.h (find_pc_function, find_pc_sect_function): Adjust
	comments.
	(find_pc_sect_containing_function): Declare.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.opt/inline-break.exp (line number, address): Add "info
	break" tests.
---
 gdb/blockframe.c                       | 13 +++++++++++++
 gdb/breakpoint.c                       |  3 ---
 gdb/linespec.c                         |  2 ++
 gdb/symtab.c                           | 32 +++++++++++++++++++++++++-------
 gdb/symtab.h                           | 15 +++++++++++++--
 gdb/testsuite/gdb.opt/inline-break.exp |  6 ++++++
 6 files changed, 59 insertions(+), 12 deletions(-)

-- 
2.14.4

Comments

Joel Brobecker June 28, 2018, 5:42 p.m. | #1
Hi Pedro,

> While experimenting with the previous patch, I noticed this inconsistency

> in GDB's output:

[...]
> When we run to the breakpoint, we present the stop at the same line

> number, and correctly show "func1" as the function name (2).

> 

> But in "info break" output (3), notice that we say "in main", not "in

> func1".


> (It seems odd to me that block_linkage_function says "the CONTAINING

> function will be returned", and then block_containing_function says it

> returns "the closest enclosing function".  Something seems reversed

> here.  Still, I've kept the same nomenclature and copied the comments,

> so that at least there's consistency.  Maybe we should fix that up

> somehow.)


That seems opposite to me as well...

> Then I wondered, why make print_breakpoint_location look up the symbol

> every time it is called, instead of just always storing the symbol

> when the location is created, since the location already stores the

> symbol in some cases.


Agreed.

> So to find which cases might be missing setting

> the symbol in the sal which is used to create the breakpoint location,

> I added an assertion to print_breakpoint_location, and ran the

> testsuite.  That caught a few places, unsurprisingly:

> 

>  - setting a breakpoint by line number

>  - setting a breapoint by address

>  - ifunc resolving

> 

> Those are all fixed by this commit.


Nice approach!

> I decided not to add the

> assertion to block_linkage_function and leave the existing "if (sym)"

> check in place, because it's plausible that we have symtabs with line

> info but no symbols.  I.e., that would not be a GDB bug, but

> a peculiarity of debug info input.


Agreed as well.

> gdb/ChangeLog:

> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

> 

> 	* blockframe.c (find_pc_sect_containing_function): New function.

> 	* breakpoint.c (print_breakpoint_location): Don't call

> 	find_pc_sect_function.

> 	* linespec.c (create_sals_line_offset): Record the location's

> 	symbol in the sal.

> 	* linespec.c (convert_address_location_to_sals): Fill in sal's

> 	symbol with find_pc_sect_containing_function.

> 	* symtab.c (find_function_start_sal): Rename to ...

> 	(find_function_start_sal_1): ... this.

> 	(find_function_start_sal): Reimplement as wrapper around

> 	find_function_start_sal_1, and use

> 	find_pc_sect_containing_function to fill in the sal's symbol.

> 	(find_function_start_sal(symbol*, bool)): Adjust.

> 	* symtab.h (find_pc_function, find_pc_sect_function): Adjust

> 	comments.

> 	(find_pc_sect_containing_function): Declare.


I know it might be considered a small and trivial part, but I really
appreciate the attention to the function's comment description.

> 

> gdb/testsuite/ChangeLog:

> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

> 

> 	* gdb.opt/inline-break.exp (line number, address): Add "info

> 	break" tests.


I went through the patch and it looks good.

Thanks again!
-- 
Joel
Pedro Alves June 29, 2018, 6:43 p.m. | #2
On 06/28/2018 06:42 PM, Joel Brobecker wrote:

>> gdb/testsuite/ChangeLog:

>> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

>>

>> 	* gdb.opt/inline-break.exp (line number, address): Add "info

>> 	break" tests.

> 

> I went through the patch and it looks good.


Great, thanks.  I pushed in both patches now.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index b3c9aa3bd73..6a018ccaefe 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -152,6 +152,19 @@  find_pc_function (CORE_ADDR pc)
   return find_pc_sect_function (pc, find_pc_mapped_section (pc));
 }
 
+/* See symtab.h.  */
+
+struct symbol *
+find_pc_sect_containing_function (CORE_ADDR pc, struct obj_section *section)
+{
+  const block *bl = block_for_pc_sect (pc, section);
+
+  if (bl == nullptr)
+    return nullptr;
+
+  return block_containing_function (bl);
+}
+
 /* These variables are used to cache the most recent result
    of find_pc_partial_function.  */
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 80df193a013..82dec7d418f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5867,9 +5867,6 @@  print_breakpoint_location (struct breakpoint *b,
     {
       const struct symbol *sym = loc->symbol;
 
-      if (sym == NULL)
-	sym = find_pc_sect_function (loc->address, loc->section);
-
       if (sym)
 	{
 	  uiout->text ("in ");
diff --git a/gdb/linespec.c b/gdb/linespec.c
index ae0200b8133..2a4189278ef 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2196,6 +2196,7 @@  create_sals_line_offset (struct linespec_state *self,
 
 	    if (self->funfirstline)
 	      skip_prologue_sal (&intermediate_results[i]);
+	    intermediate_results[i].symbol = sym;
 	    add_sal_to_sals (self, &values, &intermediate_results[i],
 			     sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0);
 	  }
@@ -2224,6 +2225,7 @@  convert_address_location_to_sals (struct linespec_state *self,
   sal.pc = address;
   sal.section = find_pc_overlay (address);
   sal.explicit_pc = 1;
+  sal.symbol = find_pc_sect_containing_function (sal.pc, sal.section);
 
   std::vector<symtab_and_line> sals;
   add_sal_to_sals (self, &sals, &sal, core_addr_to_string (address), 1);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 3e594e76c92..d8a7a16e073 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3575,11 +3575,12 @@  find_pc_line_pc_range (CORE_ADDR pc, CORE_ADDR *startptr, CORE_ADDR *endptr)
   return sal.symtab != 0;
 }
 
-/* See symtab.h.  */
+/* Helper for find_function_start_sal.  Does most of the work, except
+   setting the sal's symbol.  */
 
-symtab_and_line
-find_function_start_sal (CORE_ADDR func_addr, obj_section *section,
-			 bool funfirstline)
+static symtab_and_line
+find_function_start_sal_1 (CORE_ADDR func_addr, obj_section *section,
+			   bool funfirstline)
 {
   symtab_and_line sal = find_pc_sect_line (func_addr, section, 0);
 
@@ -3615,14 +3616,31 @@  find_function_start_sal (CORE_ADDR func_addr, obj_section *section,
 
 /* See symtab.h.  */
 
+symtab_and_line
+find_function_start_sal (CORE_ADDR func_addr, obj_section *section,
+			 bool funfirstline)
+{
+  symtab_and_line sal
+    = find_function_start_sal_1 (func_addr, section, funfirstline);
+
+  /* find_function_start_sal_1 does a linetable search, so it finds
+     the symtab and linenumber, but not a symbol.  Fill in the
+     function symbol too.  */
+  sal.symbol = find_pc_sect_containing_function (sal.pc, sal.section);
+
+  return sal;
+}
+
+/* See symtab.h.  */
+
 symtab_and_line
 find_function_start_sal (symbol *sym, bool funfirstline)
 {
   fixup_symbol_section (sym, NULL);
   symtab_and_line sal
-    = find_function_start_sal (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)),
-			       SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym),
-			       funfirstline);
+    = find_function_start_sal_1 (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)),
+				 SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym),
+				 funfirstline);
   sal.symbol = sym;
   return sal;
 }
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 84fc8976582..0b155d06592 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1670,14 +1670,25 @@  extern struct type *lookup_enum (const char *, const struct block *);
 
 /* from blockframe.c: */
 
-/* lookup the function symbol corresponding to the address.  */
+/* lookup the function symbol corresponding to the address.  The
+   return value will not be an inlined function; the containing
+   function will be returned instead.  */
 
 extern struct symbol *find_pc_function (CORE_ADDR);
 
-/* lookup the function corresponding to the address and section.  */
+/* lookup the function corresponding to the address and section.  The
+   return value will not be an inlined function; the containing
+   function will be returned instead.  */
 
 extern struct symbol *find_pc_sect_function (CORE_ADDR, struct obj_section *);
 
+/* lookup the function symbol corresponding to the address and
+   section.  The return value will be the closest enclosing function,
+   which might be an inline function.  */
+
+extern struct symbol *find_pc_sect_containing_function
+  (CORE_ADDR pc, struct obj_section *section);
+
 /* Find the symbol at the given address.  Returns NULL if no symbol
    found.  Only exact matches for ADDRESS are considered.  */
 
diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp
index 68b7b89ef31..183a1b927c6 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -272,6 +272,9 @@  with_test_prefix "line number" {
     # Set the breakpoint by line number, and check that GDB reports
     # the breakpoint location being the inline function.
     gdb_breakpoint "$srcfile:$line"
+
+    gdb_test "info break \$bpnum" "in func1 at .*$srcfile:$line"
+
     gdb_test "continue" "Breakpoint .*, func1 \\(x=1\\) at .*$srcfile:$line.*break here.*" \
 	"breakpoint hit presents stop at inlined function"
 
@@ -294,6 +297,9 @@  with_test_prefix "address" {
     # Set the breakpoint by address, and check that GDB reports the
     # breakpoint location being the inline function.
     gdb_test "break *$address" ".*Breakpoint .* at $address: file .*$srcfile, line $line."
+
+    gdb_test "info break \$bpnum" "in func1 at .*$srcfile:$line"
+
     gdb_test "continue" "Breakpoint .*, func1 \\(x=1\\) at .*$srcfile:$line.*break here.*" \
 	"breakpoint hit presents stop at inlined function"
 }