[RFA/linespec] wrong line number in breakpoint location
Commit Message
Hello,
Consider the following situation, where we have one file containing...
$ cat -n body.inc
1 i = i + 1;
... we include that file from some code, like so:
$ cat -n cat -n small.c
[...]
17 int
18 next (int i)
19 {
20 #include "body.inc"
21 return i;
22 }
When trying to insert a breakpoint on line 18, for instance:
(gdb) b small.c:18
Breakpoint 1 at 0x40049f: file body.inc, line 18.
^^
||
Here, the issue is that GDB reports the breakpoint to be in file
body.inc, which is true, but with the line number that corresponding
to the user-requested location, which is not correct.
Although the simple reproducer may look slightly artificial,
the above is simply one way to reproduce the same issue observed
when trying to insert a breakpoint on a function provided in
a .h files and then subsequently inlined in a C file.
What happens is the following:
1. We resolve the small.c:8 linespec into a symtab_and_line which
has "small.c" and 18 as the symtab and line number.
2. Next, we call skip_prologue_sal, which calculates the PC
past the prologue, and updates the symtab_and_line: PC,
but also symtab (now body.inc) and the new line (now 1).
3. However, right after that, we do:
/* Make sure the line matches the request, not what was
found. */
intermediate_results.sals[i].line = val.line;
We should either restore both symtab and line, or leave the actual
line to match the actual symtab. This patch chose the latter.
This introduces a few changes in a few tests, which required some
updates, but looking at those change, I believe them to be expected.
gdb/ChangeLog:
* linespec.c (create_sals_line_offset): Remove code that preserved
the symtab_and_line's line number.
gdb/testsuite/ChangeLog:
* break-include.c, break-include.inc, break-include.exp: New files.
* gdb.base/ending-run.exp: Minor adaptations due to the breakpoint's
line number now being the actual line number where the breakpoint
was inserted.
* gdb.mi/mi-break.exp: Likewise.
* gdb.mi/mi-reverse.exp: Likewise.
* gdb.mi/mi-simplerun.exp: Ditto.
Tested on x86_64-linux. OK to commit?
Thank you,
Comments
Hi Joel,
That change sounds good to me, but I'd suggest waiting to see if other
people have something to say.
I noted two nits.
On 2017-12-17 21:44, Joel Brobecker wrote:
> Hello,
>
> Consider the following situation, where we have one file containing...
>
> $ cat -n body.inc
> 1 i = i + 1;
>
> ... we include that file from some code, like so:
>
> $ cat -n cat -n small.c
> [...]
> 17 int
> 18 next (int i)
> 19 {
> 20 #include "body.inc"
> 21 return i;
> 22 }
>
> When trying to insert a breakpoint on line 18, for instance:
>
> (gdb) b small.c:18
> Breakpoint 1 at 0x40049f: file body.inc, line 18.
> ^^
> ||
>
> Here, the issue is that GDB reports the breakpoint to be in file
> body.inc, which is true, but with the line number that corresponding
> to the user-requested location, which is not correct.
>
> Although the simple reproducer may look slightly artificial,
> the above is simply one way to reproduce the same issue observed
> when trying to insert a breakpoint on a function provided in
> a .h files and then subsequently inlined in a C file.
>
> What happens is the following:
>
> 1. We resolve the small.c:8 linespec into a symtab_and_line which
small.c:18
> has "small.c" and 18 as the symtab and line number.
>
> 2. Next, we call skip_prologue_sal, which calculates the PC
> past the prologue, and updates the symtab_and_line: PC,
> but also symtab (now body.inc) and the new line (now 1).
>
> 3. However, right after that, we do:
>
> /* Make sure the line matches the request, not what was
> found. */
> intermediate_results.sals[i].line = val.line;
>
> We should either restore both symtab and line, or leave the actual
> line to match the actual symtab. This patch chose the latter.
> This introduces a few changes in a few tests, which required some
> updates, but looking at those change, I believe them to be expected.
>
> gdb/ChangeLog:
>
> * linespec.c (create_sals_line_offset): Remove code that
> preserved
> the symtab_and_line's line number.
>
> gdb/testsuite/ChangeLog:
>
> * break-include.c, break-include.inc, break-include.exp: New
> files.
> * gdb.base/ending-run.exp: Minor adaptations due to the
> breakpoint's
> line number now being the actual line number where the
> breakpoint
> was inserted.
> * gdb.mi/mi-break.exp: Likewise.
> * gdb.mi/mi-reverse.exp: Likewise.
> * gdb.mi/mi-simplerun.exp: Ditto.
>
> Tested on x86_64-linux. OK to commit?
>
> Thank you,
> --
> Joel
>
> ---
> gdb/linespec.c | 3 ---
> gdb/testsuite/gdb.base/break-include.c | 39
> ++++++++++++++++++++++++++++++++
> gdb/testsuite/gdb.base/break-include.exp | 34
> ++++++++++++++++++++++++++++
> gdb/testsuite/gdb.base/break-include.inc | 18 +++++++++++++++
> gdb/testsuite/gdb.base/ending-run.exp | 4 ++--
> gdb/testsuite/gdb.mi/mi-break.exp | 11 +++++----
> gdb/testsuite/gdb.mi/mi-reverse.exp | 2 +-
> gdb/testsuite/gdb.mi/mi-simplerun.exp | 4 ++--
> 8 files changed, 103 insertions(+), 12 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/break-include.c
> create mode 100644 gdb/testsuite/gdb.base/break-include.exp
> create mode 100644 gdb/testsuite/gdb.base/break-include.inc
>
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 8c36f2a..f81e4c1 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -2246,9 +2246,6 @@ create_sals_line_offset (struct linespec_state
> *self,
>
> if (self->funfirstline)
> skip_prologue_sal (&intermediate_results[i]);
> - /* Make sure the line matches the request, not what was
> - found. */
> - intermediate_results[i].line = val.line;
> add_sal_to_sals (self, &values, &intermediate_results[i],
> sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0);
> }
> diff --git a/gdb/testsuite/gdb.base/break-include.c
> b/gdb/testsuite/gdb.base/break-include.c
> new file mode 100644
> index 0000000..b6e6482
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/break-include.c
> @@ -0,0 +1,39 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2016-2017 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or
> modify
> + it under the terms of the GNU General Public License as published
> by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see
> <http://www.gnu.org/licenses/>. */
> +
> +int next (int i);
> +
> +int
> +main (void)
> +{
> + int result = -1;
> +
> + result = next (result);
> + return result;
> +}
> +
> +/* We implement the following function as far away from the first line
> + of this file, so as to reduce confusion between line numbers from
> + this file, and line numbers from body.inc (which only really has
body.inc is now called break-include.inc. I am a bit confused with "as
far away from the first line of this file". From what I understand, the
important thing is that the next function is at a different line than
the assignment in break-include.inc, to avoid the risk of having a false
PASS?
Thanks,
Simon
> That change sounds good to me, but I'd suggest waiting to see if other
> people have something to say.
Thanks, and of course!
> I noted two nits.
> > 1. We resolve the small.c:8 linespec into a symtab_and_line which
>
> small.c:18
Fixed.
> > +/* We implement the following function as far away from the first line
> > + of this file, so as to reduce confusion between line numbers from
> > + this file, and line numbers from body.inc (which only really has
>
> body.inc is now called break-include.inc. I am a bit confused with "as far
> away from the first line of this file". From what I understand, the
> important thing is that the next function is at a different line than the
> assignment in break-include.inc, to avoid the risk of having a false PASS?
That's correct. Attached is a second version of the patch which
hopefully clarifies everything:
/* The following function's implementation starts by including a file
(break-include.inc) which contains a copyright header followed by
a single C statement. When we break on the line where the function
name is declared, we expect GDB to skip the function's prologue,
and insert the breakpoint on the first line of "user" code for
that function, which we have set up to be that single statement
break-include.inc provides.
The purpose of this testcase is to verify that, when we insert
that breakpoint, GDB reports the location as being in that include
file, but also using the correct line number inside that include
file -- NOT the line number we originally used to insert the
breakpoint, nor the location where the file is included from.
In order to verify that GDB shows the right line number, we must
be careful that this first statement located in break-include.inc
and our function are not on the same line number. Otherwise,
we could potentially have a false PASS.
This is why we implement the following function as far away
from the start of this file as possible, as we know that
break-include.inc is a fairly short file (copyright header
and single statement only). */
gdb/ChangeLog:
* linespec.c (create_sals_line_offset): Remove code that preserved
the symtab_and_line's line number.
gdb/testsuite/ChangeLog:
* break-include.c, break-include.inc, break-include.exp: New files.
* gdb.base/ending-run.exp: Minor adaptations due to the breakpoint's
line number now being the actual line number where the breakpoint
was inserted.
* gdb.mi/mi-break.exp: Likewise.
* gdb.mi/mi-reverse.exp: Likewise.
* gdb.mi/mi-simplerun.exp: Ditto.
Tested on x86_64-linux.
Thanks,
On 2017-12-19 04:24, Joel Brobecker wrote:
> That's correct. Attached is a second version of the patch which
> hopefully clarifies everything:
>
> /* The following function's implementation starts by including a file
> (break-include.inc) which contains a copyright header followed by
> a single C statement. When we break on the line where the function
I would say "place a breakpoint" instead of break. For me "to break" is
the action of the program stopping on a breakpoint (though maybe it
> name is declared, we expect GDB to skip the function's prologue,
> and insert the breakpoint on the first line of "user" code for
> that function, which we have set up to be that single statement
> break-include.inc provides.
>
> The purpose of this testcase is to verify that, when we insert
> that breakpoint, GDB reports the location as being in that include
> file, but also using the correct line number inside that include
> file -- NOT the line number we originally used to insert the
> breakpoint, nor the location where the file is included from.
> In order to verify that GDB shows the right line number, we must
> be careful that this first statement located in break-include.inc
> and our function are not on the same line number. Otherwise,
> we could potentially have a false PASS.
>
> This is why we implement the following function as far away
> from the start of this file as possible, as we know that
> break-include.inc is a fairly short file (copyright header
> and single statement only). */
LGTM.
Simon
> > /* The following function's implementation starts by including a file
> > (break-include.inc) which contains a copyright header followed by
> > a single C statement. When we break on the line where the function
>
> I would say "place a breakpoint" instead of break. For me "to break" is the
> action of the program stopping on a breakpoint (though maybe it
Sounds good.
Here is a new version :). I also noticed I forgot the gdb.base/
subdir in the name of the new files in the testsuite, so I fixed
that up too.
gdb/ChangeLog:
* linespec.c (create_sals_line_offset): Remove code that preserved
the symtab_and_line's line number.
gdb/testsuite/ChangeLog:
* gdb.base/break-include.c, gdb.base/break-include.inc,
gdb.base/break-include.exp: New files.
* gdb.base/ending-run.exp: Minor adaptations due to the breakpoint's
line number now being the actual line number where the breakpoint
was inserted.
* gdb.mi/mi-break.exp: Likewise.
* gdb.mi/mi-reverse.exp: Likewise.
* gdb.mi/mi-simplerun.exp: Ditto.
Thanks,
@@ -2246,9 +2246,6 @@ create_sals_line_offset (struct linespec_state *self,
if (self->funfirstline)
skip_prologue_sal (&intermediate_results[i]);
- /* Make sure the line matches the request, not what was
- found. */
- intermediate_results[i].line = val.line;
add_sal_to_sals (self, &values, &intermediate_results[i],
sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0);
}
new file mode 100644
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2016-2017 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+int next (int i);
+
+int
+main (void)
+{
+ int result = -1;
+
+ result = next (result);
+ return result;
+}
+
+/* We implement the following function as far away from the first line
+ of this file, so as to reduce confusion between line numbers from
+ this file, and line numbers from body.inc (which only really has
+ one line of code). */
+
+int
+next (int i) /* break here */
+{
+#include "break-include.inc"
+ return i;
+}
new file mode 100644
@@ -0,0 +1,34 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2016-2017 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/>.
+
+standard_testfile
+
+if { [prepare_for_testing ${testfile}.exp $testfile] } {
+ return -1
+}
+
+set bp_line [gdb_get_line_number "break here" ${testfile}.c]
+set bp_line_actual [gdb_get_line_number "ANCHOR" ${testfile}.inc]
+
+gdb_test "break $testfile.c:$bp_line" \
+ ".*Breakpoint.*$testfile.inc, line $bp_line_actual\\."
+
+# Might as well verify that breaking on function "next" gives
+# the same result...
+
+gdb_test "break next" \
+ ".*Breakpoint.*$testfile.inc, line $bp_line_actual\\."
new file mode 100644
@@ -0,0 +1,18 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2016-2017 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/>. */
+
+i = i + 1; /* ANCHOR */
@@ -68,14 +68,14 @@ gdb_test_multiple "i b" "cleared bp at line before routine" {
gdb_test "b ending-run.c:1" ".*Breakpoint.*4.*"
gdb_test "b ending-run.c:$break1_line" ".*Note.*also.*Breakpoint.*5.*" "b ending-run.c:$break1_line, two"
gdb_test "cle ending-run.c:$break1_line" \
- ".*Deleted breakpoint 5.*" "Cleared 2 by line"
+ ".*Deleted breakpoints 4 5.*" "Cleared 2 by line"
gdb_test_multiple "info line ending-run.c:$break1_line" "" {
-re ".*address (0x\[0-9a-fA-F]*).*$gdb_prompt $" {
set line_nine $expect_out(1,string)
gdb_test "b ending-run.c:$break1_line" ".*Breakpoint 6.*ending-run.c, line $break1_line.*"
gdb_test "b *$line_nine" ".*Note.*also.*Breakpoint 7.*" "breakpoint 7 at *ending-run.c:$break1_line"
- gdb_test "cle" ".*Deleted breakpoints 4 6 7.*" "clear 2 by default"
+ gdb_test "cle" ".*Deleted breakpoints 6 7.*" "clear 2 by default"
}
-re ".*$gdb_prompt $" {
fail "need to fix test for new compile outcome"
@@ -49,7 +49,9 @@ set fullname "fullname=\"${fullname_syntax}${srcfile}\""
proc test_tbreak_creation_and_listing {} {
global srcfile
global line_callee4_head
+ global line_callee4_body
global line_callee3_head
+ global line_callee3_body
global line_callee2_body
global line_main_body
@@ -75,7 +77,7 @@ proc test_tbreak_creation_and_listing {} {
lappend bps [mi_create_breakpoint "-t basics.c:$line_callee3_head" \
"insert temp breakpoint at basics.c:\$line_callee3_head" \
-number 3 -disp del -func callee3 -file ".*basics.c" \
- -line $line_callee3_head]
+ -line $line_callee3_body]
# Getting the quoting right is tricky.
# That is "\"<file>\":$line_callee4_head"
@@ -83,7 +85,7 @@ proc test_tbreak_creation_and_listing {} {
"-t \"\\\"${srcfile}\\\":$line_callee4_head\"" \
"insert temp breakpoint at \"<fullfilename>\":\$line_callee4_head" \
-number 4 -disp del -func callee4 -file ".*basics.c" \
- -line $line_callee4_head]
+ -line $line_callee4_body]
mi_gdb_test "666-break-list" \
"666\\\^done,[mi_make_breakpoint_table $bps]" \
@@ -319,6 +321,7 @@ proc test_breakpoint_commands {} {
proc test_explicit_breakpoints {} {
global srcfile
global line_callee3_head line_callee4_head
+ global line_callee3_body line_callee4_body
global line_callee2_body line_main_body
mi_delete_breakpoints
@@ -349,13 +352,13 @@ proc test_explicit_breakpoints {} {
lappend bps \
[mi_create_breakpoint "-t --source $srcfile --line $line_callee3_head" \
"insert temp explicit breakpoint at $srcfile:$line_callee3_head" \
- -func callee3 -file ".*$srcfile" -line $line_callee3_head]
+ -func callee3 -file ".*$srcfile" -line $line_callee3_body]
lappend bps \
[mi_create_breakpoint \
"-t --source \"$srcfile\" --line $line_callee4_head" \
"insert temp explicit breakpoint at \"$srcfile\":$line_callee4_head" \
- -func callee4 -file ".*$srcfile" -line $line_callee4_head]
+ -func callee4 -file ".*$srcfile" -line $line_callee4_body]
mi_gdb_test "-break-list" "\\^done,[mi_make_breakpoint_table $bps]" \
"list of explicit breakpoints"
@@ -153,7 +153,7 @@ proc test_controlled_execution_reverse {} {
mi_create_breakpoint "-t basics.c:$line_callee3_head" \
"insert temp breakpoint at basics.c:$line_callee3_head" \
-number 3 -disp del -func callee3 -file ".*basics.c" \
- -line $line_callee3_head
+ -line $line_callee3_body
mi_execute_to "exec-continue --reverse" \
"breakpoint-hit" "callee3" \
@@ -78,13 +78,13 @@ proc test_breakpoints_creation_and_listing {} {
lappend bps [mi_create_breakpoint "basics.c:$line_callee3_head" \
"insert breakpoint at basics.c:\$line_callee3_head" \
-number 3 -func callee3 -file ".*basics.c" \
- -line $line_callee3_head]
+ -line $line_callee3_body]
lappend bps [mi_create_breakpoint \
"\"\\\"${srcfile}\\\":$line_callee4_head\"" \
"insert breakpoint at \"<fullfilename>\":\$line_callee4_head" \
-number 4 -func callee4 -file ".*basics.c" \
- -line $line_callee4_head]
+ -line $line_callee4_body]
mi_gdb_test "204-break-list" \
"204\\^done,[mi_make_breakpoint_table $bps]" \