Re: [RFC/RFA?] Should break FILE:LINENO skip prologue?

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

Hi Daniel,

> > 2008-01-09  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * breakpoint.c (skip_prologue_sal): New function.
> >         (resolve_sal_pc): Adjust SAL past prologue if the SAL was
> >         computed from a line number.
> 
> I tried to reread the thread below this patch, but eventually got
> lost.  What was the verdict?

As I said yesterday, the idea eventually got accepted by everyone.
Here is the patch again, together with the associated adjustments
to the testsuite.

2008-02-01  Joel Brobecker  <brobecker@adacore.com>

        * breakpoint.c (skip_prologue_sal): New function.
        (resolve_sal_pc): Adjust SAL past prologue if the SAL was
        computed from a line number.

2008-02-01  Joel Brobecker  <brobecker@adacore.com>

        * gdb.base/ending-run.exp: Use the first line of code inside
        function body to test breakpoints.
        * gdb.mi/mi-break.exp, gdb.mi/mi2-break.exp: Adjust the actual
        location where the breakpoint is inserted when using the line
        where a function is declared. Fix typo in the description of
        one of the tests.
        * gdb.mi/mi-simplerun.exp, gdb.mi/mi2-simplerun.exp: Likewise.

Tested on x86-linux.

In the first testcase, the adjustment I made was by avoiding the use
of the line where the function is declared or where the open curly
brace is.  By looking at the testcase, I think it still preserves
the intent of the testcase.  In the second case, I kept the breakpoints
where they are, but adjusted the expected output to reflect the new
behavior.

That leaves the documentation to be updated (and perhaps a NEWS entry?)

OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: skip-prologue.diff --]
[-- Type: text/plain, Size: 1275 bytes --]

Index: breakpoint.c
===================================================================
--- breakpoint.c	(revision 117)
+++ breakpoint.c	(revision 118)
@@ -5446,6 +5446,25 @@ gdb_breakpoint (char *address, char *con
 			       0);
 }
 
+/* Adjust SAL to the first instruction past the function prologue.
+   The end of the prologue is determined using the line table from
+   the debugging information.
+
+   If SAL is already past the prologue, then do nothing.  */
+
+static void
+skip_prologue_sal (struct symtab_and_line *sal)
+{
+  struct symbol *sym = find_pc_function (sal->pc);
+  struct symtab_and_line start_sal;
+
+  if (sym == NULL)
+    return;
+
+  start_sal = find_function_start_sal (sym, 1);
+  if (sal->pc < start_sal.pc)
+    *sal = start_sal;
+}
 
 /* Helper function for break_command_1 and disassemble_command.  */
 
@@ -5460,6 +5479,11 @@ resolve_sal_pc (struct symtab_and_line *
 	error (_("No line %d in file \"%s\"."),
 	       sal->line, sal->symtab->filename);
       sal->pc = pc;
+
+      /* If this SAL corresponds to a breakpoint inserted using
+         a line number, then skip the function prologue if necessary.  */
+      if (sal->explicit_line)
+        skip_prologue_sal (sal);
     }
 
   if (sal->section == 0 && sal->symtab != NULL)

