From patchwork Thu Dec 21 11:32:14 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 25062 Received: (qmail 43866 invoked by alias); 21 Dec 2017 11:32:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 43567 invoked by uid 89); 21 Dec 2017 11:32:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=2016-2017 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 21 Dec 2017 11:32:21 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id A73F81171B7; Thu, 21 Dec 2017 06:32:19 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id U+ZvwqxnyoJn; Thu, 21 Dec 2017 06:32:19 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id E0E67117127; Thu, 21 Dec 2017 06:32:18 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 7A22E809B4; Thu, 21 Dec 2017 15:32:14 +0400 (+04) Date: Thu, 21 Dec 2017 15:32:14 +0400 From: Joel Brobecker To: Simon Marchi Cc: gdb-patches@sourceware.org, Keith Seitz , Xavier Roirand Subject: Re: [RFA/linespec] wrong line number in breakpoint location Message-ID: <20171221113214.hezwvaatnbd4yzfq@adacore.com> References: <1513565091-118926-1-git-send-email-brobecker@adacore.com> <20171219092405.n2dql5ji52qhjilj@adacore.com> <206d75d6b1f14e55b6a0dff523d8c722@polymtl.ca> <20171221113127.ijqv6dnzjfifwfnb@adacore.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20171221113127.ijqv6dnzjfifwfnb@adacore.com> User-Agent: NeoMutt/20170113 (1.7.2) [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 From ff9cb05d65753f870ec58691cef8596e92c060ff Mon Sep 17 00:00:00 2001 From: Joel Brobecker 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 . */ + +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 . + +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 . */ + +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 "\"\":$line_callee4_head" @@ -83,7 +85,7 @@ proc test_tbreak_creation_and_listing {} { "-t \"\\\"${srcfile}\\\":$line_callee4_head\"" \ "insert temp breakpoint at \"\":\$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 \"\":\$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