[RFA/linespec] wrong line number in breakpoint location

[RFA/linespec] wrong line number in breakpoint location

[Joel Brobecker]

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

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

[Simon Marchi]

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

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

[Joel Brobecker]

> 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

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

[Simon Marchi]

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

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

[Joel Brobecker]

> > /* 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

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

[Joel Brobecker]

[-- 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

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

[Joel Brobecker]

> > 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

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

[Simon Marchi]

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

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

[Joel Brobecker]

[-- 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

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

[Simon Marchi]

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

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

[Joel Brobecker]

[-- 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

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

[Pedro Alves]

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