[RFA/linespec] wrong line number in breakpoint location
Commit Message
[with the patch, this time...]
On Thu, Dec 21, 2017 at 03:31:27PM +0400, Joel Brobecker wrote:
> > > /* 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,
> --
> Joel
Comments
> > 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.
Now pushed, after having updated the copyright year and retesting
on x86_64-linux.
On 2017-12-21 06:32 AM, Joel Brobecker wrote:
> [with the patch, this time...]
>
> On Thu, Dec 21, 2017 at 03:31:27PM +0400, Joel Brobecker wrote:
>>>> /* 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,
>> --
>> Joel
>
Hi Joel,
I started seeing a failure with this patch:
FAIL: gdb.base/break.exp: verify that they were cleared
Here is the test code:
40 int
41 main (int argc, char **argv, char **envp)
42 {
43 if (argc == 12345) { /* an unlikely value < 2^16, in case uninited */ /* set breakpoint 6 here */
44 fprintf (stderr, "usage: factorial <number>\n");
45 return 1;
46 }
47 printf ("%d\n", factorial (atoi ("6"))); /* set breakpoint 1 here */
48 /* set breakpoint 12 here */
49 marker1 (); /* set breakpoint 11 here */
50 marker2 (43); /* set breakpoint 20 here */
What happens is that we build a binary with optimization, set a breakpoint
on line 47, and expect "info break" to show it at line 47. In reality,
everything about line 47 has been inlined and there's no address associated to
line 47. The following location in that file that has generated code associated
to it is line 49, so that's where the breakpoint is placed in reality. With
this patch, "info break" therefore now shows line 49.
This particular test isn't really about testing with optimized code, it's about
checking if we can clear breakpoint commands. So we should probably test that
against a non-optimized binary.
I am using Ubuntu 16.04's default compiler, gcc 5.4.0 (the outcome of the test probably
depends on the particular compiler used). When I try on my Arch Linux machine and
gcc 7.2.1, the test passes (the generated address/line mapping is different, and
there's an address associated to line 47).
Simon
From ff9cb05d65753f870ec58691cef8596e92c060ff Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 9 Jun 2016 15:41:47 -0700
Subject: [PATCH] wrong line number in breakpoint location
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:18 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:
* 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.
Tested on x86_64-linux.
WIP/fixup
---
gdb/linespec.c | 3 --
gdb/testsuite/gdb.base/break-include.c | 57 ++++++++++++++++++++++++++++++++
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, 121 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
@@ -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,57 @@
+/* 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;
+}
+
+/* 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 place a breakpoint 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). */
+
+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]" \
--
2.1.4