Patchwork [review,v3] gdb: Don't reorder line table entries too much when sorting.

login
register
mail settings
Submitter Simon Marchi (Code Review)
Date Nov. 12, 2019, 12:38 p.m.
Message ID <20191112123805.CD25F28171@gnutoolchain-gerrit.osci.io>
Download mbox | patch
Permalink /patch/35822/
State New
Headers show

Comments

Simon Marchi (Code Review) - Nov. 12, 2019, 12:38 p.m.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/526
......................................................................

gdb: Don't reorder line table entries too much when sorting.

Don't reorder line table entries for the same address when sorting the
line table, maintain the compiler given line order.  Usually this will
reflect the order in which lines are conceptually encountered at a
given address.

Consider this example:

/* 1  */    volatile int global_var;
/* 2  */    int  __attribute__ ((noinline))
/* 3  */    bar ()
/* 4  */    {
/* 5  */      return global_var;
/* 6  */    }
/* 7  */    static inline int __attribute__ ((always_inline))
/* 8  */    foo ()
/* 9  */    {
/* 10 */      return bar ();
/* 11 */    }
/* 12 */    int
/* 13 */    main ()
/* 14 */    {
/* 15 */      global_var = 0;
/* 16 */      return foo ();
/* 17 */    }

GCC 10 currently generates a line table like this (as shown by
objdump):

  CU: ./test.c:
  File name          Line number    Starting address
  test.c                       4            0x4004b0
  test.c                       5            0x4004b0
  test.c                       6            0x4004b6
  test.c                       6            0x4004b7

  test.c                      14            0x4003b0
  test.c                      15            0x4003b0
  test.c                      16            0x4003ba
  test.c                      10            0x4003ba
  test.c                      10            0x4003c1

The interesting entries are those for lines 16 and 10 at address
0x4003ba, these represent the call to foo and the inlined body of
foo.

With the current line table sorting GDB builds the line table like
this (as shown by 'maintenance info line-table'):

  INDEX    LINE ADDRESS
  0          14 0x00000000004003b0
  1          15 0x00000000004003b0
  2          10 0x00000000004003ba
  3          16 0x00000000004003ba
  4         END 0x00000000004003c1
  5           4 0x00000000004004b0
  6           5 0x00000000004004b0
  7         END 0x00000000004004b7

Notice that entries 2 and 3 for lines 10 and 16 are now in a different
order to the line table as given by the compiler.  With this patch
applied the order is now:

  INDEX    LINE ADDRESS
  0          14 0x00000000004003b0
  1          15 0x00000000004003b0
  2          16 0x00000000004003ba
  3          10 0x00000000004003ba
  4         END 0x00000000004003c1
  5           4 0x00000000004004b0
  6           5 0x00000000004004b0
  7         END 0x00000000004004b7

Notice that entries 2 and 3 are now in their original order again.

The consequence of the incorrect ordering is that when stepping
through inlined functions GDB will display the wrong line for the
inner most frame.  Here's a GDB session before this patch is applied:

  Starting program: /home/andrew/tmp/inline/test

  Temporary breakpoint 1, main () at test.c:15
  15	/* 15 */      global_var = 0;
  (gdb) step
  16	/* 16 */      return foo ();
  (gdb) step
  foo () at test.c:16
  16	/* 16 */      return foo ();
  (gdb) step
  bar () at test.c:5
  5	/* 5  */      return global_var;

The step from line 15 to 16 was fine, but the next step should have
taken us to line 10, instead we are left at line 16.  The final step
to line 5 is as expected.

With this patch applied the session goes better:

  Starting program: /home/andrew/tmp/inline/test

  Temporary breakpoint 1, main () at test.c:15
  15	/* 15 */      global_var = 0;
  (gdb) step
  16	/* 16 */      return foo ();
  (gdb) step
  foo () at test.c:10
  10	/* 10 */      return bar ();
  (gdb) step
  bar () at test.c:5
  5	/* 5  */      return global_var;

