[PATCH 0/2] New test for find_pc_sect_line

[PATCH 0/2] New test for find_pc_sect_line

[Andrew Burgess]

Adds a new test to extend coverage of find_pc_sect_line, then adds
some asserts and safety checks to skip_prologue_using_sal.

---

Andrew Burgess (2):
  gdb/testsuite: test for a function with no line table
  gdb: safety checks in skip_prologue_using_sal

 gdb/symtab.c                                  |   8 +-
 gdb/testsuite/gdb.dwarf2/missing-line-table.c |  32 +++++
 .../gdb.dwarf2/missing-line-table.exp         | 122 ++++++++++++++++++
 3 files changed, 159 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/missing-line-table.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/missing-line-table.exp


base-commit: 66b8e6c7b8d3c137029f4f2ab8c5b798aa1cbdd7
-- 
2.25.4

[PATCH 1/2] gdb/testsuite: test for a function with no line table

[Andrew Burgess]

This commit adds a test for the following commit:

  commit e86e87f77fd5d8afb3e714f1d9e09e0ff5b4e6ff
  Date:   Tue Nov 28 16:23:32 2006 +0000

              * symtab.c (find_pc_sect_line): Do not return a line before
              the start of a symtab.

We have been carrying a test for that commit in the Fedora GDB tree
since that commit was added to GDB.  I don't know why the test wasn't
added along with the original commit, but as was written, the test is
pretty gross, it uses objcopy to pull the .text section from an object
file, which was then injected into another source file within a .asm
statement...

... these days we can just make use of the DWARF assembler to achieve
the same results, so I've rewritten the test and think it is worth
adding this to upstream GDB.

The original patch was about about how we find the best symtab and
line table entry, and what to do when GDB can't find a good match.

The new test creates a CU with two functions, only one of which is
covered by the line table.  With the above patch reverted GDB returns
an invalid address.

With the above patch reverted I did run the testsuite to see what
other tests might already be exercising this functionality, and I
found two tests:

  gdb.dwarf2/dw2-step-out-of-function-no-stmt.exp
  gdb.dwarf2/dw2-vendor-extended-opcode.exp

These are pretty similar, they either create minimal, or no line table
for one of the functions in the source file, and as a consequence GDB
returns an unexpected address at some point during the test.

However, both of those tests are really focused on other issues, so I
think this new test does add some value.  Plus the new test is not
large, so it's not a huge cost to also run this new test.
---
 gdb/testsuite/gdb.dwarf2/missing-line-table.c |  32 +++++
 .../gdb.dwarf2/missing-line-table.exp         | 122 ++++++++++++++++++
 2 files changed, 154 insertions(+)
 create mode 100644 gdb/testsuite/gdb.dwarf2/missing-line-table.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/missing-line-table.exp

