From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 52801 invoked by alias); 18 Dec 2017 04:09:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 52791 invoked by uid 89); 18 Dec 2017 04:09:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,KAM_STOCKGEN,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 18 Dec 2017 04:09:28 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id vBI49MAR016499 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sun, 17 Dec 2017 23:09:26 -0500 Received: by simark.ca (Postfix, from userid 112) id EDDBF1E5A0; Sun, 17 Dec 2017 23:09:21 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id C579F1E037; Sun, 17 Dec 2017 23:09:05 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 18 Dec 2017 04:09:00 -0000 From: Simon Marchi To: Joel Brobecker Cc: gdb-patches@sourceware.org, Keith Seitz , Xavier Roirand Subject: Re: [RFA/linespec] wrong line number in breakpoint location In-Reply-To: <1513565091-118926-1-git-send-email-brobecker@adacore.com> References: <1513565091-118926-1-git-send-email-brobecker@adacore.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.2 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 18 Dec 2017 04:09:22 +0000 X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00410.txt.bz2 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 > . */ > + > +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