We now visit the lines as 15, 16, 10, 5 as we would like.

The reason for this issue is that the inline frame unwinder is
detecting that foo is inlined in main.  When we stop at the shared
address 0x4003ba the inline frame unwinder first shows us the outer
frame, this information is extracted from the DWARF's
DW_TAG_inlined_subroutine entries and passed via GDB's block data.

When we step again the inlined frame unwinder moves us up the call
stack to the inner most frame at which point the frame is displayed as
normal, with the location for the address being looked up in the line
table.

As GDB uses the last line table entry for an address as "the" line to
report for that address it is critical that GDB maintain the order of
the line table entries.  In the first case, by reordering the line
table we report the wrong location.

I had to make a small adjustment in find_pc_sect_line in order to
correctly find the previous line in the line table.  In some line
tables I was seeing an actual line entry and an end of sequence marker
at the same address, before this commit these would reorder to move
the end of sequence marker before the line entry (end of sequence has
line number 0).  Now the end of sequence marker remains in its correct
location, and in order to find a previous line we should step backward
over any end of sequence markers.

As an example, the binary:
  gdb/testsuite/outputs/gdb.dwarf2/dw2-ranges-func/dw2-ranges-func-lo-cold

Has this line table before the patch:

  INDEX    LINE ADDRESS
  0          48 0x0000000000400487
  1         END 0x000000000040048e
  2          52 0x000000000040048e
  3          54 0x0000000000400492
  4          56 0x0000000000400497
  5         END 0x000000000040049a
  6          62 0x000000000040049a
  7         END 0x00000000004004a1
  8          66 0x00000000004004a1
  9          68 0x00000000004004a5
  10         70 0x00000000004004aa
  11         72 0x00000000004004b9
  12        END 0x00000000004004bc
  13         76 0x00000000004004bc
  14         78 0x00000000004004c0
  15         80 0x00000000004004c5
  16        END 0x00000000004004cc

And after this patch:

  INDEX    LINE ADDRESS
  0          48 0x0000000000400487
  1          52 0x000000000040048e
  2         END 0x000000000040048e
  3          54 0x0000000000400492
  4          56 0x0000000000400497
  5         END 0x000000000040049a
  6          62 0x000000000040049a
  7          66 0x00000000004004a1
  8         END 0x00000000004004a1
  9          68 0x00000000004004a5
  10         70 0x00000000004004aa
  11         72 0x00000000004004b9
  12        END 0x00000000004004bc
  13         76 0x00000000004004bc
  14         78 0x00000000004004c0
  15         80 0x00000000004004c5
  16        END 0x00000000004004cc

When calling find_pc_sect_line with the address 0x000000000040048e, in
both cases we find entry #3, we then try to find the previous entry,
which originally found this entry '2         52 0x000000000040048e',
after the patch it finds '2         END 0x000000000040048e', which
cases the lookup to fail.

By skipping the END marker after this patch we get back to the correct
entry, which is now #1: '1          52 0x000000000040048e', and
everything works again.

gdb/ChangeLog:

	* buildsym.c (lte_is_less_than): Delete.
	(buildsym_compunit::end_symtab_with_blockvector): Create local
	lambda function to sort line table entries, and use
	std::stable_sort instead of std::sort.
	* symtab.c (find_pc_sect_line): Skip backward over end of sequence
	markers when looking for a previous line.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/dw2-inline-stepping.c: New file.
	* gdb.dwarf2/dw2-inline-stepping.exp: New file.

Change-Id: Ia0309494be4cfd9dcc554f30209477f5f040b21b
---
M gdb/ChangeLog
M gdb/buildsym.c
M gdb/symtab.c
M gdb/testsuite/ChangeLog
A gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c
A gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp
6 files changed, 230 insertions(+), 25 deletions(-)

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ad07ca2..abce84d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@ 
 2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* buildsym.c (lte_is_less_than): Delete.
