Re: [RFA/linespec] wrong line number in breakpoint location

[Simon Marchi <simon.marchi@polymtl.ca>]

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