[-- Attachment #3: skip-prologue-tc.diff --]
[-- Type: text/plain, Size: 10318 bytes --]

Index: gdb.base/ending-run.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/ending-run.exp,v
retrieving revision 1.30
diff -u -p -r1.30 ending-run.exp
--- gdb.base/ending-run.exp	1 Jan 2008 22:53:18 -0000	1.30
+++ gdb.base/ending-run.exp	2 Feb 2008 01:08:55 -0000
@@ -55,9 +55,9 @@ gdb_load ${binfile}
 gdb_test "b ending-run.c:1" ".*Breakpoint.*ending-run.c, line 1.*" \
 	"bpt at line before routine"
 
-gdb_test "b ending-run.c:13" \
-	".*Note.*also.*Breakpoint 2.*ending-run.c, line 13.*" \
-	"b ending-run.c:13, one"
+gdb_test "b ending-run.c:14" \
+	".*Note.*also.*Breakpoint 2.*ending-run.c, line 14.*" \
+	"b ending-run.c:14, one"
 
 # Set up to go to the next-to-last line of the program
 #
@@ -67,7 +67,7 @@ gdb_test "b ending-run.c:31" ".*Breakpoi
 # as line "13".  Then try to clear it--this should work.
 #
 gdb_run_cmd
-gdb_test "" ".*Breakpoint.*1.*callee.*13.*" "run"
+gdb_test "" ".*Breakpoint.*1.*callee.*14.*" "run"
 
 gdb_test "cle" ".*Deleted breakpoints 1 2.*" "clear worked"
 send_gdb "i b\n"
@@ -86,30 +86,17 @@ gdb_expect {
 # Test some other "clear" combinations
 #
 gdb_test "b ending-run.c:1" ".*Breakpoint.*4.*"
-gdb_test "b ending-run.c:13" ".*Note.*also.*Breakpoint.*5.*" "b ending-run.c:13, two"
-gdb_test "cle ending-run.c:13" \
-	".*Deleted breakpoint 5.*" "Only cleared 1 by line"
-
-send_gdb "inf line ending-run.c:13\n"
-gdb_expect {
-    -re ".*address (0x\[0-9a-fA-F]*).*$gdb_prompt $" {
-        set line_eight $expect_out(1,string)
-        gdb_test "b 13" ".*Breakpoint.*6.*"
-        gdb_test "cle *$line_eight" ".*Deleted breakpoints 4 6.*" "Clear 2 by address"
-    }
-    -re ".*$gdb_prompt $" {
-        fail "need to fix test for new compile outcome"
-    }
-}
+gdb_test "b ending-run.c:14" ".*Note.*also.*Breakpoint.*5.*" "b ending-run.c:14, two"
+gdb_test "cle ending-run.c:14" \
+	".*Deleted breakpoints 4 5.*" "Cleared 2 by line"
 
 send_gdb "inf line ending-run.c:14\n"
 gdb_expect {
     -re ".*address (0x\[0-9a-fA-F]*).*$gdb_prompt $" {
         set line_nine $expect_out(1,string)
-        gdb_test "b ending-run.c:14" ".*Breakpoint 7.*ending-run.c, line 14.*"
-        gdb_test "b *$line_nine" ".*Note.*also.*Breakpoint 8.*" "Breakpoint 7 at *ending-run.c:14"
-        gdb_test "c" ".*Breakpoint.*7.*callee.*14.*"
-        gdb_test "cle" ".*Deleted breakpoints 7 8.*" "Clear 2 by default"
+        gdb_test "b ending-run.c:14" ".*Breakpoint 6.*ending-run.c, line 14.*"
+        gdb_test "b *$line_nine" ".*Note.*also.*Breakpoint 7.*" "Breakpoint 7 at *ending-run.c:14"
+        gdb_test "cle" ".*Deleted breakpoints 6 7.*" "Clear 2 by default"
     }
     -re ".*$gdb_prompt $" {
         fail "need to fix test for new compile outcome"
Index: gdb.mi/mi-break.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-break.exp,v
retrieving revision 1.15
diff -u -p -r1.15 mi-break.exp
--- gdb.mi/mi-break.exp	1 Feb 2008 16:24:47 -0000	1.15
+++ gdb.mi/mi-break.exp	2 Feb 2008 01:09:31 -0000
@@ -87,12 +87,12 @@ proc test_tbreak_creation_and_listing {}
              "insert temp breakpoint at basics.c:callee2"
 
     mi_gdb_test "444-break-insert -t basics.c:$line_callee3_head" \
-            "444\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",${fullname},line=\"$line_callee3_head\",times=\"0\"\}" \
-             "insert temp breakpoint at basics.c:\$line_callee3_body"
+            "444\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",${fullname},line=\"$line_callee3_body\",times=\"0\"\}" \
+             "insert temp breakpoint at basics.c:\$line_callee3_head"
 
     # Getting the quoting right is tricky.  That is "\"<file>\":$line_callee4_head"
     mi_gdb_test "555-break-insert -t \"\\\"${srcfile}\\\":$line_callee4_head\"" \
-            "555\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",${fullname},line=\"$line_callee4_head\",times=\"0\"\}" \
+            "555\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",${fullname},line=\"$line_callee4_body\",times=\"0\"\}" \
              "insert temp breakpoint at \"<fullfilename>\":\$line_callee4_head"
 
     mi_gdb_test "666-break-list" \
Index: gdb.mi/mi-simplerun.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-simplerun.exp,v
retrieving revision 1.17
diff -u -p -r1.17 mi-simplerun.exp
--- gdb.mi/mi-simplerun.exp	1 Jan 2008 22:53:20 -0000	1.17
+++ gdb.mi/mi-simplerun.exp	2 Feb 2008 01:09:32 -0000
@@ -52,7 +52,9 @@ proc test_breakpoints_creation_and_listi
     global hex
 
     set line_callee4_head  [gdb_get_line_number "callee4 ("]
+    set line_callee4_body  [expr $line_callee4_head + 2]
     set line_callee3_head  [gdb_get_line_number "callee3 ("]
+    set line_callee3_body  [expr $line_callee3_head + 2]
     set line_callee2_head  [gdb_get_line_number "callee2 ("]
     set line_callee2_body  [expr $line_callee2_head + 2]
     set line_main_head     [gdb_get_line_number "main ("]
@@ -75,11 +77,11 @@ proc test_breakpoints_creation_and_listi
              "insert breakpoint at basics.c:callee2"
 
     mi_gdb_test "202-break-insert basics.c:$line_callee3_head" \
-             "202\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",line=\"$line_callee3_head\",times=\"0\"\}" \
+             "202\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",line=\"$line_callee3_body\",times=\"0\"\}" \
              "insert breakpoint at basics.c:\$line_callee3_head"
 
     mi_gdb_test "203-break-insert \"\\\"${srcfile}\\\":$line_callee4_head\"" \
-             "203\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",line=\"$line_callee4_head\",times=\"0\"\}" \
+             "203\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",line=\"$line_callee4_body\",times=\"0\"\}" \
              "insert breakpoint at \"<fullfilename>\":\$line_callee4_head"
 
     mi_gdb_test "204-break-list" \
Index: gdb.mi/mi2-break.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi2-break.exp,v
retrieving revision 1.8
diff -u -p -r1.8 mi2-break.exp
--- gdb.mi/mi2-break.exp	1 Jan 2008 22:53:20 -0000	1.8
+++ gdb.mi/mi2-break.exp	2 Feb 2008 01:09:33 -0000
@@ -88,12 +88,12 @@ proc test_tbreak_creation_and_listing {}
              "insert temp breakpoint at basics.c:callee2"
 
     mi_gdb_test "444-break-insert -t basics.c:$line_callee3_head" \
-             "444\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",${fullname},line=\"$line_callee3_head\",times=\"0\"\}" \
-             "insert temp breakpoint at basics.c:\$line_callee3_body"
+             "444\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",${fullname},line=\"$line_callee3_body\",times=\"0\"\}" \
+             "insert temp breakpoint at basics.c:\$line_callee3_head"
 
     # Getting the quoting right is tricky.  That is "\"<file>\":$line_callee4_head"
     mi_gdb_test "555-break-insert -t \"\\\"${srcfile}\\\":$line_callee4_head\"" \
-             "555\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",${fullname},line=\"$line_callee4_head\",times=\"0\"\}" \
+             "555\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"del\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",${fullname},line=\"$line_callee4_body\",times=\"0\"\}" \
              "insert temp breakpoint at \"<fullfilename>\":\$line_callee4_head"
 
     mi_gdb_test "666-break-list" \
Index: gdb.mi/mi2-simplerun.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi2-simplerun.exp,v
retrieving revision 1.8
diff -u -p -r1.8 mi2-simplerun.exp
--- gdb.mi/mi2-simplerun.exp	1 Jan 2008 22:53:20 -0000	1.8
+++ gdb.mi/mi2-simplerun.exp	2 Feb 2008 01:09:34 -0000
@@ -52,7 +52,9 @@ proc test_breakpoints_creation_and_listi
     global hex
 
     set line_callee4_head  [gdb_get_line_number "callee4 ("]
+    set line_callee4_body  [expr $line_callee4_head + 2]
     set line_callee3_head  [gdb_get_line_number "callee3 ("]
+    set line_callee3_body  [expr $line_callee3_head + 2]
     set line_callee2_head  [gdb_get_line_number "callee2 ("]
     set line_callee2_body  [expr $line_callee2_head + 2]
     set line_main_head     [gdb_get_line_number "main ("]
@@ -75,11 +77,11 @@ proc test_breakpoints_creation_and_listi
              "insert breakpoint at basics.c:callee2"
 
     mi_gdb_test "202-break-insert basics.c:$line_callee3_head" \
-             "202\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",line=\"$line_callee3_head\",times=\"0\"\}" \
+             "202\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee3\",file=\".*basics.c\",line=\"$line_callee3_body\",times=\"0\"\}" \
              "insert breakpoint at basics.c:\$line_callee3_head"
 
     mi_gdb_test "203-break-insert \"\\\"${srcfile}\\\":$line_callee4_head\"" \
-             "203\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",line=\"$line_callee4_head\",times=\"0\"\}" \
+             "203\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"callee4\",file=\".*basics.c\",line=\"$line_callee4_body\",times=\"0\"\}" \
              "insert breakpoint at \"<fullfilename>\":\$line_callee4_head"
 
     mi_gdb_test "204-break-list" \