+	(buildsym_compunit::end_symtab_with_blockvector): Create local
+	lambda function to sort line table entries, and use
+	std::stable_sort instead of std::sort.
+	* symtab.c (find_pc_sect_line): Skip backward over end of sequence
+	markers when looking for a previous line.
+
+2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* dwarf2read.c (lnp_state_machine::record_line): Include
 	end_sequence parameter in debug print out.  Record the line if we
 	are at an end_sequence marker even if it's not the start of a
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 954a610..4f3d666 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -725,23 +725,6 @@ 
   e->pc = pc;
 }
 
-/* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */
-
-static bool
-lte_is_less_than (const linetable_entry &ln1, const linetable_entry &ln2)
-{
-  /* Note: this code does not assume that CORE_ADDRs can fit in ints.
-     Please keep it that way.  */
-  if (ln1.pc < ln2.pc)
-    return true;
-
-  if (ln1.pc > ln2.pc)
-    return false;
-
-  /* If pc equal, sort by line.  I'm not sure whether this is optimum
-     behavior (see comment at struct linetable in symtab.h).  */
-  return ln1.line < ln2.line;
-}
 
 /* Subroutine of end_symtab to simplify it.  Look for a subfile that
    matches the main source file's basename.  If there is only one, and
@@ -959,14 +942,25 @@ 
 	  linetablesize = sizeof (struct linetable) +
 	    subfile->line_vector->nitems * sizeof (struct linetable_entry);
 
-	  /* Like the pending blocks, the line table may be
-	     scrambled in reordered executables.  Sort it if
-	     OBJF_REORDERED is true.  */
+	  const auto lte_is_less_than
+	    = [] (const linetable_entry &ln1,
+		  const linetable_entry &ln2)->bool
+	      {
+		/* Note: this code does not assume that CORE_ADDRs can fit
+		   in ints.  Please keep it that way.  */
+		return (ln1.pc < ln2.pc);
+	      };
+
+	  /* Like the pending blocks, the line table may be scrambled in
+	     reordered executables.  Sort it if OBJF_REORDERED is true.  It
+	     is important to preserve the order of lines at the same
+	     address, as this maintains the inline function caller/callee
+	     relationships, this is why std::stable_sort is used.  */
 	  if (m_objfile->flags & OBJF_REORDERED)
