[binutils-gdb] gdb: Don't remove duplicate entries from the line table

[binutils-gdb] gdb: Don't remove duplicate entries from the line table

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=c90d28ac8903c5781b1a82e487072081383fd7b9

commit c90d28ac8903c5781b1a82e487072081383fd7b9
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Mar 23 12:40:24 2020 +0000

    gdb: Don't remove duplicate entries from the line table
    
    In this commit:
    
      commit 8c95582da858ac981f689a6f599acacb8c5c490f
      Date:   Mon Dec 30 21:04:51 2019 +0000
    
          gdb: Add support for tracking the DWARF line table is-stmt field
    
    A change was made in buildsym_compunit::record_line to remove
    duplicate line table entries in some cases.  This was an invalid
    change, as these duplicate line table entries are used in _some_ cases
    as part of prologue detection (see skip_prologue_using_sal).
    
    It might be possible to identify those line table entries that are
    required by skip_prologue_using_sal and only keep those duplicates
    around, however, I have not done this here.  The original duplicate
    removal was done because (a) it was easy to implement, and (b) it
    seemed obviously harmless.
    
    As (b) is now known to be false, and implementation would be more
    complex, and so (a) is also false.  As such, it seems better to keep
    all duplicates until an actual reason presents itself for why we
    should remove any.
    
    The original regression was spotted on RISC-V, which makes use of
    skip_prologue_using_sal as part of riscv_skip_prologue.  Originally I
    created the test gdb.dwarf2/dw2-inline-small-func.exp, however, this
    test will not compile on RISC-V as this target doesn't support
    .uleb128 or .sleb128 assembler directives containing complex
    expressions.  As a result I added the gdb.opt/inline-small-func.exp
    test, which exposes the bug on RISC-V, but obviously depends on the
    compiler to produce specific DWARF information in order to expose the
    bug.  Still this test does ensure we always get the desired result,
    even if the DWARF changes.
    
    Originally the gdb.dwarf2/dw2-inline-small-func.exp test passed on
    x86-64 even with the duplicate line table entries incorrectly
    removed.  The reason for this is that when a compilation unit doesn't
    have a 'producer' string then skip_prologue_using_sal is not used,
    instead the prologue is always skipped using analysis of the assembler
    code.
    
    However, for Clang on x86-64 skip_prologue_using_sal is used, so I
    modified the gdb.dwarf2/dw2-inline-small-func.exp test to include a
    'producer' string that names the Clang compiler.  With this done the
    test would fail on x86-64.
    
    One thing to note is that the gdb.opt/inline-small-func.exp test might
    fail on some targets.  For example, if we compare sparc to risc-v by
    looking at sparc32_skip_prologue we see that this function doesn't use
    skip_prologue_using_sal, but instead uses find_pc_partial_function
    directly.  I don't know the full history behind why the code is like
    it is, but it feels like sparc32_skip_prologue is an attempt to
    duplicate some of the functionality of skip_prologue_using_sal, but
    without all of the special cases.  If this is true then the new test
    could easily fail on this target, this would suggest that sparc should
    consider switching to use skip_prologue_using_sal like risc-v does.
    
    gdb/ChangeLog:
    
            * buildsym.c (buildsym_compunit::record_line): Remove
            deduplication code.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.dwarf2/dw2-inline-small-func-lbls.c: New file.
            * gdb.dwarf2/dw2-inline-small-func.c: New file.
            * gdb.dwarf2/dw2-inline-small-func.exp: New file.
            * gdb.dwarf2/dw2-inline-small-func.h: New file.
            * gdb.opt/inline-small-func.c: New file.
            * gdb.opt/inline-small-func.exp: New file.
            * gdb.opt/inline-small-func.h: New file.

Diff:
---
 gdb/ChangeLog                                      |   7 +
 gdb/buildsym.c                                     |  14 --
 gdb/testsuite/ChangeLog                            |  10 ++
 .../gdb.dwarf2/dw2-inline-small-func-lbls.c        |  37 +++++
 gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.c   |  22 +++
 gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.exp | 158 +++++++++++++++++++++
 gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.h   |  21 +++
 gdb/testsuite/gdb.opt/inline-small-func.c          |  22 +++
 gdb/testsuite/gdb.opt/inline-small-func.exp        |  60 ++++++++
 gdb/testsuite/gdb.opt/inline-small-func.h          |  21 +++
 10 files changed, 358 insertions(+), 14 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a6e0ec9d522..d8019713cdb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2020-04-02  Andrew Burgess  <andrew.burgess@embecosm.com>
+	    Bernd Edlinger <bernd.edlinger@hotmail.de>
+	    Tom Tromey  <tromey@adacore.com>
+
+	* buildsym.c (buildsym_compunit::record_line): Remove
+	deduplication code.
+
 2020-04-02  Tom de Vries  <tdevries@suse.de>
 
 	PR ada/24671
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 46c5bb1477b..fe071035542 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -681,20 +681,6 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
       m_have_line_numbers = true;
     }
 