diff --git a/gdb/testsuite/gdb.dwarf2/missing-line-table.c b/gdb/testsuite/gdb.dwarf2/missing-line-table.c
new file mode 100644
index 00000000000..656fa0687ef
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/missing-line-table.c
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+void
+foo (void)
+{
+  asm ("foo_label: .globl foo_label");
+}
+
+int
+main()
+{				/* L1 */
+  asm ("main_label: .globl main_label");
+  foo ();			/* L2 */
+
+  asm ("main_label_2: .globl main_label_2");
+  return 0;			/* L3 */
+}
diff --git a/gdb/testsuite/gdb.dwarf2/missing-line-table.exp b/gdb/testsuite/gdb.dwarf2/missing-line-table.exp
new file mode 100644
index 00000000000..a66d853726d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/missing-line-table.exp
@@ -0,0 +1,122 @@
+# Copyright 2023 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/>.
+
+# Create a compilation unit containing two functions (the CU's
+# low/high pc range will include both functions), but define a line
+# table that only covers one of the functions.
+#
+# Use GDB to try and place a breakpoint in both functions.  GDB should
+# correctly find some address within both functions at which to place
+# a breakpoint.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support the DWARF
+# assembler.
+require dwarf2_support
+
+standard_testfile .c -dw.S
+
+get_func_info main
+get_func_info foo
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    declare_labels Llines
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name missing-line-table.c}
+	    {stmt_list $Llines DW_FORM_sec_offset}
+	    {DW_AT_low_pc $::foo_start DW_FORM_addr}
+	    {DW_AT_high_pc $::main_end DW_FORM_addr}
+	} {
+	    subprogram {
+		{external 1 flag}
+		{MACRO_AT_func {main}}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{MACRO_AT_func {foo}}
+	    }
+	}
+    }
+
+    lines {version 2} Llines {
+	include_dir "${::srcdir}/${::subdir}"
+	file_name "$::srcfile" 1
+
+	program {
+	    DW_LNE_set_address main
+	    line [gdb_get_line_number "L1"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address main_label
+	    line [gdb_get_line_number "L2"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address main_label_2
+	    line [gdb_get_line_number "L3"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address "$::main_start + $::main_len"
+	    DW_LNE_end_sequence
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+# Place a breakpoint on FUNC.  Check that the address at which the
+# breakpoint is placed lies within FUNC which is starts at address
+# START, and address END is the first address after FUNC.
+proc check_breakpoint { func start end } {
+    with_test_prefix "func=$func" {
+	set addr "*unknown*"
+	gdb_test_multiple "break $func" "place breakpoint" {
+	    -re -wrap "Breakpoint $::decimal at ($::hex)(?:: \[^\r\n\]+)?" {
+		set addr $expect_out(1,string)
+		pass $gdb_test_name
+	    }
+	}
+
+	verbose -log "breakpoint at: $addr"
+	verbose -log "$func start: $start, $func end: $end"
+
+	gdb_assert {$addr >= $start && $addr < $end} \
+	    "check breakpoint address is within function"
+    }
+}
+
+# The get_func_info calls declare the function start/end in terms of a
+# program label and offsets, but we now need these as actual
+# addresses.  As the program is running, this can be done easily by
+# asking GDB to evaluate each in turn.
+foreach func {foo main} {
+    foreach part {start end} {
+	set ${func}_${part} \
+	    [get_hexadecimal_valueof [set ${func}_${part}] "??" \
+		 "get value of ${func}_${part}"]
+    }
+}
+
+# Place breakpoints within 'foo' and 'main', and check that the
+# breakpoint is actually placed within the function.
+check_breakpoint "foo" $foo_start $foo_end
+check_breakpoint "main" $main_start $main_end
-- 
2.25.4

[PATCH 2/2] gdb: safety checks in skip_prologue_using_sal

[Andrew Burgess]

While working on the previous patch I reverted this commit:

  commit e86e87f77fd5d8afb3e714f1d9e09e0ff5b4e6ff
  Date:   Tue Nov 28 16:23:32 2006 +0000

          * symtab.c (find_pc_sect_line): Do not return a line before
            the start of a symtab.

When I re-ran the testsuite I saw some GDB crashes in the tests:

  gdb.dwarf2/dw2-line-number-zero.exp
  gdb.dwarf2/dw2-lines.exp
  gdb.dwarf2/dw2-vendor-extended-opcode.exp

GDB was reading beyond the end of an array in the function
skip_prologue_using_sal.

Now, without the above commit reverted I don't believe that this
should ever happen.  Reverting the above commit effectively breaks
GDB's symtab_and_line lookup, we try to find a result for an address,
and return the wrong symtab and line-table.  In
skip_prologue_using_sal we then walk the line table looking for an
appropriate entry, except we never find one, and GDB just keeps going,
wandering off the end of the array.

However, I think adding extra protection to prevent walking off the
end of the array is pretty cheap, and if something does go wrong in
the future then this should prevent a random crash.

Obviously, I have no reproducer for this, as I said, I don't think
this should impact GDB at all, this is just adding a little extra
caution.
---
 gdb/symtab.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 4f28667b1b3..5e1b9d91879 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3953,15 +3953,17 @@ skip_prologue_using_sal (struct gdbarch *gdbarch, CORE_ADDR func_addr)
 	  struct objfile *objfile
 	    = prologue_sal.symtab->compunit ()->objfile ();
 	  const linetable *linetable = prologue_sal.symtab->linetable ();
+	  gdb_assert (linetable->nitems > 0);
 	  int idx = 0;
 
 	  /* Skip any earlier lines, and any end-of-sequence marker
 	     from a previous function.  */
-	  while (linetable->item[idx].pc (objfile) != prologue_sal.pc
-		 || linetable->item[idx].line == 0)
+	  while (idx + 1 < linetable->nitems
+		 && (linetable->item[idx].pc (objfile) != prologue_sal.pc
+		     || linetable->item[idx].line == 0))
 	    idx++;
 
-	  if (idx+1 < linetable->nitems
+	  if (idx + 1 < linetable->nitems
 	      && linetable->item[idx+1].line != 0
 	      && linetable->item[idx+1].pc (objfile) == start_pc)
 	    return start_pc;
-- 
2.25.4

Re: [PATCH 0/2] New test for find_pc_sect_line

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Adds a new test to extend coverage of find_pc_sect_line, then adds
Andrew> some asserts and safety checks to skip_prologue_using_sal.

These both seem fine to me.

Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 0/2] New test for find_pc_sect_line

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> Adds a new test to extend coverage of find_pc_sect_line, then adds
> Andrew> some asserts and safety checks to skip_prologue_using_sal.
>
> These both seem fine to me.
>
> Reviewed-By: Tom Tromey <tom@tromey.com>

Pushed.

Thanks,
Andrew