-	    std::sort (subfile->line_vector->item,
-		       subfile->line_vector->item
-			 + subfile->line_vector->nitems,
-		       lte_is_less_than);
+	    std::stable_sort (subfile->line_vector->item,
+			      subfile->line_vector->item
+			      + subfile->line_vector->nitems,
+			      lte_is_less_than);
 	}
 
       /* Allocate a symbol table if necessary.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 2c934b9..91416a5 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3199,7 +3199,12 @@ 
       struct linetable_entry *last = item + len;
       item = std::upper_bound (first, last, pc, pc_compare);
       if (item != first)
-	prev = item - 1;		/* Found a matching item.  */
+	{
+	  /* Found a matching item.  Skip backwards over any end of
+	     sequence markers.  */
+	  for (prev = item - 1; prev->line == 0 && prev != first; prev--)
+	    /* Nothing.  */;
+	}
 
       /* At this point, prev points at the line whose start addr is <= pc, and
          item points at the next line.  If we ran off the end of the linetable
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index ba9a2b6..18ae148 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@ 
 2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* gdb.dwarf2/dw2-inline-stepping.c: New file.
+	* gdb.dwarf2/dw2-inline-stepping.exp: New file.
+
+2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gdb.base/maint.exp: Update line table parsing test.
 	* gdb.dwarf2/dw2-ranges-base.exp: Add new line table parsing test.
 
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c
new file mode 100644
index 0000000..41a8937
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.c
@@ -0,0 +1,45 @@ 
+/* Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This test relies on foo being inlined into main and bar not being
+   inlined.  The test is checking GDB's behaviour as we single step from
+   main through foo and into bar.  */
+
+volatile int global_var;
+
+int  __attribute__ ((noinline))
+bar ()
+{						/* bar prologue */
+  asm ("bar_label: .globl bar_label");
+  return global_var;				/* bar return global_var */
+}						/* bar end */
+
+static inline int __attribute__ ((always_inline))
+foo ()
+{						/* foo prologue */
+  return bar ();				/* foo call bar */
+}						/* foo end */
+
+int
+main ()
+{						/* main prologue */
+  int ans;
+  asm ("main_label: .globl main_label");
+  global_var = 0;				/* main set global_var */
+  asm ("main_label2: .globl main_label2");
+  ans = foo ();					/* main call foo */
+  asm ("main_label3: .globl main_label3");
+  return ans;
+}						/* main end */
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp
new file mode 100644
index 0000000..64da28a
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping.exp
@@ -0,0 +1,147 @@ 
+# Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This test shows the importance of not corrupting the order of line
+# table information.  When multiple lines are given for the same
+# address the compiler usually lists these in the order in which we
+# would expect to encounter them.  When stepping through nested inline
+# frames the last line given for an address is assumed by GDB to be
+# the most inner frame, and this is what GDB displays.
+#
+# If we corrupt the order of the line table entries then GDB will
+# display the wrong line as being the inner most frame.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+# The .c files use __attribute__.
+if [get_compiler_info] {
+    return -1
+}
+if !$gcc_compiled {
+    return 0
+}
+
+standard_testfile dw2-inline-stepping.c dw2-inline-stepping.S
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile2
+    declare_labels ranges_label lines_label foo_prog
+
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	main_start main_len
+    set main_end "$main_start + $main_len"
+    lassign [function_range bar [list ${srcdir}/${subdir}/$srcfile]] \
+	bar_start bar_len
+    set bar_end "$bar_start + $bar_len"
+
+    set call_line [gdb_get_line_number "main call foo"]
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name dw2-inline-stepping.c}
+	    {low_pc 0 addr}
+	    {stmt_list ${lines_label} DW_FORM_sec_offset}
+	    {ranges ${ranges_label} DW_FORM_sec_offset}
+	} {
+	    subprogram {
+		{external 1 flag}
+		{name bar}
+		{low_pc $bar_start addr}
+		{high_pc "$bar_start + $bar_len" addr}
+	    }
+	    foo_prog: subprogram {
+		{name foo}
+		{inline 3 data1}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{name main}
+		{low_pc $main_start addr}
+		{high_pc "$main_start + $main_len" addr}
+	    } {
+		inlined_subroutine {
+		    {abstract_origin %$foo_prog}
+		    {low_pc main_label2 addr}
+		    {high_pc main_label3 addr}
+		    {call_file 1 data1}
+		    {call_line $call_line data1}
+               }
+	    }
+	}
+    }
+
+    lines {version 2} lines_label {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	program {
+	    {DW_LNE_set_address $main_start}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "main prologue"] - 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "main set global_var"] - [gdb_get_line_number "main prologue"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label2}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "main call foo"] - [gdb_get_line_number "main set global_var"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label2}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo call bar"] - [gdb_get_line_number "main call foo"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $main_end}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $bar_start}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "bar prologue"] - 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address bar_label}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "bar return global_var"] - [gdb_get_line_number "bar prologue"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $bar_end}
+	    {DW_LNE_end_sequence}
+	}
+    }
+
+    ranges {is_64 [is_64_target]} {
+	ranges_label: sequence {
+	    {range {${main_start}} ${main_end}}
+	    {range {${bar_start}} ${bar_end}}
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+set patterns [list "main call foo" \
+		  "foo call bar" \
+		  "bar return global_var"]
+foreach p $patterns {
+    gdb_test "step" "/\\* $p \\*/" \
+	"step to '$p'"
+}
+