[binutils-gdb] gdb: Don't skip prologue for explicit line breakpoints in assembler

[binutils-gdb] gdb: Don't skip prologue for explicit line breakpoints in assembler

[Andrew Burgess]

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

commit 5b0e2db4fa08b43e9ff78d6e9a45d496522bd241
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Jun 11 23:33:53 2019 +0100

    gdb: Don't skip prologue for explicit line breakpoints in assembler
    
    It was observed that in some cases, placing a breakpoint in an
    assembler file using filename:line-number syntax would result in the
    breakpoint being placed at a different line within the file.
    
    For example, consider this x86-64 assembler:
    
        test:
                push   %rbp		/* Break here.  */
                mov    %rsp, %rbp
                nop			/* Stops here.  */
    
    The user places the breakpoint using file:line notation targeting the
    line marked 'Break here', GDB actually stops at the line marked 'Stops
    here'.
    
    The reason is that the label 'test' is identified as the likely start
    of a function, and the call to symtab.c:skip_prologue_sal causes GDB
    to skip forward over the instructions that GDB believes to be part of
    the prologue.
    
    I believe however, that when debugging assembler code, where the user
    has instruction-by-instruction visibility, if they ask for a specific
    line, GDB should (as far as possible) stop on that line, and not
    perform any prologue skipping.  I don't believe that the behaviour of
    higher level languages should change, in these cases skipping the
    prologue seems like the correct thing to do.
    
    In order to implement this change I needed to extend our current
    tracking of when the user has requested an explicit line number.  We
    already tracked this in some cases, but not in others (see the changes
    in linespec.c).  However, once I did this I started to see some
    additional failures (in tests gdb.base/break-include.exp
    gdb.base/ending-run.exp gdb.mi/mi-break.exp gdb.mi/mi-reverse.exp
    gdb.mi/mi-simplerun.exp) where we currently expected a breakpoint
    placed at one file and line number to be updated to reference a
    different line number, this was fixed by removing some code in
    symtab.c:skip_prologue_sal.  My concern here is that removing this
    check didn't cause anything else to fail.
    
    I have a new test that covers my original case, this is written for
    x86-64 as most folk have access to such a target, however, any
    architecture that has a prologue scanner can be impacted by this
    change.
    
    gdb/ChangeLog:
    
    	* linespec.c (decode_digits_list_mode): Set explicit_line to a
    	bool value.
    	(decode_digits_ordinary): Set explicit_line field in sal.
    	* symtab.c (skip_prologue_sal): Don't skip prologue for a
    	symtab_and_line that was set on an explicit line number in
    	assembler code.  Do always update the recorded symtab and line if
    	we do skip the prologue.
    
    gdb/testsuite/ChangeLog:
    
    	* gdb.arch/amd64-break-on-asm-line.S: New file.
    	* gdb.arch/amd64-break-on-asm-line.exp: New file.

Diff:
---
 gdb/ChangeLog                                      | 10 +++++++
 gdb/linespec.c                                     |  3 +-
 gdb/symtab.c                                       | 21 ++++++++-----
 gdb/testsuite/ChangeLog                            |  5 ++++
 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S   | 35 ++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp | 35 ++++++++++++++++++++++
 6 files changed, 100 insertions(+), 9 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2fef7d2..0ad7576 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2019-07-09  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* linespec.c (decode_digits_list_mode): Set explicit_line to a
+	bool value.
+	(decode_digits_ordinary): Set explicit_line field in sal.
+	* symtab.c (skip_prologue_sal): Don't skip prologue for a
+	symtab_and_line that was set on an explicit line number in
+	assembler code.  Do always update the recorded symtab and line if
+	we do skip the prologue.
+
+2019-07-09  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* breakpoint.c (set_breakpoint_location_function): Remove
 	explicit_loc parameter.
 	(momentary_breakpoint_from_master): Update call to
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 8db3924..83468f8 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4101,7 +4101,7 @@ decode_digits_list_mode (struct linespec_state *self,
 	val.symtab = elt;
       val.pspace = SYMTAB_PSPACE (elt);
       val.pc = 0;
-      val.explicit_line = 1;
+      val.explicit_line = true;
 
       add_sal_to_sals (self, &values, &val, NULL, 0);
     }
