* [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
@ 2019-06-12 12:34 Andrew Burgess
2019-06-20 1:11 ` Kevin Buettner
0 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-06-12 12:34 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
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.
---
gdb/ChangeLog | 10 +++++++
gdb/linespec.c | 3 +-
gdb/symtab.c | 15 ++++++----
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, 96 insertions(+), 7 deletions(-)
create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 94400f3f336..1d8d2ca3273 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4102,7 +4102,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);
}
@@ -4136,6 +4136,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 4920d94a247..c10e6b3e358 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3693,6 +3693,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 +3821,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/gdb.arch/amd64-break-on-asm-line.S b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
new file mode 100644
index 00000000000..698c3e6d624
--- /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 place 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 00000000000..6218ce541bd
--- /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\\. \\*/.*"
--
2.14.5
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-12 12:34 [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Andrew Burgess
@ 2019-06-20 1:11 ` Kevin Buettner
2019-06-20 20:58 ` Andrew Burgess
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Buettner @ 2019-06-20 1:11 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
On Wed, 12 Jun 2019 13:34:03 +0100
Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> 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.
I agree with all of this.
> 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.
Did you investigate the reason for the failures with this hunk
left in place?...
> @@ -3812,12 +3821,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->symt> FAIL: gdb.base/break-include.exp: break break-include.c:53
ab = start_sal.symtab;
> sal->line = start_sal.line;
> sal->end = start_sal.end;
The rest of the patch looks fine to me. Deleting those lines might
be okay also, but I'd like to understand why adding additional
explicit line number tracking caused these failures:
FAIL: gdb.base/break-include.exp: break break-include.c:53
FAIL: gdb.base/ending-run.exp: Cleared 2 by line
FAIL: gdb.base/ending-run.exp: clear 2 by default
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-20 1:11 ` Kevin Buettner
@ 2019-06-20 20:58 ` Andrew Burgess
2019-06-20 23:23 ` Andrew Burgess
0 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-06-20 20:58 UTC (permalink / raw)
To: Kevin Buettner; +Cc: gdb-patches
* Kevin Buettner <kevinb@redhat.com> [2019-06-19 18:11:47 -0700]:
> On Wed, 12 Jun 2019 13:34:03 +0100
> Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>
> > 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.
>
> I agree with all of this.
>
> > 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.
>
> Did you investigate the reason for the failures with this hunk
> left in place?...
>
> > @@ -3812,12 +3821,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->symt> FAIL: gdb.base/break-include.exp: break break-include.c:53
> ab = start_sal.symtab;
> > sal->line = start_sal.line;
> > sal->end = start_sal.end;
>
> The rest of the patch looks fine to me. Deleting those lines might
> be okay also, but I'd like to understand why adding additional
> explicit line number tracking caused these failures:
>
> FAIL: gdb.base/break-include.exp: break break-include.c:53
> FAIL: gdb.base/ending-run.exp: Cleared 2 by line
> FAIL: gdb.base/ending-run.exp: clear 2 by default
Thanks for taking a look at this.
Just to be clear, this is the hunk that is in question (in symtab.c):
@@ -3812,12 +3821,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;
If we take the first of these 'gdb.base/break-include.exp: break
break-include.c:53', then in a pre-patched test run the log looks like
this:
(gdb) break break-include.c:53
Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.inc, line 18.
(gdb) PASS: gdb.base/break-include.exp: break break-include.c:53
And in a post-patch world, but with the hunk above removed here's the
same part of the log file:
(gdb) break break-include.c:53
Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.c, line 54.
(gdb) FAIL: gdb.base/break-include.exp: break break-include.c:53
What we see is that in the failing case the line number has failed to
update from break-include.c:53 to break-include.inc:18, instead it has
updated to break-include.c:54.
To understand what's going on we need to consider two steps, first in
convert_linespec_to_sals the file and line is used to index into the
line table, this is where the break-include.c:54 comes from, there is
no entry for line 53, and 54 is the next available line.
Then skip_prologue_sal is called, this is where we move forward to
break-include.inc line 18, and this is where the difference is.
In the pre-patch world, the sal that is passe into skip_prologue_sal
is not marked as explicit_line, and so we successfully update the file
and line.
In the post patch world, the sal now is marked as explicit_line, so
the above check triggers and GDB doesn't update the file/line in the
sal, this leaves us stuck on break-include.c:54.
The other two failures all stem from the exact same problem, in
gdb.base/ending-run.exp many breakpoints are placed, and then cleared
using the 'clear' command. One of the breakpoints (breakpoint 4) is
placed at the wrong file/line as a result of not updating, this then
causes the 'clear' tests to not clear the expected breakpoints.
What worries more about the above hunk is that it never triggers
during testing (in a pre-patched world). I applied this patch to
master:
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 4920d94a247..5cd5bb69147 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3816,7 +3816,11 @@ skip_prologue_sal (struct symtab_and_line *sal)
/* 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;
+ {
+ fprintf ("Got here!\n");
+ abort ();
+ return;
+ }
sal->symtab = start_sal.symtab;
sal->line = start_sal.line;
And all the tests still pass. This code has been in place for ~9
years and unfortunately didn't have any tests associated when it was
added.
I've spent some time trying to figure out what conditions might need
to be true in order to trigger this code, but so far I've not managed
to figure it out - any suggestions would be appreciated.
Thanks,
Andrew
>
> Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-20 20:58 ` Andrew Burgess
@ 2019-06-20 23:23 ` Andrew Burgess
2019-06-21 3:20 ` Kevin Buettner
2019-06-21 13:43 ` Pedro Alves
0 siblings, 2 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-06-20 23:23 UTC (permalink / raw)
To: Kevin Buettner; +Cc: gdb-patches
* Andrew Burgess <andrew.burgess@embecosm.com> [2019-06-20 21:57:59 +0100]:
> * Kevin Buettner <kevinb@redhat.com> [2019-06-19 18:11:47 -0700]:
>
> > On Wed, 12 Jun 2019 13:34:03 +0100
> > Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> >
> > > 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.
> >
> > I agree with all of this.
> >
> > > 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.
> >
> > Did you investigate the reason for the failures with this hunk
> > left in place?...
> >
> > > @@ -3812,12 +3821,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->symt> FAIL: gdb.base/break-include.exp: break break-include.c:53
> > ab = start_sal.symtab;
> > > sal->line = start_sal.line;
> > > sal->end = start_sal.end;
> >
> > The rest of the patch looks fine to me. Deleting those lines might
> > be okay also, but I'd like to understand why adding additional
> > explicit line number tracking caused these failures:
> >
> > FAIL: gdb.base/break-include.exp: break break-include.c:53
> > FAIL: gdb.base/ending-run.exp: Cleared 2 by line
> > FAIL: gdb.base/ending-run.exp: clear 2 by default
>
> Thanks for taking a look at this.
>
> Just to be clear, this is the hunk that is in question (in symtab.c):
>
> @@ -3812,12 +3821,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;
>
>
> If we take the first of these 'gdb.base/break-include.exp: break
> break-include.c:53', then in a pre-patched test run the log looks like
> this:
>
> (gdb) break break-include.c:53
> Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.inc, line 18.
> (gdb) PASS: gdb.base/break-include.exp: break break-include.c:53
>
> And in a post-patch world, but with the hunk above removed here's the
> same part of the log file:
>
> (gdb) break break-include.c:53
> Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.c, line 54.
> (gdb) FAIL: gdb.base/break-include.exp: break break-include.c:53
>
> What we see is that in the failing case the line number has failed to
> update from break-include.c:53 to break-include.inc:18, instead it has
> updated to break-include.c:54.
>
> To understand what's going on we need to consider two steps, first in
> convert_linespec_to_sals the file and line is used to index into the
> line table, this is where the break-include.c:54 comes from, there is
> no entry for line 53, and 54 is the next available line.
>
> Then skip_prologue_sal is called, this is where we move forward to
> break-include.inc line 18, and this is where the difference is.
>
> In the pre-patch world, the sal that is passe into skip_prologue_sal
> is not marked as explicit_line, and so we successfully update the file
> and line.
>
> In the post patch world, the sal now is marked as explicit_line, so
> the above check triggers and GDB doesn't update the file/line in the
> sal, this leaves us stuck on break-include.c:54.
>
> The other two failures all stem from the exact same problem, in
> gdb.base/ending-run.exp many breakpoints are placed, and then cleared
> using the 'clear' command. One of the breakpoints (breakpoint 4) is
> placed at the wrong file/line as a result of not updating, this then
> causes the 'clear' tests to not clear the expected breakpoints.
>
> What worries more about the above hunk is that it never triggers
> during testing (in a pre-patched world). I applied this patch to
> master:
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 4920d94a247..5cd5bb69147 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3816,7 +3816,11 @@ skip_prologue_sal (struct symtab_and_line *sal)
> /* 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;
> + {
> + fprintf ("Got here!\n");
Ooops, that should be 'fprintf (stderr, "Got here!\n");' of course.
> + abort ();
> + return;
> + }
>
> sal->symtab = start_sal.symtab;
> sal->line = start_sal.line;
>
> And all the tests still pass. This code has been in place for ~9
> years and unfortunately didn't have any tests associated when it was
> added.
>
> I've spent some time trying to figure out what conditions might need
> to be true in order to trigger this code, but so far I've not managed
> to figure it out - any suggestions would be appreciated.
I spent some more time trying to find a path that would call both
'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
can't find one.
Looking back at how the explicit_line flag was originally used when
it was added in commit ed0616c6b78a0966, things have changed quite a
bit in the 10+ years since. There were some tests added along with
the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
master and in my patched branch.
My current thinking is that the explicit_line flag was no longer doing
anything useful in master, but if someone disagrees I'd love to
understand more about this.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-20 23:23 ` Andrew Burgess
@ 2019-06-21 3:20 ` Kevin Buettner
2019-06-21 13:43 ` Pedro Alves
1 sibling, 0 replies; 20+ messages in thread
From: Kevin Buettner @ 2019-06-21 3:20 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
On Fri, 21 Jun 2019 00:23:14 +0100
Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> * Andrew Burgess <andrew.burgess@embecosm.com> [2019-06-20 21:57:59 +0100]:
>
> > * Kevin Buettner <kevinb@redhat.com> [2019-06-19 18:11:47 -0700]:
> >
> > > On Wed, 12 Jun 2019 13:34:03 +0100
> > > Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> > >
> > > > 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.
> > >
> > > I agree with all of this.
> > >
> > > > 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.
> > >
> > > Did you investigate the reason for the failures with this hunk
> > > left in place?...
> > >
> > > > @@ -3812,12 +3821,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->symt> FAIL: gdb.base/break-include.exp: break break-include.c:53
> > > ab = start_sal.symtab;
> > > > sal->line = start_sal.line;
> > > > sal->end = start_sal.end;
> > >
> > > The rest of the patch looks fine to me. Deleting those lines might
> > > be okay also, but I'd like to understand why adding additional
> > > explicit line number tracking caused these failures:
> > >
> > > FAIL: gdb.base/break-include.exp: break break-include.c:53
> > > FAIL: gdb.base/ending-run.exp: Cleared 2 by line
> > > FAIL: gdb.base/ending-run.exp: clear 2 by default
> >
> > Thanks for taking a look at this.
> >
> > Just to be clear, this is the hunk that is in question (in symtab.c):
> >
> > @@ -3812,12 +3821,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;
> >
> >
> > If we take the first of these 'gdb.base/break-include.exp: break
> > break-include.c:53', then in a pre-patched test run the log looks like
> > this:
> >
> > (gdb) break break-include.c:53
> > Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.inc, line 18.
> > (gdb) PASS: gdb.base/break-include.exp: break break-include.c:53
> >
> > And in a post-patch world, but with the hunk above removed here's the
> > same part of the log file:
> >
> > (gdb) break break-include.c:53
> > Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.c, line 54.
> > (gdb) FAIL: gdb.base/break-include.exp: break break-include.c:53
> >
> > What we see is that in the failing case the line number has failed to
> > update from break-include.c:53 to break-include.inc:18, instead it has
> > updated to break-include.c:54.
> >
> > To understand what's going on we need to consider two steps, first in
> > convert_linespec_to_sals the file and line is used to index into the
> > line table, this is where the break-include.c:54 comes from, there is
> > no entry for line 53, and 54 is the next available line.
> >
> > Then skip_prologue_sal is called, this is where we move forward to
> > break-include.inc line 18, and this is where the difference is.
> >
> > In the pre-patch world, the sal that is passe into skip_prologue_sal
> > is not marked as explicit_line, and so we successfully update the file
> > and line.
> >
> > In the post patch world, the sal now is marked as explicit_line, so
> > the above check triggers and GDB doesn't update the file/line in the
> > sal, this leaves us stuck on break-include.c:54.
> >
> > The other two failures all stem from the exact same problem, in
> > gdb.base/ending-run.exp many breakpoints are placed, and then cleared
> > using the 'clear' command. One of the breakpoints (breakpoint 4) is
> > placed at the wrong file/line as a result of not updating, this then
> > causes the 'clear' tests to not clear the expected breakpoints.
> >
> > What worries more about the above hunk is that it never triggers
> > during testing (in a pre-patched world). I applied this patch to
> > master:
> >
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index 4920d94a247..5cd5bb69147 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3816,7 +3816,11 @@ skip_prologue_sal (struct symtab_and_line *sal)
> > /* 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;
> > + {
> > + fprintf ("Got here!\n");
>
> Ooops, that should be 'fprintf (stderr, "Got here!\n");' of course.
>
> > + abort ();
> > + return;
> > + }
> >
> > sal->symtab = start_sal.symtab;
> > sal->line = start_sal.line;
> >
> > And all the tests still pass. This code has been in place for ~9
> > years and unfortunately didn't have any tests associated when it was
> > added.
> >
> > I've spent some time trying to figure out what conditions might need
> > to be true in order to trigger this code, but so far I've not managed
> > to figure it out - any suggestions would be appreciated.
>
> I spent some more time trying to find a path that would call both
> 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
> can't find one.
>
> Looking back at how the explicit_line flag was originally used when
> it was added in commit ed0616c6b78a0966, things have changed quite a
> bit in the 10+ years since. There were some tests added along with
> the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
> master and in my patched branch.
>
> My current thinking is that the explicit_line flag was no longer doing
> anything useful in master, but if someone disagrees I'd love to
> understand more about this.
Hi Andrew,
Thanks for the additional analysis and explanation. I found the part
about the "Got here!" printf to be especially compelling.
Anyway, I'm convinced, so... Okay.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-20 23:23 ` Andrew Burgess
2019-06-21 3:20 ` Kevin Buettner
@ 2019-06-21 13:43 ` Pedro Alves
2019-06-22 11:06 ` Andrew Burgess
1 sibling, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2019-06-21 13:43 UTC (permalink / raw)
To: Andrew Burgess, Kevin Buettner; +Cc: gdb-patches
On 6/21/19 12:23 AM, Andrew Burgess wrote:
> I spent some more time trying to find a path that would call both
> 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
> can't find one.
But won't that change affect any code path that ends up in
skip_prologue_sal with explicit_line set?
E.g.:
/* Helper function for break_command_1 and disassemble_command. */
void
resolve_sal_pc (struct symtab_and_line *sal)
{
CORE_ADDR pc;
if (sal->pc == 0 && sal->symtab != NULL)
{
if (!find_line_pc (sal->symtab, sal->line, &pc))
error (_("No line %d in file \"%s\"."),
sal->line, symtab_to_filename_for_display (sal->symtab));
sal->pc = pc;
/* If this SAL corresponds to a breakpoint inserted using a line
number, then skip the function prologue if necessary. */
if (sal->explicit_line)
skip_prologue_sal (sal);
}
Is that path unreachable today?
>
> Looking back at how the explicit_line flag was originally used when
> it was added in commit ed0616c6b78a0966, things have changed quite a
> bit in the 10+ years since. There were some tests added along with
> the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
> master and in my patched branch.
>
> My current thinking is that the explicit_line flag was no longer doing
> anything useful in master, but if someone disagrees I'd love to
> understand more about this.
I seem to recall that GDB didn't use to update a breakpoint's line
number to advance to the next line number that includes some actual
compiled code. Like if you set a breakpoint at line 10 below:
10 // just a comment
11 i++;
you end up with a breakpoint at line 11. Maybe it's old code
related to that.
Maybe I'm misremembering.
In any case, if you change this, then you also should change
the function's entry comment:
/* 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. */
^^^^^^^^^^
void
skip_prologue_sal (struct symtab_and_line *sal)
{
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-21 13:43 ` Pedro Alves
@ 2019-06-22 11:06 ` Andrew Burgess
2019-06-22 11:23 ` Andrew Burgess
2019-06-24 19:16 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
0 siblings, 2 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-06-22 11:06 UTC (permalink / raw)
To: Pedro Alves; +Cc: Kevin Buettner, gdb-patches
* Pedro Alves <palves@redhat.com> [2019-06-21 14:43:26 +0100]:
> On 6/21/19 12:23 AM, Andrew Burgess wrote:
>
> > I spent some more time trying to find a path that would call both
> > 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
> > can't find one.
>
> But won't that change affect any code path that ends up in
> skip_prologue_sal with explicit_line set?
[ Disclaimer: In the below I'll take about 'in current testing we
never do X'. I understand that this doesn't mean GDB will
_never_ do X as our testing doesn't guarantee to hit every
possible code path, it's more an invitation for people to
suggest how me might create a test that does do X. ]
Indeed. My claim is that in the current testing we never get to
skip_prologue_sal with explicit_line set. My patch means we do now
enter skip_prologue_sal with explicit_line set, and I find that the
existing check that uses explicit_line means GDB doesn't behave as I'd
like.
Given that in HEAD explicit_line only seems to be set when decoding a
line spec in 'list_mode', my current belief is that explicit_line is
never set in a condition where the flag will then be checked. In
other words, I think the explicit_line is currently useless.
>
> E.g.:
>
> /* Helper function for break_command_1 and disassemble_command. */
>
> void
> resolve_sal_pc (struct symtab_and_line *sal)
> {
> CORE_ADDR pc;
>
> if (sal->pc == 0 && sal->symtab != NULL)
> {
> if (!find_line_pc (sal->symtab, sal->line, &pc))
> error (_("No line %d in file \"%s\"."),
> sal->line, symtab_to_filename_for_display (sal->symtab));
> sal->pc = pc;
>
> /* If this SAL corresponds to a breakpoint inserted using a line
> number, then skip the function prologue if necessary. */
> if (sal->explicit_line)
> skip_prologue_sal (sal);
> }
>
> Is that path unreachable today?
In current testing we enter this code block once, by accident.
The test 'gdb.dwarf2/dw2-objfile-overlap.exp' enters the block because
we load a symbol file at address 0. The check '(sal->pc == 0 &&
sal->symtab != NULL)' is trying to find SALs where the 'pc' has not
been set, in our case the 'pc' has been set; to zero. When we then
call 'find_line_pc' and then 'sal->pc = pc', we reset the 'pc' to zero
again.
In this one case the explicit_line flag is false, so skip_prologue_sal
is never called.
As an aside how would you feel about a patch that made the 'pc' field
of symtab_and_line private, and updated all users to use getter/setter
methods? I already did this in order to add a 'is_pc_initialised?'
type method to symtab_and_line. When I add this and change the above
code to say this:
void
resolve_sal_pc (struct symtab_and_line *sal)
{
CORE_ADDR pc;
if (sal->symtab != NULL && !sal->pc_p ())
{
// ... etc ...
then we no longer enter this block at all during the current testing.
>
>
> >
> > Looking back at how the explicit_line flag was originally used when
> > it was added in commit ed0616c6b78a0966, things have changed quite a
> > bit in the 10+ years since. There were some tests added along with
> > the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
> > master and in my patched branch.
> >
> > My current thinking is that the explicit_line flag was no longer doing
> > anything useful in master, but if someone disagrees I'd love to
> > understand more about this.
>
> I seem to recall that GDB didn't use to update a breakpoint's line
> number to advance to the next line number that includes some actual
> compiled code. Like if you set a breakpoint at line 10 below:
>
> 10 // just a comment
> 11 i++;
>
> you end up with a breakpoint at line 11. Maybe it's old code
> related to that.
I wonder if what you meant to say here is the breakpoint is placed at
the address of line 11, but is recorded as being at line 10. This
actually would line up with what the explicit line flag was doing if
the explicit line flag was being set.
The problem seems to be that when the explicit_line flag was first
added there was just function for decoding linespec line numbers
'decode_all_digits'. At some point in time this split into
decode_digits_ordinary and decode_digits_list_mode, when this happened
the explicit_line flag was only ever being set in one path.
I suspect that if the behaviour you discussed above ever existed, then
it was before the split in how digits were decoded.
I'm running out of time to investigate this today, but when I get some
more time I'll dig a little more on this line of enquiry to see if I
can confirm or deny the above theory.
>
> Maybe I'm misremembering.
>
> In any case, if you change this, then you also should change
> the function's entry comment:
>
> /* 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. */
> ^^^^^^^^^^
Would this be OK? (I'm not pushing anything until the above questions
are resolved):
diff --git a/gdb/symtab.c b/gdb/symtab.c
index c10e6b3e358..6e7e32fb4d8 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)
Thanks,
Andrew
>
> void
> skip_prologue_sal (struct symtab_and_line *sal)
> {
>
> Thanks,
> Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-22 11:06 ` Andrew Burgess
@ 2019-06-22 11:23 ` Andrew Burgess
2019-06-24 19:16 ` Pedro Alves
2019-06-24 19:16 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
1 sibling, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-06-22 11:23 UTC (permalink / raw)
To: Pedro Alves; +Cc: Kevin Buettner, gdb-patches
* Andrew Burgess <andrew.burgess@embecosm.com> [2019-06-22 12:05:58 +0100]:
> * Pedro Alves <palves@redhat.com> [2019-06-21 14:43:26 +0100]:
>
> > On 6/21/19 12:23 AM, Andrew Burgess wrote:
> >
> > > I spent some more time trying to find a path that would call both
> > > 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
> > > can't find one.
> >
> > But won't that change affect any code path that ends up in
> > skip_prologue_sal with explicit_line set?
>
> [ Disclaimer: In the below I'll take about 'in current testing we
> never do X'. I understand that this doesn't mean GDB will
> _never_ do X as our testing doesn't guarantee to hit every
> possible code path, it's more an invitation for people to
> suggest how me might create a test that does do X. ]
>
> Indeed. My claim is that in the current testing we never get to
> skip_prologue_sal with explicit_line set. My patch means we do now
> enter skip_prologue_sal with explicit_line set, and I find that the
> existing check that uses explicit_line means GDB doesn't behave as I'd
> like.
>
> Given that in HEAD explicit_line only seems to be set when decoding a
> line spec in 'list_mode', my current belief is that explicit_line is
> never set in a condition where the flag will then be checked. In
> other words, I think the explicit_line is currently useless.
>
> >
> > E.g.:
> >
> > /* Helper function for break_command_1 and disassemble_command. */
> >
> > void
> > resolve_sal_pc (struct symtab_and_line *sal)
> > {
> > CORE_ADDR pc;
> >
> > if (sal->pc == 0 && sal->symtab != NULL)
> > {
> > if (!find_line_pc (sal->symtab, sal->line, &pc))
> > error (_("No line %d in file \"%s\"."),
> > sal->line, symtab_to_filename_for_display (sal->symtab));
> > sal->pc = pc;
> >
> > /* If this SAL corresponds to a breakpoint inserted using a line
> > number, then skip the function prologue if necessary. */
> > if (sal->explicit_line)
> > skip_prologue_sal (sal);
> > }
> >
> > Is that path unreachable today?
>
> In current testing we enter this code block once, by accident.
>
> The test 'gdb.dwarf2/dw2-objfile-overlap.exp' enters the block because
> we load a symbol file at address 0. The check '(sal->pc == 0 &&
> sal->symtab != NULL)' is trying to find SALs where the 'pc' has not
> been set, in our case the 'pc' has been set; to zero. When we then
> call 'find_line_pc' and then 'sal->pc = pc', we reset the 'pc' to zero
> again.
>
> In this one case the explicit_line flag is false, so skip_prologue_sal
> is never called.
>
> As an aside how would you feel about a patch that made the 'pc' field
> of symtab_and_line private, and updated all users to use getter/setter
> methods? I already did this in order to add a 'is_pc_initialised?'
> type method to symtab_and_line. When I add this and change the above
> code to say this:
>
> void
> resolve_sal_pc (struct symtab_and_line *sal)
> {
> CORE_ADDR pc;
>
> if (sal->symtab != NULL && !sal->pc_p ())
> {
> // ... etc ...
>
> then we no longer enter this block at all during the current testing.
>
> >
> >
> > >
> > > Looking back at how the explicit_line flag was originally used when
> > > it was added in commit ed0616c6b78a0966, things have changed quite a
> > > bit in the 10+ years since. There were some tests added along with
> > > the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
> > > master and in my patched branch.
> > >
> > > My current thinking is that the explicit_line flag was no longer doing
> > > anything useful in master, but if someone disagrees I'd love to
> > > understand more about this.
> >
> > I seem to recall that GDB didn't use to update a breakpoint's line
> > number to advance to the next line number that includes some actual
> > compiled code. Like if you set a breakpoint at line 10 below:
> >
> > 10 // just a comment
> > 11 i++;
> >
> > you end up with a breakpoint at line 11. Maybe it's old code
> > related to that.
>
> I wonder if what you meant to say here is the breakpoint is placed at
> the address of line 11, but is recorded as being at line 10. This
> actually would line up with what the explicit line flag was doing if
> the explicit line flag was being set.
>
> The problem seems to be that when the explicit_line flag was first
> added there was just function for decoding linespec line numbers
> 'decode_all_digits'. At some point in time this split into
> decode_digits_ordinary and decode_digits_list_mode, when this happened
> the explicit_line flag was only ever being set in one path.
>
> I suspect that if the behaviour you discussed above ever existed, then
> it was before the split in how digits were decoded.
>
> I'm running out of time to investigate this today, but when I get some
> more time I'll dig a little more on this line of enquiry to see if I
> can confirm or deny the above theory.
The decode_all_digits function split into decode_digits_ordinary and
decode_digits_list_mode with commit f8eba3c61629b3c03a back in Dec
2011, I suspect that the behaviour you recall would have stopped
working then. Though I haven't confirmed this (building such old
versions of GDB is time consuming).
Related to what we're discussing seems to be commit a9e408182d2faaed5
from Jan 2018, where we appear to double down on having the breakpoint
file and line update to reflect where the breakpoint was placed, not
where the breakpoint ended up.
The question then would be if we can confirm GDB did used to behave
the way you recall some time ago, do we want to go back to that
behaviour now? And is that a blocker for my change going in? Yes if
we did want to go back to the old behaviour then part of my patch
would probably end up being reverted - as I suspect would
a9e408182d2faaed5. I'm happy to keep digging on this to see if I can
confirm if/when the behaviour changed if that helps bring clarity to
this issue.
Thanks,
Andrew
>
> >
> > Maybe I'm misremembering.
> >
> > In any case, if you change this, then you also should change
> > the function's entry comment:
> >
> > /* 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. */
> > ^^^^^^^^^^
>
> Would this be OK? (I'm not pushing anything until the above questions
> are resolved):
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index c10e6b3e358..6e7e32fb4d8 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)
>
>
>
> Thanks,
> Andrew
>
>
>
>
> >
> > void
> > skip_prologue_sal (struct symtab_and_line *sal)
> > {
> >
> > Thanks,
> > Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-22 11:06 ` Andrew Burgess
2019-06-22 11:23 ` Andrew Burgess
@ 2019-06-24 19:16 ` Pedro Alves
2019-07-01 17:12 ` Andrew Burgess
1 sibling, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2019-06-24 19:16 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Kevin Buettner, gdb-patches
On 6/22/19 12:05 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2019-06-21 14:43:26 +0100]:
>
>> On 6/21/19 12:23 AM, Andrew Burgess wrote:
>>
>>> I spent some more time trying to find a path that would call both
>>> 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
>>> can't find one.
>>
>> But won't that change affect any code path that ends up in
>> skip_prologue_sal with explicit_line set?
>
> [ Disclaimer: In the below I'll take about 'in current testing we
> never do X'. I understand that this doesn't mean GDB will
> _never_ do X as our testing doesn't guarantee to hit every
> possible code path, it's more an invitation for people to
> suggest how me might create a test that does do X. ]
>
> Indeed. My claim is that in the current testing we never get to
> skip_prologue_sal with explicit_line set. My patch means we do now
> enter skip_prologue_sal with explicit_line set, and I find that the
> existing check that uses explicit_line means GDB doesn't behave as I'd
> like.
>
> Given that in HEAD explicit_line only seems to be set when decoding a
> line spec in 'list_mode', my current belief is that explicit_line is
> never set in a condition where the flag will then be checked. In
> other words, I think the explicit_line is currently useless.
Since the flag is checked in breakpoint.c, it likely had a use
for breakpoint mode too, though as you say, it's probably not used today.
Greping for explicit_line finds this case:
set_breakpoint_location_function (loc,
sal->explicit_pc || sal->explicit_line);
in
add_location_to_breakpoint
I wonder whether that explicit_line use makes sense here.
set_breakpoint_location_function says:
/* Initialize loc->function_name. EXPLICIT_LOC says no indirect function
resolutions should be made as the user specified the location explicitly
enough. */
static void
set_breakpoint_location_function (struct bp_location *loc, int explicit_loc)
{
I'm not immediately seeing how to set a breakpoint at an ifunc symbol
by line number such that you end up in set_breakpoint_location_function
with loc->msymbol != NULL.
We should probably delete that sal->explicit_line reference.
>
>>
>> E.g.:
>>
>> /* Helper function for break_command_1 and disassemble_command. */
>>
>> void
>> resolve_sal_pc (struct symtab_and_line *sal)
>> {
>> CORE_ADDR pc;
>>
>> if (sal->pc == 0 && sal->symtab != NULL)
>> {
>> if (!find_line_pc (sal->symtab, sal->line, &pc))
>> error (_("No line %d in file \"%s\"."),
>> sal->line, symtab_to_filename_for_display (sal->symtab));
>> sal->pc = pc;
>>
>> /* If this SAL corresponds to a breakpoint inserted using a line
>> number, then skip the function prologue if necessary. */
>> if (sal->explicit_line)
>> skip_prologue_sal (sal);
>> }
>>
>> Is that path unreachable today?
>
> In current testing we enter this code block once, by accident.
>
> The test 'gdb.dwarf2/dw2-objfile-overlap.exp' enters the block because
> we load a symbol file at address 0. The check '(sal->pc == 0 &&
> sal->symtab != NULL)' is trying to find SALs where the 'pc' has not
> been set, in our case the 'pc' has been set; to zero. When we then
> call 'find_line_pc' and then 'sal->pc = pc', we reset the 'pc' to zero
> again.
>
> In this one case the explicit_line flag is false, so skip_prologue_sal
> is never called.
>
> As an aside how would you feel about a patch that made the 'pc' field
> of symtab_and_line private, and updated all users to use getter/setter
> methods?
Sounds fine to me in principle.
> I already did this in order to add a 'is_pc_initialised?'
> type method to symtab_and_line. When I add this and change the above
> code to say this:
>
> void
> resolve_sal_pc (struct symtab_and_line *sal)
> {
> CORE_ADDR pc;
>
> if (sal->symtab != NULL && !sal->pc_p ())
> {
> // ... etc ...
>
> then we no longer enter this block at all during the current testing.
So does this mean that linespec nowadays always fills in the PC then?
If that's the case, when do we need that is_pc_initialized method?
I wonder what else is not reachable around here, laying around
waiting to be garbage collected...
>>> Looking back at how the explicit_line flag was originally used when
>>> it was added in commit ed0616c6b78a0966, things have changed quite a
>>> bit in the 10+ years since. There were some tests added along with
>>> the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
>>> master and in my patched branch.
>>>
>>> My current thinking is that the explicit_line flag was no longer doing
>>> anything useful in master, but if someone disagrees I'd love to
>>> understand more about this.
>>
>> I seem to recall that GDB didn't use to update a breakpoint's line
>> number to advance to the next line number that includes some actual
>> compiled code. Like if you set a breakpoint at line 10 below:
>>
>> 10 // just a comment
>> 11 i++;
>>
>> you end up with a breakpoint at line 11. Maybe it's old code
>> related to that.
>
> I wonder if what you meant to say here is the breakpoint is placed at
> the address of line 11, but is recorded as being at line 10. This
> actually would line up with what the explicit line flag was doing if
> the explicit line flag was being set.
Yes, exactly.
>
> The problem seems to be that when the explicit_line flag was first
> added there was just function for decoding linespec line numbers
> 'decode_all_digits'. At some point in time this split into
> decode_digits_ordinary and decode_digits_list_mode, when this happened
> the explicit_line flag was only ever being set in one path.
>
> I suspect that if the behaviour you discussed above ever existed, then
> it was before the split in how digits were decoded.
>
> I'm running out of time to investigate this today, but when I get some
> more time I'll dig a little more on this line of enquiry to see if I
> can confirm or deny the above theory.
Did you check whether we're already setting explicit_line when
parsing "b -line N", i.e., when using the explicit locations syntax?
>
>>
>> Maybe I'm misremembering.
>>
>> In any case, if you change this, then you also should change
>> the function's entry comment:
>>
>> /* 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. */
>> ^^^^^^^^^^
>
> Would this be OK? (I'm not pushing anything until the above questions
> are resolved):
Sure.
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index c10e6b3e358..6e7e32fb4d8 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)
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-22 11:23 ` Andrew Burgess
@ 2019-06-24 19:16 ` Pedro Alves
2019-06-24 19:54 ` [PROTOTYPE] Make "info breakpoints" show breakpoint's specs (Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler) Pedro Alves
0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2019-06-24 19:16 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Kevin Buettner, gdb-patches
On 6/22/19 12:23 PM, Andrew Burgess wrote:
>> The problem seems to be that when the explicit_line flag was first
>> added there was just function for decoding linespec line numbers
>> 'decode_all_digits'. At some point in time this split into
>> decode_digits_ordinary and decode_digits_list_mode, when this happened
>> the explicit_line flag was only ever being set in one path.
>>
>> I suspect that if the behaviour you discussed above ever existed, then
>> it was before the split in how digits were decoded.
I'm like, 98.64% sure older gdbs behave like that. But I can't rule out
misremembering without trying an old gdb, which I don't have built handy
at the moment either.
>>
>> I'm running out of time to investigate this today, but when I get some
>> more time I'll dig a little more on this line of enquiry to see if I
>> can confirm or deny the above theory.
>
> The decode_all_digits function split into decode_digits_ordinary and
> decode_digits_list_mode with commit f8eba3c61629b3c03a back in Dec
> 2011, I suspect that the behaviour you recall would have stopped
> working then. Though I haven't confirmed this (building such old
> versions of GDB is time consuming).
Indeed.
>
> Related to what we're discussing seems to be commit a9e408182d2faaed5
> from Jan 2018, where we appear to double down on having the breakpoint
> file and line update to reflect where the breakpoint was placed, not
> where the breakpoint ended up.
Indeed. Thanks for digging that up.
>
> The question then would be if we can confirm GDB did used to behave
> the way you recall some time ago, do we want to go back to that
> behaviour now?
No, I don't think we want to go back.
I've thought for a few years that "info breakpoints" should show
BOTH the canonical spec behind each breakpoint, and the actual
location(s) where the breakpoint is inserted. It wouldn't
be that hard, even. I cooked up a prototype patch for that.
I'll post it as a follow up.
> And is that a blocker for my change going in? Yes if
> we did want to go back to the old behaviour then part of my patch
> would probably end up being reverted - as I suspect would
> a9e408182d2faaed5. I'm happy to keep digging on this to see if I can
> confirm if/when the behaviour changed if that helps bring clarity to
> this issue.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PROTOTYPE] Make "info breakpoints" show breakpoint's specs (Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler)
2019-06-24 19:16 ` Pedro Alves
@ 2019-06-24 19:54 ` Pedro Alves
2019-07-03 22:37 ` Pedro Alves
0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2019-06-24 19:54 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Kevin Buettner, gdb-patches
On 6/24/19 8:16 PM, Pedro Alves wrote:
>
> I've thought for a few years that "info breakpoints" should show
> BOTH the canonical spec behind each breakpoint, and the actual
> location(s) where the breakpoint is inserted. It wouldn't
> be that hard, even. I cooked up a prototype patch for that.
> I'll post it as a follow up.
Here's what I meant.
Here are for example 3 breakpoints that I set while debugging gdb:
(top-gdb) info breakpoints
1 breakpoint keep y 0x0000000000575127 in internal_error(char const*, int, char const*, ...) at src/gdb/common/errors.c:54
2 breakpoint keep y 0x0000000000575127 in internal_error(char const*, int, char const*, ...) at src/gdb/common/errors.c:54
3 breakpoint keep y <MULTIPLE>
3.1 y 0x0000000000575127 in internal_error(char const*, int, char const*, ...) at src/gdb/common/errors.c:54
3.2 y 0x00007ffff6d50410 in PyErr_SetObject at /usr/src/debug/python2-2.7.15-4.fc27.x86_64/Python/errors.c:54
(top-gdb)
From looking at those, you have no idea how I created the breakpoints in
the first place, which specs I used. The first two look like the same,
but really aren't. I don't recall which was which though.
And what's with the third, showing seemingly unrelated functions?
GDB of course knows the breakpoints were set differently. It needs to
remember the spec in order to re_set the locations properly. And it
needs to remember the spec in order to save the breakpoints, like:
(top-gdb) save breakpoints bpts.cmd
Saved to file 'bpts.cmd'.
Let's look at the file, see how the breakpoint had been created:
(top-gdb) shell cat bpts.cmd
break internal_error
break -qualified internal_error
break errors.c:54
(top-gdb)
Ah, see how the "-qualified" information was lost from
"info breakpoints" output? And how now it's obvious
why we have two locations for breakpoint 3?
Why not show this in "info breakpoints" too?
A way to do that would be always show separate lines for
breakpoint and locations, as if all breakpoints had multiple
locations, even if they only have one location. The "What"
column for the owner breakpoint could then show the canonical
location string:
(top-gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y internal_error
1.1 y 0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
2 breakpoint keep y -qualified internal_error
2.1 y 0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
3 breakpoint keep y errors.c:54
3.1 y 0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
3.2 y 0x00007ffff6d50410 in PyErr_SetObject at /usr/src/debug/python2-2.7.15-4.fc27.x86_64/Python/errors.c:54
Breakpoints would no longer move from single-location to multiple-location
"display modes" as location were added/removed (e.g., when a shared library
is loaded), they'd always be in multi-location display mode.
Note we currently have a special case when we show the location of
a breakpoint with a single location separately, it's when a breakpoint
has a single location that is disabled. The above gets rid of the
special case, by always applying the special case, you could say. :-)
Here's what it would look like in that case we're discussing,
where the user sets a breapoint on a comment or empty line:
(top-gdb) b 27
Breakpoint 4 at 0x469aa6: file /home/pedro/gdb/mygit/src/gdb/gdb.c, line 28.
(top-gdb) info breakpoints 4
Num Type Disp Enb Address What
4 breakpoint keep y /home/pedro/gdb/mygit/src/gdb/gdb.c:27
4.1 y 0x0000000000469aa6 in main(int, char**) at /home/pedro/gdb/mygit/src/gdb/gdb.c:28
Note how GDB remembers line 27, but we don't currently show it, unlike
in the snippet above with patched gdb.
If we had this, then I think it would also make sense to
add a way to list breakpoints and skip showing the locations, for
a more condensed, higher level view of the breakpoints.
E.g., try "b main" while debugging gdb. You'll end up with
24 locations...:
(top-gdb) b main
Breakpoint 3 at 0x469aa6: main. (24 locations)
(top-gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y internal_error
1.1 y 0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
2 breakpoint keep y info_command
silent
return
2.1 y 0x0000000000545204 in info_command(char const*, int) at /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c:201
3 breakpoint keep y main
3.1 y 0x0000000000469aa6 in main(int, char**) at /home/pedro/gdb/mygit/src/gdb/gdb.c:28
3.2 y 0x00000000009875fa in selftests::string_view::capacity_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/capacity/1.cc:167
3.3 y 0x00000000009879ac in selftests::string_view::cons_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/cons/char/1.cc:62
3.4 y 0x0000000000987a67 in selftests::string_view::cons_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/cons/char/2.cc:41
3.5 y 0x0000000000987aa2 in selftests::string_view::cons_3::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/cons/char/3.cc:34
3.6 y 0x0000000000987c2b in selftests::string_view::element_access_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/element_access/char/1.cc:66
3.7 y 0x0000000000987c3f in selftests::string_view::element_access_empty::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/element_access/char/empty.cc:27
3.8 y 0x0000000000987de3 in selftests::string_view::element_access_front_back::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/element_access/char/front_back.cc:38
3.9 y 0x000000000098819b in selftests::string_view::inserters_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/inserters/char/2.cc:84
3.10 y 0x00000000009882cf in selftests::string_view::modifiers_remove_prefix::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/modifiers/remove_prefix/char/1.cc:58
3.11 y 0x00000000009883e0 in selftests::string_view::modifiers_remove_suffix::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/modifiers/remove_suffix/char/1.cc:58
3.12 y 0x0000000000988b99 in selftests::string_view::operations_compare_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/compare/char/1.cc:127
3.13 y 0x0000000000988cc6 in selftests::string_view::operations_compare_13650::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/compare/char/13650.cc:45
3.14 y 0x0000000000988d82 in selftests::string_view::operations_copy_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/copy/char/1.cc:41
3.15 y 0x0000000000988e20 in selftests::string_view::operations_data_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/data/char/1.cc:39
3.16 y 0x0000000000989370 in selftests::string_view::operations_find_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/1.cc:160
3.17 y 0x0000000000989861 in selftests::string_view::operations_find_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/2.cc:158
3.18 y 0x0000000000989e43 in selftests::string_view::operations_find_3::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/3.cc:158
3.19 y 0x0000000000989ebb in selftests::string_view::operations_find_4::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/4.cc:40
3.20 y 0x000000000098a41b in selftests::string_view::operations_rfind_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/rfind/char/1.cc:90
3.21 y 0x000000000098a623 in selftests::string_view::operations_rfind_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/rfind/char/2.cc:48
3.22 y 0x000000000098a970 in selftests::string_view::operations_rfind_3::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/rfind/char/3.cc:62
3.23 y 0x000000000098ac52 in selftests::string_view::operations_substr_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/substr/char/1.cc:74
3.24 y 0x000000000098cd7a in selftests::string_view::operators_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operators/char/2.cc:366
With a "-hide-locations" switch, a user could do this to get the locations
out of the way:
(top-gdb) info breakpoints -hide-locations
Num Type Disp Enb Address What
1 breakpoint keep y internal_error
2 breakpoint keep y info_command
silent
return
3 breakpoint keep y main
(We could have switches for only showing enabled or disabled breakpoints
too, btw.)
Below's the prototype patch.
I didn't think about what to do with watchpoints, catchpoints,
etc. Currently they show like an unpatched gdb:
3 catchpoint keep y syscall "<any syscall>"
4 hw watchpoint keep y *main
I did notice that the patch currently crashes with catchpoints that
are implemented via breakpoints, for example, and possibly
other types:
(top-gdb) catch throw
Catchpoint 5 (throw)
(top-gdb) info breakpoints
5 breakpoint keep y ../../gdb/breakpoint.c:6387: internal-error: void print_one_breakpoint_location(breakpoint*, bp_location*, int, bp_location**, int): Assertion `b->loc == NULL || b->loc->next == NULL' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)
whoops. :-) I did say this was a prototype, right? :-)
From 8087acd6926d1054979ed42076a9bb0b28c4551e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 22 Jun 2019 21:10:58 +0100
Subject: [PATCH] bkpts
---
gdb/breakpoint.c | 114 ++++++++++++++++++++++++++++++++++++++-------------
gdb/cli/cli-option.c | 12 +++++-
gdb/cli/cli-option.h | 6 +++
3 files changed, 102 insertions(+), 30 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8422db8b571..d28c4736d43 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5973,6 +5973,33 @@ output_thread_groups (struct ui_out *uiout,
}
}
+/* The options for the "info breakpoints" command. */
+
+struct info_breakpoints_opts
+{
+ /* For "-hide-locations". */
+ int hide_locations = 0;
+};
+
+static const gdb::option::option_def info_breakpoints_option_defs[] = {
+
+ gdb::option::flag_option_def<info_breakpoints_opts> {
+ "hide-locations",
+ [] (info_breakpoints_opts *opts) { return &opts->hide_locations; },
+ N_("Hide breakpoint locations."),
+ },
+
+};
+
+/* Create an option_def_group for the "info breakpoints" options, with
+ OPTS as context. */
+
+static inline gdb::option::option_def_group
+make_info_breakpoints_options_def_group (info_breakpoints_opts *opts)
+{
+ return {{info_breakpoints_option_defs}, opts};
+}
+
/* Print B to gdb_stdout. */
static void
@@ -5993,14 +6020,11 @@ print_one_breakpoint_location (struct breakpoint *b,
get_user_print_options (&opts);
gdb_assert (!loc || loc_number != 0);
- /* See comment in print_one_breakpoint concerning treatment of
- breakpoints with single disabled location. */
- if (loc == NULL
- && (b->loc != NULL
- && (b->loc->next != NULL || !b->loc->enabled)))
- header_of_multiple = 1;
if (loc == NULL)
- loc = b->loc;
+ {
+ header_of_multiple = 1;
+ loc = b->loc;
+ }
annotate_record ();
@@ -6098,7 +6122,7 @@ print_one_breakpoint_location (struct breakpoint *b,
{
annotate_field (4);
if (header_of_multiple)
- uiout->field_string ("addr", "<MULTIPLE>");
+ uiout->field_skip ("addr");
else if (b->loc == NULL || loc->shlib_disabled)
uiout->field_string ("addr", "<PENDING>");
else
@@ -6106,7 +6130,9 @@ print_one_breakpoint_location (struct breakpoint *b,
loc->gdbarch, loc->address);
}
annotate_field (5);
- if (!header_of_multiple)
+ if (header_of_multiple)
+ uiout->field_string ("what", event_location_to_string (b->location.get ()));
+ else
print_breakpoint_location (b, loc);
if (b->loc)
*last_loc = b->loc;
@@ -6333,7 +6359,8 @@ bool fix_multi_location_breakpoint_output_globally = false;
static void
print_one_breakpoint (struct breakpoint *b,
struct bp_location **last_loc,
- int allflag)
+ int allflag,
+ const info_breakpoints_opts &ib_opts)
{
struct ui_out *uiout = current_uiout;
bool use_fixed_output
@@ -6348,21 +6375,15 @@ print_one_breakpoint (struct breakpoint *b,
if (!use_fixed_output)
bkpt_tuple_emitter.reset ();
- /* If this breakpoint has custom print function,
- it's already printed. Otherwise, print individual
- locations, if any. */
- if (b->ops == NULL || b->ops->print_one == NULL)
+ /* If this breakpoint has a custom print function, it's already
+ printed. Otherwise, print individual locations, if any, and if
+ not explicitly disabled by the user. */
+ if ((b->ops == NULL || b->ops->print_one == NULL)
+ && !ib_opts.hide_locations)
{
- /* If breakpoint has a single location that is disabled, we
- print it as if it had several locations, since otherwise it's
- hard to represent "breakpoint enabled, location disabled"
- situation.
-
- Note that while hardware watchpoints have several locations
- internally, that's not a property exposed to user. */
- if (b->loc
- && !is_hardware_watchpoint (b)
- && (b->loc->next || !b->loc->enabled))
+ /* While hardware watchpoints have several locations internally,
+ that's not a property exposed to the user. */
+ if (!is_hardware_watchpoint (b))
{
gdb::optional<ui_out_emit_list> locations_list;
@@ -6410,8 +6431,9 @@ breakpoint_address_bits (struct breakpoint *b)
void
print_breakpoint (breakpoint *b)
{
+ info_breakpoints_opts ib_opts;
struct bp_location *dummy_loc = NULL;
- print_one_breakpoint (b, &dummy_loc, 0);
+ print_one_breakpoint (b, &dummy_loc, 0, ib_opts);
}
/* Return true if this breakpoint was set by the user, false if it is
@@ -6452,6 +6474,17 @@ breakpoint_1 (const char *args, int allflag,
get_user_print_options (&opts);
+ info_breakpoints_opts ib_opts;
+
+ auto grp = make_info_breakpoints_options_def_group (&ib_opts);
+
+ gdb::option::process_options
+ (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
+
+ if (args != nullptr
+ && args[0] == '-' && (!allflag || !isdigit (args[1])))
+ gdb::option::error_unrecognized_option_at (args);
+
/* Compute the number of rows in the table, as well as the size
required for address fields. */
nr_printable_breakpoints = 0;
@@ -6549,7 +6582,7 @@ breakpoint_1 (const char *args, int allflag,
/* We only print out user settable breakpoints unless the
allflag is set. */
if (allflag || user_breakpoint_p (b))
- print_one_breakpoint (b, &last_loc, allflag);
+ print_one_breakpoint (b, &last_loc, allflag, ib_opts);
}
}
@@ -6579,6 +6612,29 @@ breakpoint_1 (const char *args, int allflag,
return nr_printable_breakpoints;
}
+/* Completer for the "info breakpoints" command. */
+
+static void
+info_breakpoints_command_completer (struct cmd_list_element *ignore,
+ completion_tracker &tracker,
+ const char *text, const char *word_ignored)
+{
+ const auto grp = make_info_breakpoints_options_def_group (nullptr);
+
+ if (gdb::option::complete_options
+ (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp))
+ return;
+
+ /* Convenience to let the user know what the command can accept. */
+ if (*text == '\0')
+ {
+ gdb::option::complete_on_all_options (tracker, grp);
+ /* Keep this "ID" in sync with what "help info breakpoints"
+ says. */
+ tracker.add_completion (make_unique_xstrdup ("ID"));
+ }
+}
+
/* Display the value of default-collect in a way that is generally
compatible with the breakpoint list. */
@@ -15611,7 +15667,7 @@ Convenience variable \"$bpnum\" contains the number of the last\n\
breakpoint set."));
}
- add_info ("breakpoints", info_breakpoints_command, _("\
+ c = add_info ("breakpoints", info_breakpoints_command, _("\
Status of specified breakpoints (all user-settable breakpoints if no argument).\n\
The \"Type\" column indicates one of:\n\
\tbreakpoint - normal breakpoint\n\
@@ -15626,10 +15682,11 @@ are set to the address of the last breakpoint listed unless the command\n\
is prefixed with \"server \".\n\n\
Convenience variable \"$bpnum\" contains the number of the last\n\
breakpoint set."));
+ set_cmd_completer_handle_brkchars (c, info_breakpoints_command_completer);
add_info_alias ("b", "breakpoints", 1);
- add_cmd ("breakpoints", class_maintenance, maintenance_info_breakpoints, _("\
+ c = add_cmd ("breakpoints", class_maintenance, maintenance_info_breakpoints, _("\
Status of all breakpoints, or breakpoint number NUMBER.\n\
The \"Type\" column indicates one of:\n\
\tbreakpoint - normal breakpoint\n\
@@ -15649,6 +15706,7 @@ is prefixed with \"server \".\n\n\
Convenience variable \"$bpnum\" contains the number of the last\n\
breakpoint set."),
&maintenanceinfolist);
+ set_cmd_completer_handle_brkchars (c, info_breakpoints_command_completer);
add_prefix_cmd ("catch", class_breakpoint, catch_command, _("\
Set catchpoints to catch events."),
diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
index 9a53ec0592d..d43edb78641 100644
--- a/gdb/cli/cli-option.c
+++ b/gdb/cli/cli-option.c
@@ -121,6 +121,14 @@ complete_on_all_options (completion_tracker &tracker,
complete_on_options (options_group, tracker, opt + 1, opt);
}
+/* See cli-option.h. */
+
+void
+error_unrecognized_option_at (const char *at)
+{
+ error (_("Unrecognized option at: %s"), at);
+}
+
/* Parse ARGS, guided by OPTIONS_GROUP. HAVE_DELIMITER is true if the
whole ARGS line included the "--" options-terminator delimiter. */
@@ -136,7 +144,7 @@ parse_option (gdb::array_view<const option_def_group> options_group,
else if (**args != '-')
{
if (have_delimiter)
- error (_("Unrecognized option at: %s"), *args);
+ error_unrecognized_option_at (*args);
return {};
}
else if (check_for_argument (args, "--"))
@@ -182,7 +190,7 @@ parse_option (gdb::array_view<const option_def_group> options_group,
if (match == nullptr)
{
if (have_delimiter || mode != PROCESS_OPTIONS_UNKNOWN_IS_OPERAND)
- error (_("Unrecognized option at: %s"), *args);
+ error_unrecognized_option_at (*args);
return {};
}
diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
index 1bfbfce1ce5..06f12751c36 100644
--- a/gdb/cli/cli-option.h
+++ b/gdb/cli/cli-option.h
@@ -314,6 +314,12 @@ extern void
complete_on_all_options (completion_tracker &tracker,
gdb::array_view<const option_def_group> options_group);
+/* Throw an error indicating an unrecognized option was detected at
+ AT. Use this in conjunction with UNKNOWN_IS_OPERAND instead of
+ UNKNOWN_IS_ERROR when the operand may or may not begin with '-'
+ depending on some condition determined at run time. */
+extern void error_unrecognized_option_at (const char *at);
+
/* Return a string with the result of replacing %OPTIONS% in HELP_TMLP
with an auto-generated "help" string fragment for all the options
in OPTIONS_GROUP. */
--
2.14.5
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-06-24 19:16 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
@ 2019-07-01 17:12 ` Andrew Burgess
2019-07-01 18:03 ` [PATCHv2 2/2] " Andrew Burgess
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-07-01 17:12 UTC (permalink / raw)
To: Pedro Alves; +Cc: Kevin Buettner, gdb-patches
* Pedro Alves <palves@redhat.com> [2019-06-24 20:16:23 +0100]:
> On 6/22/19 12:05 PM, Andrew Burgess wrote:
> > * Pedro Alves <palves@redhat.com> [2019-06-21 14:43:26 +0100]:
> >
> >> On 6/21/19 12:23 AM, Andrew Burgess wrote:
> >>
> >>> I spent some more time trying to find a path that would call both
> >>> 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
> >>> can't find one.
> >>
> >> But won't that change affect any code path that ends up in
> >> skip_prologue_sal with explicit_line set?
> >
> > [ Disclaimer: In the below I'll take about 'in current testing we
> > never do X'. I understand that this doesn't mean GDB will
> > _never_ do X as our testing doesn't guarantee to hit every
> > possible code path, it's more an invitation for people to
> > suggest how me might create a test that does do X. ]
> >
> > Indeed. My claim is that in the current testing we never get to
> > skip_prologue_sal with explicit_line set. My patch means we do now
> > enter skip_prologue_sal with explicit_line set, and I find that the
> > existing check that uses explicit_line means GDB doesn't behave as I'd
> > like.
> >
> > Given that in HEAD explicit_line only seems to be set when decoding a
> > line spec in 'list_mode', my current belief is that explicit_line is
> > never set in a condition where the flag will then be checked. In
> > other words, I think the explicit_line is currently useless.
>
> Since the flag is checked in breakpoint.c, it likely had a use
> for breakpoint mode too, though as you say, it's probably not used today.
>
> Greping for explicit_line finds this case:
>
>
> set_breakpoint_location_function (loc,
> sal->explicit_pc || sal->explicit_line);
>
> in
>
> add_location_to_breakpoint
>
> I wonder whether that explicit_line use makes sense here.
>
> set_breakpoint_location_function says:
>
> /* Initialize loc->function_name. EXPLICIT_LOC says no indirect function
> resolutions should be made as the user specified the location explicitly
> enough. */
>
> static void
> set_breakpoint_location_function (struct bp_location *loc, int explicit_loc)
> {
>
>
> I'm not immediately seeing how to set a breakpoint at an ifunc symbol
> by line number such that you end up in set_breakpoint_location_function
> with loc->msymbol != NULL.
We certainly never get into set_breakpoint_location_function with
explicit_line set in current HEAD.
The explicit_loc parameter was added in commit 0e30163f127122 (which
was really about adding or improving ifunc support) and the tests were
in commit e462023046d892.
Out of interest I ran the testsuite checking to see if:
loc->msymbol != NULL && explicit_loc
was ever true (in current HEAD) and it never is. I suspect this is
because loc->msymbol is initialised from sal->msymbol, which is only
set in one place linespec.c:minsym_found, which I don't think allows
for the address and/or line number to be explicitly stated.
After this investigation I propose that the explicit_loc parameter be
removed completely from set_breakpoint_location_function - I've
included this in an updated series that I'll post shortly
>
> We should probably delete that sal->explicit_line reference.
Agreed.
>
> >
> >>
> >> E.g.:
> >>
> >> /* Helper function for break_command_1 and disassemble_command. */
> >>
> >> void
> >> resolve_sal_pc (struct symtab_and_line *sal)
> >> {
> >> CORE_ADDR pc;
> >>
> >> if (sal->pc == 0 && sal->symtab != NULL)
> >> {
> >> if (!find_line_pc (sal->symtab, sal->line, &pc))
> >> error (_("No line %d in file \"%s\"."),
> >> sal->line, symtab_to_filename_for_display (sal->symtab));
> >> sal->pc = pc;
> >>
> >> /* If this SAL corresponds to a breakpoint inserted using a line
> >> number, then skip the function prologue if necessary. */
> >> if (sal->explicit_line)
> >> skip_prologue_sal (sal);
> >> }
> >>
> >> Is that path unreachable today?
> >
> > In current testing we enter this code block once, by accident.
> >
> > The test 'gdb.dwarf2/dw2-objfile-overlap.exp' enters the block because
> > we load a symbol file at address 0. The check '(sal->pc == 0 &&
> > sal->symtab != NULL)' is trying to find SALs where the 'pc' has not
> > been set, in our case the 'pc' has been set; to zero. When we then
> > call 'find_line_pc' and then 'sal->pc = pc', we reset the 'pc' to zero
> > again.
> >
> > In this one case the explicit_line flag is false, so skip_prologue_sal
> > is never called.
> >
> > As an aside how would you feel about a patch that made the 'pc' field
> > of symtab_and_line private, and updated all users to use getter/setter
> > methods?
>
> Sounds fine to me in principle.
I'll do this as a separate patch.
>
> > I already did this in order to add a 'is_pc_initialised?'
> > type method to symtab_and_line. When I add this and change the above
> > code to say this:
> >
> > void
> > resolve_sal_pc (struct symtab_and_line *sal)
> > {
> > CORE_ADDR pc;
> >
> > if (sal->symtab != NULL && !sal->pc_p ())
> > {
> > // ... etc ...
> >
> > then we no longer enter this block at all during the current testing.
>
> So does this mean that linespec nowadays always fills in the PC
> then?
This is my claim.
>
> If that's the case, when do we need that is_pc_initialized method?
I imagine it would only be used within an assertion.
>
> I wonder what else is not reachable around here, laying around
> waiting to be garbage collected...
Indeed...
>
> >>> Looking back at how the explicit_line flag was originally used when
> >>> it was added in commit ed0616c6b78a0966, things have changed quite a
> >>> bit in the 10+ years since. There were some tests added along with
> >>> the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
> >>> master and in my patched branch.
> >>>
> >>> My current thinking is that the explicit_line flag was no longer doing
> >>> anything useful in master, but if someone disagrees I'd love to
> >>> understand more about this.
> >>
> >> I seem to recall that GDB didn't use to update a breakpoint's line
> >> number to advance to the next line number that includes some actual
> >> compiled code. Like if you set a breakpoint at line 10 below:
> >>
> >> 10 // just a comment
> >> 11 i++;
> >>
> >> you end up with a breakpoint at line 11. Maybe it's old code
> >> related to that.
> >
> > I wonder if what you meant to say here is the breakpoint is placed at
> > the address of line 11, but is recorded as being at line 10. This
> > actually would line up with what the explicit line flag was doing if
> > the explicit line flag was being set.
>
> Yes, exactly.
>
> >
> > The problem seems to be that when the explicit_line flag was first
> > added there was just function for decoding linespec line numbers
> > 'decode_all_digits'. At some point in time this split into
> > decode_digits_ordinary and decode_digits_list_mode, when this happened
> > the explicit_line flag was only ever being set in one path.
> >
> > I suspect that if the behaviour you discussed above ever existed, then
> > it was before the split in how digits were decoded.
> >
> > I'm running out of time to investigate this today, but when I get some
> > more time I'll dig a little more on this line of enquiry to see if I
> > can confirm or deny the above theory.
>
> Did you check whether we're already setting explicit_line when
> parsing "b -line N", i.e., when using the explicit locations syntax?
In current HEAD explicit_line will only get set for the clear, edit,
list, and 'info line' commands. Any variation of setting breakpoints
will never set explicit_line.
I'll post an updated series very shortly.
Thanks,
Andrew
>
> >
> >>
> >> Maybe I'm misremembering.
> >>
> >> In any case, if you change this, then you also should change
> >> the function's entry comment:
> >>
> >> /* 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. */
> >> ^^^^^^^^^^
> >
> > Would this be OK? (I'm not pushing anything until the above questions
> > are resolved):
>
> Sure.
>
> >
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index c10e6b3e358..6e7e32fb4d8 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)
> Thanks,
> Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 0/2] Changes for explicit_line tracking, and prologue skipping
2019-07-01 17:12 ` Andrew Burgess
2019-07-01 18:03 ` [PATCHv2 2/2] " Andrew Burgess
2019-07-01 18:03 ` [PATCHv2 1/2] gdb: Remove unneeded parameter from set_breakpoint_location_function Andrew Burgess
@ 2019-07-01 18:03 ` Andrew Burgess
2019-07-01 18:21 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
3 siblings, 0 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-07-01 18:03 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Changes since v1:
- There's now two patches, with #1 being a new clean up change,
- Patch #2 has an additional updated comment.
Thanks,
Andrew
---
Andrew Burgess (2):
gdb: Remove unneeded parameter from set_breakpoint_location_function
gdb: Don't skip prologue for explicit line breakpoints in assembler
gdb/ChangeLog | 18 +++++++++++
gdb/breakpoint.c | 14 ++++-----
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 ++++++++++++++++++++++
7 files changed, 113 insertions(+), 18 deletions(-)
create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp
--
2.14.5
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 1/2] gdb: Remove unneeded parameter from set_breakpoint_location_function
2019-07-01 17:12 ` Andrew Burgess
2019-07-01 18:03 ` [PATCHv2 2/2] " Andrew Burgess
@ 2019-07-01 18:03 ` Andrew Burgess
2019-07-03 22:13 ` Pedro Alves
2019-07-01 18:03 ` [PATCHv2 0/2] Changes for explicit_line tracking, and prologue skipping Andrew Burgess
2019-07-01 18:21 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
3 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-07-01 18:03 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The explicit_loc parameter in set_breakpoint_location_function is not
useful. This parameter is set from two possible fields of the
symtab_and_line used to create the breakpoint; the explicit_pc field,
and the explicit_line field.
First, the explicit_line field, this is not currently set for any
breakpoint command, so will never be true.
Next, the explicit_pc field. This can be true but will never be true
at the same time that the sal->msymbol field is also true - the
sal->msymbol is only ever set in linespec.c:minsym_found, which
doesn't allow for explicitly setting the pc.
The result of this is that if we are setting a breakpoint on an
msymbol that could turn out to be an ifunc, then we will not also have
either an explicit_pc or an explicit_line, this check can therefore be
removed.
There should be no user visible changes after this commit.
gdb/ChangeLog:
* breakpoint.c (set_breakpoint_location_function): Remove
explicit_loc parameter.
(momentary_breakpoint_from_master): Update call to
set_breakpoint_location_function.
(add_location_to_breakpoint): Likewise.
---
gdb/ChangeLog | 8 ++++++++
gdb/breakpoint.c | 14 +++++---------
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8422db8b571..9baef38a3de 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7096,12 +7096,10 @@ set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
return add_to_breakpoint_chain (std::move (b));
}
-/* Initialize loc->function_name. EXPLICIT_LOC says no indirect function
- resolutions should be made as the user specified the location explicitly
- enough. */
+/* Initialize loc->function_name. */
static void
-set_breakpoint_location_function (struct bp_location *loc, int explicit_loc)
+set_breakpoint_location_function (struct bp_location *loc)
{
gdb_assert (loc->owner != NULL);
@@ -7113,8 +7111,7 @@ set_breakpoint_location_function (struct bp_location *loc, int explicit_loc)
if (loc->msymbol != NULL
&& (MSYMBOL_TYPE (loc->msymbol) == mst_text_gnu_ifunc
- || MSYMBOL_TYPE (loc->msymbol) == mst_data_gnu_ifunc)
- && !explicit_loc)
+ || MSYMBOL_TYPE (loc->msymbol) == mst_data_gnu_ifunc))
{
struct breakpoint *b = loc->owner;
@@ -8482,7 +8479,7 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
copy = set_raw_breakpoint_without_location (orig->gdbarch, type, ops);
copy->loc = allocate_bp_location (copy);
- set_breakpoint_location_function (copy->loc, 1);
+ set_breakpoint_location_function (copy->loc);
copy->loc->gdbarch = orig->loc->gdbarch;
copy->loc->requested_address = orig->loc->requested_address;
@@ -8587,8 +8584,7 @@ add_location_to_breakpoint (struct breakpoint *b,
loc->msymbol = sal->msymbol;
loc->objfile = sal->objfile;
- set_breakpoint_location_function (loc,
- sal->explicit_pc || sal->explicit_line);
+ set_breakpoint_location_function (loc);
/* While by definition, permanent breakpoints are already present in the
code, we don't mark the location as inserted. Normally one would expect
--
2.14.5
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 2/2] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-07-01 17:12 ` Andrew Burgess
@ 2019-07-01 18:03 ` Andrew Burgess
2019-07-03 22:13 ` Pedro Alves
2019-07-01 18:03 ` [PATCHv2 1/2] gdb: Remove unneeded parameter from set_breakpoint_location_function Andrew Burgess
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-07-01 18:03 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
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.
---
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(-)
create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 8db3924906c..83468f83ed9 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 4920d94a247..6e7e32fb4d8 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/gdb.arch/amd64-break-on-asm-line.S b/gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
new file mode 100644
index 00000000000..698c3e6d624
--- /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 place 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 00000000000..6218ce541bd
--- /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\\. \\*/.*"
--
2.14.5
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-07-01 17:12 ` Andrew Burgess
` (2 preceding siblings ...)
2019-07-01 18:03 ` [PATCHv2 0/2] Changes for explicit_line tracking, and prologue skipping Andrew Burgess
@ 2019-07-01 18:21 ` Pedro Alves
2019-07-02 8:28 ` Andrew Burgess
3 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2019-07-01 18:21 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Kevin Buettner, gdb-patches
On 7/1/19 6:12 PM, Andrew Burgess wrote:
>> Did you check whether we're already setting explicit_line when
>> parsing "b -line N", i.e., when using the explicit locations syntax?
> In current HEAD explicit_line will only get set for the clear, edit,
> list, and 'info line' commands. Any variation of setting breakpoints
> will never set explicit_line.
OK, but I was also curious to know whether your patch already handles that
case, or whether we need to set explicit_line somewhere else too.
Maybe it already works if we end up in decode_digits_ordinary too
with the explicit syntax.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-07-01 18:21 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
@ 2019-07-02 8:28 ` Andrew Burgess
0 siblings, 0 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-07-02 8:28 UTC (permalink / raw)
To: Pedro Alves; +Cc: Kevin Buettner, gdb-patches
* Pedro Alves <palves@redhat.com> [2019-07-01 19:21:43 +0100]:
> On 7/1/19 6:12 PM, Andrew Burgess wrote:
> >> Did you check whether we're already setting explicit_line when
> >> parsing "b -line N", i.e., when using the explicit locations syntax?
> > In current HEAD explicit_line will only get set for the clear, edit,
> > list, and 'info line' commands. Any variation of setting breakpoints
> > will never set explicit_line.
>
> OK, but I was also curious to know whether your patch already handles that
> case, or whether we need to set explicit_line somewhere else too.
> Maybe it already works if we end up in decode_digits_ordinary too
> with the explicit syntax.
Sorry I misunderstood your question.
Yes, if I use 'break -line N' then I do end up in
decode_digits_ordinary, so explicit_line will be set after my patch.
Thanks,
Andrew
>
> Thanks,
> Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 2/2] gdb: Don't skip prologue for explicit line breakpoints in assembler
2019-07-01 18:03 ` [PATCHv2 2/2] " Andrew Burgess
@ 2019-07-03 22:13 ` Pedro Alves
0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2019-07-03 22:13 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
LGTM. A couple nits below.
On 7/1/19 7:03 PM, Andrew Burgess wrote:
> + This file is part of the gdb testsuite.
> +
> + Test that a breakpoint place by line number in an assembler file
"placed", I believe.
> + 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. */
spaces vs tabs.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 1/2] gdb: Remove unneeded parameter from set_breakpoint_location_function
2019-07-01 18:03 ` [PATCHv2 1/2] gdb: Remove unneeded parameter from set_breakpoint_location_function Andrew Burgess
@ 2019-07-03 22:13 ` Pedro Alves
0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2019-07-03 22:13 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 7/1/19 7:02 PM, Andrew Burgess wrote:
> The explicit_loc parameter in set_breakpoint_location_function is not
> useful. This parameter is set from two possible fields of the
> symtab_and_line used to create the breakpoint; the explicit_pc field,
> and the explicit_line field.
>
> First, the explicit_line field, this is not currently set for any
> breakpoint command, so will never be true.
>
> Next, the explicit_pc field. This can be true but will never be true
> at the same time that the sal->msymbol field is also true - the
> sal->msymbol is only ever set in linespec.c:minsym_found, which
> doesn't allow for explicitly setting the pc.
>
> The result of this is that if we are setting a breakpoint on an
> msymbol that could turn out to be an ifunc, then we will not also have
> either an explicit_pc or an explicit_line, this check can therefore be
> removed.
>
> There should be no user visible changes after this commit.
>
> gdb/ChangeLog:
>
> * breakpoint.c (set_breakpoint_location_function): Remove
> explicit_loc parameter.
> (momentary_breakpoint_from_master): Update call to
> set_breakpoint_location_function.
> (add_location_to_breakpoint): Likewise.
OK.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PROTOTYPE] Make "info breakpoints" show breakpoint's specs (Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler)
2019-06-24 19:54 ` [PROTOTYPE] Make "info breakpoints" show breakpoint's specs (Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler) Pedro Alves
@ 2019-07-03 22:37 ` Pedro Alves
0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2019-07-03 22:37 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Kevin Buettner, gdb-patches
On 6/24/19 8:54 PM, Pedro Alves wrote:
> I did notice that the patch currently crashes with catchpoints that
> are implemented via breakpoints, for example, and possibly
> other types:
>
> (top-gdb) catch throw
> Catchpoint 5 (throw)
> (top-gdb) info breakpoints
> 5 breakpoint keep y ../../gdb/breakpoint.c:6387: internal-error: void print_one_breakpoint_location(breakpoint*, bp_location*, int, bp_location**, int): Assertion `b->loc == NULL || b->loc->next == NULL' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
>
> whoops. :-) I did say this was a prototype, right? :-)
Actually, turns out this is a preexisting problem.
GDB is not expecting to find more than one location for a catchpoint
backed by breakpoints. But it is now finding two locations.
The breakpoint is set at "__cxa_begin_catch", and if we do that manually,
we see what are those locations:
(top-gdb) b __cxa_begin_catch
Breakpoint 5 at 0xb126c0 (2 locations)
(top-gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <MULTIPLE>
1.1 y 0x0000000000b126c0 <__cxa_begin_catch>
1.2 y 0x00007ffff2f4ddb0 in __cxxabiv1::__cxa_begin_catch(void*) at ../../../../libstdc++-v3/libsupc++/eh_catch.cc:41
Looks like a regression caused by the -qualified stuff.
I guess we want a breakpoint at <__cxa_begin_catch> only.
Should be easy to fix.
But I also suggest that "info breakpoints" shouldn't be showing any
address for "catch catch" breakpoints (and catch throw, etc.).
It's an implementation detail that these catchpoints are implemented
as magic breakpoints.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-07-03 22:37 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 12:34 [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Andrew Burgess
2019-06-20 1:11 ` Kevin Buettner
2019-06-20 20:58 ` Andrew Burgess
2019-06-20 23:23 ` Andrew Burgess
2019-06-21 3:20 ` Kevin Buettner
2019-06-21 13:43 ` Pedro Alves
2019-06-22 11:06 ` Andrew Burgess
2019-06-22 11:23 ` Andrew Burgess
2019-06-24 19:16 ` Pedro Alves
2019-06-24 19:54 ` [PROTOTYPE] Make "info breakpoints" show breakpoint's specs (Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler) Pedro Alves
2019-07-03 22:37 ` Pedro Alves
2019-06-24 19:16 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
2019-07-01 17:12 ` Andrew Burgess
2019-07-01 18:03 ` [PATCHv2 2/2] " Andrew Burgess
2019-07-03 22:13 ` Pedro Alves
2019-07-01 18:03 ` [PATCHv2 1/2] gdb: Remove unneeded parameter from set_breakpoint_location_function Andrew Burgess
2019-07-03 22:13 ` Pedro Alves
2019-07-01 18:03 ` [PATCHv2 0/2] Changes for explicit_line tracking, and prologue skipping Andrew Burgess
2019-07-01 18:21 ` [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Pedro Alves
2019-07-02 8:28 ` Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox