* [RFA/linespec] wrong line number in breakpoint location @ 2017-12-18 2:44 Joel Brobecker 2017-12-18 4:09 ` Simon Marchi 0 siblings, 1 reply; 12+ messages in thread From: Joel Brobecker @ 2017-12-18 2:44 UTC (permalink / raw) To: gdb-patches; +Cc: Keith Seitz, Xavier Roirand 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, -- 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 + one line of code). */ + +int +next (int i) /* break here */ +{ +#include "break-include.inc" + return i; +} diff --git a/gdb/testsuite/gdb.base/break-include.exp b/gdb/testsuite/gdb.base/break-include.exp new file mode 100644 index 0000000..a29a221 --- /dev/null +++ b/gdb/testsuite/gdb.base/break-include.exp @@ -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\\." diff --git a/gdb/testsuite/gdb.base/break-include.inc b/gdb/testsuite/gdb.base/break-include.inc new file mode 100644 index 0000000..4573672 --- /dev/null +++ b/gdb/testsuite/gdb.base/break-include.inc @@ -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 */ diff --git a/gdb/testsuite/gdb.base/ending-run.exp b/gdb/testsuite/gdb.base/ending-run.exp index 42cbf23..e2b1ccc 100644 --- a/gdb/testsuite/gdb.base/ending-run.exp +++ b/gdb/testsuite/gdb.base/ending-run.exp @@ -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" diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp index 41a48a1..0005f7e 100644 --- a/gdb/testsuite/gdb.mi/mi-break.exp +++ b/gdb/testsuite/gdb.mi/mi-break.exp @@ -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" diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp index 448a9a3..30b96f3 100644 --- a/gdb/testsuite/gdb.mi/mi-reverse.exp +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp @@ -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" \ diff --git a/gdb/testsuite/gdb.mi/mi-simplerun.exp b/gdb/testsuite/gdb.mi/mi-simplerun.exp index db75f4d..e6b5a4f 100644 --- a/gdb/testsuite/gdb.mi/mi-simplerun.exp +++ b/gdb/testsuite/gdb.mi/mi-simplerun.exp @@ -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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2017-12-18 2:44 [RFA/linespec] wrong line number in breakpoint location Joel Brobecker @ 2017-12-18 4:09 ` Simon Marchi 2017-12-19 9:24 ` Joel Brobecker 0 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2017-12-18 4:09 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches, Keith Seitz, Xavier Roirand 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2017-12-18 4:09 ` Simon Marchi @ 2017-12-19 9:24 ` Joel Brobecker 2017-12-21 1:31 ` Simon Marchi 0 siblings, 1 reply; 12+ messages in thread From: Joel Brobecker @ 2017-12-19 9:24 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches, Keith Seitz, Xavier Roirand > 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, -- Joel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2017-12-19 9:24 ` Joel Brobecker @ 2017-12-21 1:31 ` Simon Marchi 2017-12-21 11:31 ` Joel Brobecker 0 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2017-12-21 1:31 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches, Keith Seitz, Xavier Roirand 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2017-12-21 1:31 ` Simon Marchi @ 2017-12-21 11:31 ` Joel Brobecker 2017-12-21 11:32 ` Joel Brobecker 0 siblings, 1 reply; 12+ messages in thread From: Joel Brobecker @ 2017-12-21 11:31 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches, Keith Seitz, Xavier Roirand > > /* 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2017-12-21 11:31 ` Joel Brobecker @ 2017-12-21 11:32 ` Joel Brobecker 2018-01-22 4:17 ` pushed: " Joel Brobecker 2018-01-26 17:16 ` Simon Marchi 0 siblings, 2 replies; 12+ messages in thread From: Joel Brobecker @ 2017-12-21 11:32 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches, Keith Seitz, Xavier Roirand [-- Attachment #1: Type: text/plain, Size: 1287 bytes --] [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 -- Joel [-- Attachment #2: 0001-wrong-line-number-in-breakpoint-location.patch --] [-- Type: text/x-diff, Size: 13964 bytes --] 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 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..3662bc9 --- /dev/null +++ b/gdb/testsuite/gdb.base/break-include.c @@ -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; +} diff --git a/gdb/testsuite/gdb.base/break-include.exp b/gdb/testsuite/gdb.base/break-include.exp new file mode 100644 index 0000000..a29a221 --- /dev/null +++ b/gdb/testsuite/gdb.base/break-include.exp @@ -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\\." diff --git a/gdb/testsuite/gdb.base/break-include.inc b/gdb/testsuite/gdb.base/break-include.inc new file mode 100644 index 0000000..4573672 --- /dev/null +++ b/gdb/testsuite/gdb.base/break-include.inc @@ -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 */ diff --git a/gdb/testsuite/gdb.base/ending-run.exp b/gdb/testsuite/gdb.base/ending-run.exp index 42cbf23..e2b1ccc 100644 --- a/gdb/testsuite/gdb.base/ending-run.exp +++ b/gdb/testsuite/gdb.base/ending-run.exp @@ -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" diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp index 41a48a1..0005f7e 100644 --- a/gdb/testsuite/gdb.mi/mi-break.exp +++ b/gdb/testsuite/gdb.mi/mi-break.exp @@ -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" diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp index 448a9a3..30b96f3 100644 --- a/gdb/testsuite/gdb.mi/mi-reverse.exp +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp @@ -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" \ diff --git a/gdb/testsuite/gdb.mi/mi-simplerun.exp b/gdb/testsuite/gdb.mi/mi-simplerun.exp index db75f4d..e6b5a4f 100644 --- a/gdb/testsuite/gdb.mi/mi-simplerun.exp +++ b/gdb/testsuite/gdb.mi/mi-simplerun.exp @@ -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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* pushed: [RFA/linespec] wrong line number in breakpoint location 2017-12-21 11:32 ` Joel Brobecker @ 2018-01-22 4:17 ` Joel Brobecker 2018-01-26 17:16 ` Simon Marchi 1 sibling, 0 replies; 12+ messages in thread From: Joel Brobecker @ 2018-01-22 4:17 UTC (permalink / raw) To: gdb-patches; +Cc: Keith Seitz > > 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. -- Joel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2017-12-21 11:32 ` Joel Brobecker 2018-01-22 4:17 ` pushed: " Joel Brobecker @ 2018-01-26 17:16 ` Simon Marchi 2018-01-29 4:45 ` Joel Brobecker 1 sibling, 1 reply; 12+ messages in thread From: Simon Marchi @ 2018-01-26 17:16 UTC (permalink / raw) To: Joel Brobecker, Simon Marchi; +Cc: gdb-patches, Keith Seitz, Xavier Roirand 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2018-01-26 17:16 ` Simon Marchi @ 2018-01-29 4:45 ` Joel Brobecker 2018-01-29 17:01 ` Simon Marchi 0 siblings, 1 reply; 12+ messages in thread From: Joel Brobecker @ 2018-01-29 4:45 UTC (permalink / raw) To: Simon Marchi; +Cc: Simon Marchi, gdb-patches, Keith Seitz, Xavier Roirand [-- Attachment #1: Type: text/plain, Size: 2641 bytes --] Hi Simon, > 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. Looking at the assembly code between the two versions, the difference is that in the GCC 5.x case, the printf called gets inlined (!), whereas it does not when usin GCC 7.x, even on the same system. So, in the first case, the code generated for line 47 gets line numbers referencing either another file, or another function, which explains why we end up breaking on the next line of code, which is line 49. With the more recent version of GCC, the call to printf is no longer inlined, and so we have some instructions "attached" to line 47, thanks to the call to "printf". > 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. That's true that it doesn't seem necessary to perform that check against the optimized version. On the other hand, we could keep the testcase as is, by simply extracting from the output of the "break" command which line we actually broke on, and then use that in the expected output. WDYT? gdb/testsuite/ChangeLog: * gdb.base/break.exp: Save the location where the breakpoint on break.c:47 was actually inserted when debugging the version compiled at -O2 and use it in the expected output of the "info break" test performed soon after. tested on x86_64-linux, with two configurations: - Ubuntu 16.04 with the system compiler (breakpoint lands on line 49) - Ubuntu 16.04 with GCC 7.3.1 (breakpoint lands on line 47) -- Joel [-- Attachment #2: 0001-gdb.base-break.exp-fix-last-info-break-test-failure-.patch --] [-- Type: text/x-diff, Size: 3402 bytes --] From d9207d517fc2e9b6460ac769553f7c0841ac9d91 Mon Sep 17 00:00:00 2001 From: Joel Brobecker <brobecker@adacore.com> Date: Sun, 28 Jan 2018 23:20:42 -0500 Subject: [PATCH] gdb.base/break.exp: fix last "info break" test failure on Ubuntu 16.04 The last test of this testcase fails when run on Ubuntu 16.04 using the system compiler (16.04): FAIL: gdb.base/break.exp: verify that they were cleared This is because the testcase expected that a breakpoint on line 47 of break.c... printf ("%d\n", factorial (atoi ("6"))); /* set breakpoint 1 here */ ... would actually be inserted on an instruction belonging to that line. However, what actually happens is that system GCC on that version of Ubuntu ends up inlining everything, including the call to printf, thus reporting every instruction of generated for this line of code as belonging to a different function. As a result, GDB ends up insering the breakpoint on the next line of code, which is line 49: (gdb) break break.c:$l Breakpoint 3 at 0x4005c1: file /[...]/gdb.base/break.c, line 49. This causes a spurious failure in the "info break" test later on, as it assumed that the breakpoint above is inserted on line 47: gdb_test "info break" "$srcfile:$line" "verify that they were cleared" This patch fixes the issue by saving the actual source location where the breakpoint was inserted. gdb/testsuite/ChangeLog: * gdb.base/break.exp: Save the location where the breakpoint on break.c:47 was actually inserted when debugging the version compiled at -O2 and use it in the expected output of the "info break" test performed soon after. tested on x86_64-linux, with two configurations: - Ubuntu 16.04 with the system compiler (breakpoint lands on line 49) - Ubuntu 16.04 with GCC 7.3.1 (breakpoint lands on line 47) --- gdb/testsuite/gdb.base/break.exp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp index 8a21222..2bd0eb1 100644 --- a/gdb/testsuite/gdb.base/break.exp +++ b/gdb/testsuite/gdb.base/break.exp @@ -847,7 +847,18 @@ gdb_test_multiple "" $test { # set line [gdb_get_line_number "set breakpoint 1 here"] gdb_test_no_output "set \$l = $line" -gdb_breakpoint ${srcfile}:\$l +set test "break ${srcfile}:\$l" +gdb_test_multiple "$test" $test { + -re "Breakpoint $decimal at $hex: file .*break\\.c, line ($decimal)\\.\r\n$gdb_prompt $" { + # Save the actual line number on which the breakpoint was + # actually set. On some systems (Eg: Ubuntu 16.04 with GCC + # version 5.4.0), that line gets completely inlined, including + # the call to printf, and so we end up inserting the breakpoint + # on one of the following lines instead. + set line_actual $expect_out(1,string) + pass $test + } +} gdb_test_no_output "set \$foo=81.5" \ "set convenience variable \$foo to 81.5" @@ -865,4 +876,4 @@ gdb_test "commands\nend" ">end" "clear breakpoint commands" # We verify that the commands were cleared by ensuring that the last # breakpoint's location ends the output -- if there were commands, # they would have been printed after the location. -gdb_test "info break" "$srcfile:$line" "verify that they were cleared" +gdb_test "info break" "$srcfile:$line_actual" "verify that they were cleared" -- 2.1.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2018-01-29 4:45 ` Joel Brobecker @ 2018-01-29 17:01 ` Simon Marchi 2018-01-30 4:09 ` Joel Brobecker 0 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2018-01-29 17:01 UTC (permalink / raw) To: Joel Brobecker; +Cc: Simon Marchi, gdb-patches, Keith Seitz, Xavier Roirand On 2018-01-28 23:45, Joel Brobecker wrote: > Hi Simon, > >> 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. > > Looking at the assembly code between the two versions, the difference > is that in the GCC 5.x case, the printf called gets inlined (!), > whereas it does not when usin GCC 7.x, even on the same system. > So, in the first case, the code generated for line 47 gets > line numbers referencing either another file, or another function, > which explains why we end up breaking on the next line of code, > which is line 49. > > With the more recent version of GCC, the call to printf is no longer > inlined, and so we have some instructions "attached" to line 47, > thanks to the call to "printf". > >> 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. > > That's true that it doesn't seem necessary to perform that check > against the optimized version. On the other hand, we could keep > the testcase as is, by simply extracting from the output of the > "break" command which line we actually broke on, and then use that > in the expected output. > > WDYT? > > gdb/testsuite/ChangeLog: > > * gdb.base/break.exp: Save the location where the breakpoint > on break.c:47 was actually inserted when debugging the version > compiled at -O2 and use it in the expected output of the "info > break" test performed soon after. > > tested on x86_64-linux, with two configurations: > - Ubuntu 16.04 with the system compiler (breakpoint lands on line 49) > - Ubuntu 16.04 with GCC 7.3.1 (breakpoint lands on line 47) Hi Joel, Thanks, this is fine with me. Just a really small nit, I would suggest initializing the line_actual variable to 0 or -1 (an invalid line number) prior to calling gdb_test_multiple. This way, if that test fails, line_actual will still be defined, and the expression that refers to it will generate a FAIL instead of an unreadable tcl backtrace. Simon ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2018-01-29 17:01 ` Simon Marchi @ 2018-01-30 4:09 ` Joel Brobecker 2018-01-30 12:39 ` Pedro Alves 0 siblings, 1 reply; 12+ messages in thread From: Joel Brobecker @ 2018-01-30 4:09 UTC (permalink / raw) To: Simon Marchi; +Cc: Simon Marchi, gdb-patches, Keith Seitz, Xavier Roirand [-- Attachment #1: Type: text/plain, Size: 815 bytes --] > Thanks, this is fine with me. Just a really small nit, I would suggest > initializing the line_actual variable to 0 or -1 (an invalid line number) > prior to calling gdb_test_multiple. This way, if that test fails, > line_actual will still be defined, and the expression that refers to it will > generate a FAIL instead of an unreadable tcl backtrace. Sounds good. Just for kicks, I took at look at what it looks like when the variable is undefined, and the error message is obvious about why it fails. When the error is defined, however, you have to figure out what the difference is, and track the value of that variable down. What won me over to your suggestion is that the error can go unnoticed if you just compare .sum files... Attached is the patch I just pushed (re-tested on x86_64-linux). -- Joel [-- Attachment #2: 0001-gdb.base-break.exp-fix-last-info-break-test-failure-.patch --] [-- Type: text/x-diff, Size: 4031 bytes --] From fc413dc467e4c46013f6e5a60dc5db24d63f72ea Mon Sep 17 00:00:00 2001 From: Joel Brobecker <brobecker@adacore.com> Date: Mon, 29 Jan 2018 23:03:09 -0500 Subject: [PATCH] gdb.base/break.exp: fix last "info break" test failure on Ubuntu 16.04 The last test of this testcase fails when run on Ubuntu 16.04 using the system compiler (16.04): FAIL: gdb.base/break.exp: verify that they were cleared This is because the testcase expected that a breakpoint on line 47 of break.c... printf ("%d\n", factorial (atoi ("6"))); /* set breakpoint 1 here */ ... would actually be inserted on an instruction belonging to that line. However, what actually happens is that system GCC on that version of Ubuntu ends up inlining everything, including the call to printf, thus reporting every instruction of generated for this line of code as belonging to a different function. As a result, GDB ends up insering the breakpoint on the next line of code, which is line 49: (gdb) break break.c:$l Breakpoint 3 at 0x4005c1: file /[...]/gdb.base/break.c, line 49. This causes a spurious failure in the "info break" test later on, as it assumed that the breakpoint above is inserted on line 47: gdb_test "info break" "$srcfile:$line" "verify that they were cleared" This patch fixes the issue by saving the actual source location where the breakpoint was inserted. gdb/testsuite/ChangeLog: * gdb.base/break.exp: Save the location where the breakpoint on break.c:47 was actually inserted when debugging the version compiled at -O2 and use it in the expected output of the "info break" test performed soon after. tested on x86_64-linux, with two configurations: - Ubuntu 16.04 with the system compiler (breakpoint lands on line 49) - Ubuntu 16.04 with GCC 7.3.1 (breakpoint lands on line 47) --- gdb/testsuite/ChangeLog | 7 +++++++ gdb/testsuite/gdb.base/break.exp | 17 +++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 0f02f4a..52a689f 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2018-01-30 Joel Brobecker <brobecker@adacore.com> + + * gdb.base/break.exp: Save the location where the breakpoint + on break.c:47 was actually inserted when debugging the version + compiled at -O2 and use it in the expected output of the "info + break" test performed soon after. + 2018-01-22 Pedro Alves <palves@redhat.com> Sergio Durigan Junior <sergiodj@redhat.com> diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp index 8a21222..7f035a8 100644 --- a/gdb/testsuite/gdb.base/break.exp +++ b/gdb/testsuite/gdb.base/break.exp @@ -847,7 +847,20 @@ gdb_test_multiple "" $test { # set line [gdb_get_line_number "set breakpoint 1 here"] gdb_test_no_output "set \$l = $line" -gdb_breakpoint ${srcfile}:\$l + +set line_actual "-1" +set test "break ${srcfile}:\$l" +gdb_test_multiple "$test" $test { + -re "Breakpoint $decimal at $hex: file .*break\\.c, line ($decimal)\\.\r\n$gdb_prompt $" { + # Save the actual line number on which the breakpoint was + # actually set. On some systems (Eg: Ubuntu 16.04 with GCC + # version 5.4.0), that line gets completely inlined, including + # the call to printf, and so we end up inserting the breakpoint + # on one of the following lines instead. + set line_actual $expect_out(1,string) + pass $test + } +} gdb_test_no_output "set \$foo=81.5" \ "set convenience variable \$foo to 81.5" @@ -865,4 +878,4 @@ gdb_test "commands\nend" ">end" "clear breakpoint commands" # We verify that the commands were cleared by ensuring that the last # breakpoint's location ends the output -- if there were commands, # they would have been printed after the location. -gdb_test "info break" "$srcfile:$line" "verify that they were cleared" +gdb_test "info break" "$srcfile:$line_actual" "verify that they were cleared" -- 2.1.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFA/linespec] wrong line number in breakpoint location 2018-01-30 4:09 ` Joel Brobecker @ 2018-01-30 12:39 ` Pedro Alves 0 siblings, 0 replies; 12+ messages in thread From: Pedro Alves @ 2018-01-30 12:39 UTC (permalink / raw) To: Joel Brobecker, Simon Marchi Cc: Simon Marchi, gdb-patches, Keith Seitz, Xavier Roirand On 01/30/2018 04:09 AM, Joel Brobecker wrote: >> Thanks, this is fine with me. Just a really small nit, I would suggest >> initializing the line_actual variable to 0 or -1 (an invalid line number) >> prior to calling gdb_test_multiple. This way, if that test fails, >> line_actual will still be defined, and the expression that refers to it will >> generate a FAIL instead of an unreadable tcl backtrace. > > Sounds good. Just for kicks, I took at look at what it looks like > when the variable is undefined, and the error message is obvious > about why it fails. When the error is defined, however, you have > to figure out what the difference is, and track the value of that > variable down. What won me over to your suggestion is that the > error can go unnoticed if you just compare .sum files... My view is that since "using" an undefined variable results in a TCL error, that is a testsuite bug, plain and simple. A TCL error is lower level than a dejagnu FAIL, and aborts the whole testcase, while a dejagnu FAIL indicates that the testing harness is working and caught GDB behaving unexpectedly in the particular use case being tested. A FAIL can go on and run subsequent procedures/tests. In cases like this where we're extracting variables with expect_out, we often add failed-to-extract-variable handling after the gdb_test_multiple, like this: # start with "impossible" value. set whatever -1 gdb_test_multiple $test $test { -re "whatever is ($decimal)\r\n$gdb_prompt $" { set whatever $expect_out(1,string) pass $test } } if {$whatever == -1} { # bail out, no use continuing the procedure. return } So you get a FAIL in the gdb_test_multiple case, and skip running the rest of the procedure. Sometimes we call "untested" before returning. It's a bit easier to see usefulness of the pattern if the tests _are_ put in a procedure called by a main testcase driver, like: ~~~~~~~~~~~~~~~~~~~~~~~~~~~ proc_with_prefix test_whatever {} { set whatever -1 gdb_test_multiple $test $test { -re "whatever is ($decimal)\r\n$gdb_prompt $" { set whatever $expect_out(1,string) pass $test } } if {$whatever == -1} { # bail out, no use continuing the procedure. return } } proc_with_prefix test_whatever_else {} { ... } # The drive code that calls test procedures. test_whatever test_whatever_else ~~~~~~~~~~~~~~~~~~~~~~~~~~~ And now test_whatever_else runs even if test_whatever bails out. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-01-30 12:39 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-18 2:44 [RFA/linespec] wrong line number in breakpoint location Joel Brobecker 2017-12-18 4:09 ` Simon Marchi 2017-12-19 9:24 ` Joel Brobecker 2017-12-21 1:31 ` Simon Marchi 2017-12-21 11:31 ` Joel Brobecker 2017-12-21 11:32 ` Joel Brobecker 2018-01-22 4:17 ` pushed: " Joel Brobecker 2018-01-26 17:16 ` Simon Marchi 2018-01-29 4:45 ` Joel Brobecker 2018-01-29 17:01 ` Simon Marchi 2018-01-30 4:09 ` Joel Brobecker 2018-01-30 12:39 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).