@@ -4135,6 +4135,7 @@ decode_digits_ordinary (struct linespec_state *self,
 	  sal.pspace = SYMTAB_PSPACE (elt);
 	  sal.symtab = elt;
 	  sal.line = line;
+	  sal.explicit_line = true;
 	  sal.pc = pc;
 	  sals.push_back (std::move (sal));
 	}
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 4920d94..6e7e32f 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3673,8 +3673,10 @@ skip_prologue_using_lineinfo (CORE_ADDR func_addr, struct symtab *symtab)
 
 /* Adjust SAL to the first instruction past the function prologue.
    If the PC was explicitly specified, the SAL is not changed.
-   If the line number was explicitly specified, at most the SAL's PC
-   is updated.  If SAL is already past the prologue, then do nothing.  */
+   If the line number was explicitly specified then the SAL can still be
+   updated, unless the language for SAL is assembler, in which case the SAL
+   will be left unchanged.
+   If SAL is already past the prologue, then do nothing.  */
 
 void
 skip_prologue_sal (struct symtab_and_line *sal)
@@ -3693,6 +3695,15 @@ skip_prologue_sal (struct symtab_and_line *sal)
   if (sal->explicit_pc)
     return;
 
+  /* In assembly code, if the user asks for a specific line then we should
+     not adjust the SAL.  The user already has instruction level
+     visibility in this case, so selecting a line other than one requested
+     is likely to be the wrong choice.  */
+  if (sal->symtab != nullptr
+      && sal->explicit_line
+      && SYMTAB_LANGUAGE (sal->symtab) == language_asm)
+    return;
+
   scoped_restore_current_pspace_and_thread restore_pspace_thread;
 
   switch_to_program_space_and_thread (sal->pspace);
@@ -3812,12 +3823,6 @@ skip_prologue_sal (struct symtab_and_line *sal)
 
   sal->pc = pc;
   sal->section = section;
-
-  /* Unless the explicit_line flag was set, update the SAL line
-     and symtab to correspond to the modified PC location.  */
-  if (sal->explicit_line)
-    return;
-
   sal->symtab = start_sal.symtab;
   sal->line = start_sal.line;
   sal->end = start_sal.end;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index f8ef540..a7d09cb 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2019-07-09  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.arch/amd64-break-on-asm-line.S: New file.
+	* gdb.arch/amd64-break-on-asm-line.exp: New file.
+
 2019-07-08  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
 
 	* gdb.base/printcmds.exp: Test printing C string and
diff --git a/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
new file mode 100644
index 0000000..141f71f
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
@@ -0,0 +1,35 @@
+/* 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 file is part of the gdb testsuite.
+
+   Test that a breakpoint placed by line number in an assembler file
+   will stop at the specified line.  Previously versions of GDB have
+   incorrectly invoked the prologue analysis logic and skipped
+   forward.  */
+
+	.text
+	.global main
+main:
+	nop
+test:
+	/* The next two instructions are required to look like an
+	   x86-64 prologue so that GDB's prologue scanner will spot
+	   them and skip forward.  */
+	push    %rbp		/* Break here.  */
+	mov	%rsp, %rbp
+	nop			/* Incorrect.  */
+	nop
+	nop
diff --git a/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp
new file mode 100644
index 0000000..6218ce5
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp
@@ -0,0 +1,35 @@
+# 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/>.
+
+if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
+    return
+}
+
+standard_testfile .S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  { debug }] } {
+    untested "could not compile"
+    return -1
+}
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "Break here"]
+gdb_continue_to_breakpoint "Break on specified line" \
+    ".*/\\* Break here\\.  \\*/.*"