-  if (subfile->line_vector->nitems > 0)
-    {
-      /* If we have a duplicate for the previous entry then ignore the new
-	 entry, except, if the new entry is setting the is_stmt flag, then
-	 ensure the previous entry respects the new setting.  */
-      e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
-      if (e->line == line && e->pc == pc)
-	{
-	  if (is_stmt && !e->is_stmt)
-	    e->is_stmt = 1;
-	  return;
-	}
-    }
-
   if (subfile->line_vector->nitems >= subfile->line_vector_length)
     {
       subfile->line_vector_length *= 2;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index eed6f45279f..f794bbd0d3c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,13 @@
+2020-04-02  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.dwarf2/dw2-inline-small-func-lbls.c: New file.
+	* gdb.dwarf2/dw2-inline-small-func.c: New file.
+	* gdb.dwarf2/dw2-inline-small-func.exp: New file.
+	* gdb.dwarf2/dw2-inline-small-func.h: New file.
+	* gdb.opt/inline-small-func.c: New file.
+	* gdb.opt/inline-small-func.exp: New file.
+	* gdb.opt/inline-small-func.h: New file.
+
 2020-04-02  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* lib/dwarf.exp (Dwarf::lines::program::DW_LNS_set_file): New
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func-lbls.c b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func-lbls.c
new file mode 100644
index 00000000000..5772131569c
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func-lbls.c
@@ -0,0 +1,37 @@
+/* Copyright 2020 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/>.  */
+
+/* Used to insert labels with which we can build a fake line table.  */
+#define LL(N) asm ("line_label_" #N ": .globl line_label_" #N)
+
+volatile int var;
+volatile int bar;
+
+/* Generate some code to take up some space.  */
+#define FILLER do { \
+    var = 99;	    \
+} while (0)
+
+int
+main ()
+{					/* main prologue */
+  asm ("main_label: .globl main_label");
+  LL (1);
+  FILLER;
+  LL (2);
+  FILLER;
+  LL (3);
+  return 0;				/* main end */
+}
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.c b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.c
new file mode 100644
index 00000000000..e2095e36625
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.c
@@ -0,0 +1,22 @@
+/* Copyright 2020 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/>.  */
+
+#include "dw2-inline-small-func.h"
+
+int
+main ()
+{		/* caller: before call.  */
+  callee ();	/* caller: the call.  */
+}		/* caller: after call.  */
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.exp b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.exp
new file mode 100644
index 00000000000..777db062b37
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.exp
@@ -0,0 +1,158 @@
+# Copyright 2020 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/>.
+
+# Check for an issue in GDB where buildsym_compunit::record_line was
+# removing duplicate line table entries, but skip_prologue_using_sal
+# depends on these duplicates to spot the end of the prologue.
+#
+# When the de-duplication was added this regression was not spotted as
+# it requires a particular combination of a (very) small function
+# being inlined into an also very small outer function.
+#
+# This test recreates the exact combination of line table entries that
+# were seen in the original test using the Dejagnu DWARF compiler.
+
+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-small-func-lbls.c dw2-inline-small-func.S \
+    dw2-inline-small-func.c dw2-inline-small-func.h
+
+# Extract the start, length, and end for function called NAME and
+# create suitable variables in the callers scope.
+proc get_func_info { name } {
+    global srcdir subdir srcfile
+
+    upvar 1 "${name}_start" func_start
+    upvar 1 "${name}_len" func_len
+    upvar 1 "${name}_end" func_end
+
+    lassign [function_range ${name} [list ${srcdir}/${subdir}/$srcfile] {debug optimize=-O1}] \
+	func_start func_len
+    set func_end "$func_start + $func_len"
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile3 srcfile4
+    declare_labels lines_label callee_subprog_label
+
+    get_func_info main
+
+    cu {} {
+	# It is important that the producer here be 'clang' as, at the
+	# time of writing this, GCC for x86-64 doesn't make use of
+	# skip_prologue_using_sal, while clang does.
+	compile_unit {
+	    {producer "clang xxxx" }
+	    {language @DW_LANG_C}
+	    {name ${srcfile3}}
+	    {low_pc 0 addr}
+	    {stmt_list ${lines_label} DW_FORM_sec_offset}
+	} {
+	    callee_subprog_label: subprogram {
+		{external 1 flag}
+		{name callee}
+		{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 %$callee_subprog_label}
+		    {low_pc line_label_1 addr}
+		    {high_pc line_label_2 addr}
+		    {call_file 1 data1}
+		    {call_line 21 data1}
+		}
+	    }
+	}
+    }
+
+    lines {version 2 default_is_stmt 1} lines_label {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile3" 1
+	file_name "$srcfile4" 1
+
+	set f1_l1 [gdb_get_line_number "caller: before call" $srcfile3]
+	set f1_l2 [gdb_get_line_number "caller: the call" $srcfile3]
+	set f1_l3 [gdb_get_line_number "caller: after call" $srcfile3]
+
+	set f2_l1 [gdb_get_line_number "callee: body" $srcfile4]
+
+	program {
+	    {DW_LNE_set_address line_label_1}
+	    {DW_LNS_advance_line [expr $f1_l1 - 1]}
+	    {DW_LNS_copy}
+
+	    {DW_LNS_advance_line [expr ${f1_l2} - ${f1_l1}]}
+	    {DW_LNS_copy}
+
+	    {DW_LNS_set_file 2}
+	    {DW_LNS_advance_line [expr ${f2_l1} - ${f1_l2}]}
+	    {DW_LNS_copy}
+
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+
+	    {DW_LNS_set_file 1}
+	    {DW_LNE_set_address line_label_2}
+	    {DW_LNS_advance_line [expr ${f1_l3} - ${f2_l1}]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address line_label_3}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug optimize=-O1}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# Delete all breakpoints so that the output of "info breakpoints"
+# below will only contain a single breakpoint.
+delete_breakpoints
+
+# Place a breakpoint within the function in the header file.
+set linenum [gdb_get_line_number "callee: body" $srcfile4]
+gdb_breakpoint "${srcfile4}:${linenum}"
+
+# Check that the breakpoint was placed where we expected.  It should
+# appear at the requested line.  When the bug in GDB was present the
+# breakpoint would be placed on one of the following lines instead.
+gdb_test "info breakpoints" \
+    ".* in callee at \[^\r\n\]+${srcfile4}:${linenum}\\y.*"
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.h b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.h
new file mode 100644
index 00000000000..69c67af6011
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-small-func.h
@@ -0,0 +1,21 @@
+/* Copyright 2020 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/>.  */
+
+int counter = 42;
+
+inline void
+callee () {
+  counter = 0;	/* callee: body.  */
+}
diff --git a/gdb/testsuite/gdb.opt/inline-small-func.c b/gdb/testsuite/gdb.opt/inline-small-func.c
new file mode 100644
index 00000000000..902674e8773
--- /dev/null
+++ b/gdb/testsuite/gdb.opt/inline-small-func.c
@@ -0,0 +1,22 @@
+/* Copyright 2020 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/>.  */
+
+#include "inline-small-func.h"
+
+int
+main ()
+{		/* caller: before call.  */
+  callee ();	/* caller: the call.  */
+}		/* caller: after call.  */
diff --git a/gdb/testsuite/gdb.opt/inline-small-func.exp b/gdb/testsuite/gdb.opt/inline-small-func.exp
new file mode 100644
index 00000000000..dfe046ad723
--- /dev/null
+++ b/gdb/testsuite/gdb.opt/inline-small-func.exp
@@ -0,0 +1,60 @@
+# Copyright 2020 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/>.
+
+# Check for an issue in GDB where buildsym_compunit::record_line was
+# removing duplicate line table entries, but skip_prologue_using_sal
+# depends on these duplicates to spot the end of the prologue.
+#
+# When the de-duplication was added this regression was not spotted as
+# it requires a particular combination of a (very) small function
+# being inlined into an also very small outer function.
+#
+# See also gdb.dwarf/dw2-inline-small-func.exp for a version of this
+# test that makes use of the Dejagnu DWARF compiler.
+#
+# This test simply compiles with optimization and checks that GDB can
+# do something suitable with the compiled binary.  Problems with this
+# test are most likely to occur when GDB asks the target specific code
+# to skip the prologue (gdbarch_skip_prologue).  Some targets make use
+# of skip_prologue_using_sal, which should be fine, however, some
+# targets make a poor attempt to duplicate parts of
+# skip_prologue_using_sal, these targets could easily fail this test.
+# This is not (necessarily) a problem with this test, but could
+# indicate a weakness with the target in question.
+
+standard_testfile inline-small-func.c inline-small-func.h
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile] {debug optimize=-O1}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# Delete all breakpoints so that the output of "info breakpoints"
+# below will only contain a single breakpoint.
+delete_breakpoints
+
+# Place a breakpoint within the function in the header file.
+set linenum [gdb_get_line_number "callee: body" $srcfile2]
+gdb_breakpoint "${srcfile2}:${linenum}"
+
+# Check that the breakpoint was placed where we expected.  It should
+# appear at the requested line.  When the bug in GDB was present the
+# breakpoint would be placed on one of the following lines instead.
+gdb_test "info breakpoints" \
+    ".* in callee at \[^\r\n\]+${srcfile2}:${linenum}\\y.*"
diff --git a/gdb/testsuite/gdb.opt/inline-small-func.h b/gdb/testsuite/gdb.opt/inline-small-func.h
new file mode 100644
index 00000000000..69c67af6011
--- /dev/null
+++ b/gdb/testsuite/gdb.opt/inline-small-func.h
@@ -0,0 +1,21 @@
+/* Copyright 2020 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/>.  */
+
+int counter = 42;
+
+inline void
+callee () {
+  counter = 0;	/* callee: body.  */
+}