[2/2] gdb, linespec: reject inserting breakpoints for both entry and prologue end PC

Message ID 20241018102156.350310-3-klaus.gerlicher@intel.com
State New
Headers
Series some amendments to rejecting inserting breakpoints between functions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Klaus Gerlicher Oct. 18, 2024, 10:21 a.m. UTC
  From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>

This patch extends "gdb: reject inserting breakpoints between functions".

We are keeping BPs from sliding into non-related blocks when they would slide
into a line with an entry PC.  However this is not enough to prevent all
BP slides into non-related blocks since the compiler can also generate
a prologue end PC that would be used.

To avoid this also treat the prologue end PC the same as the entry PC.
---
 gdb/linespec.c                           | 41 ++++++++++++++++--
 gdb/testsuite/gdb.linespec/bad-slide.c   | 41 ++++++++++++++++++
 gdb/testsuite/gdb.linespec/bad-slide.exp | 54 ++++++++++++++++++++++++
 3 files changed, 133 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.linespec/bad-slide.c
 create mode 100644 gdb/testsuite/gdb.linespec/bad-slide.exp
  

Comments

Alexandra Petlanova Hajkova Oct. 21, 2024, 10:29 a.m. UTC | #1
On Fri, Oct 18, 2024 at 12:22 PM Klaus Gerlicher <klaus.gerlicher@intel.com>
wrote:

> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
>
> This patch extends "gdb: reject inserting breakpoints between functions".
>
> We are keeping BPs from sliding into non-related blocks when they would
> slide
> into a line with an entry PC.  However this is not enough to prevent all
> BP slides into non-related blocks since the compiler can also generate
> a prologue end PC that would be used.
>
> To avoid this also treat the prologue end PC the same as the entry PC.
> ---
> Great fix, I can also confirm this change adds no regressions for
> Fedora Rawhide x86_64.
>
> Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
>
  

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index a2723e1c06a..bb262b180b1 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2154,13 +2154,48 @@  create_sals_line_offset (struct linespec_state *self,
 	       The intent of this heuristic is that a breakpoint requested on
 	       line 11 and 12 will not result in a breakpoint on main, but a
 	       breakpoint on line 13 will.  A breakpoint requested on the empty
-	       line 16 will also result in a breakpoint in main, at line 17.  */
+	       line 16 will also result in a breakpoint in main, at line 17.
+
+	       Also consider that there may be a separate line table entry
+	       for the same line at the prologue end.  Treat this the
+	       same as the entry PC.
+
+	       01
+	       02 int
+	       03 main () { { int i = 1; }
+	       04   return 0;
+	       05 }
+
+	       will generate linetable entries like this where line 3 has two
+	       entries because of the extra code block.  The first one is
+	       the entry pc, the second one is the end of the prologue.
+
+	       INDEX  LINE   REL-ADDRESS        UNREL-ADDRESS      IS-STMT P-END
+	       0      3      0x0000555555555130 0x0000000000001130 Y
+	       1      3      0x000055555555513b 0x000000000000113b Y       Y
+	       2      4      0x0000555555555142 0x0000000000001142 Y
+	       3      END    0x0000555555555146 0x0000000000001146 Y
+
+	       The second entry would still be considered a valid location,
+	       therefore this should also be skipped in the same way as
+	       the entry pc.  */
+
 	    if (!was_exact
 		&& sym != nullptr
 		&& sym->aclass () == LOC_BLOCK
-		&& sal.pc == sym->value_block ()->entry_pc ()
 		&& val.line < sym->line ())
-	      continue;
+	      {
+		std::optional<CORE_ADDR> prologue_end;
+
+		if (self->funfirstline)
+		  prologue_end = skip_prologue_using_sal (
+		    sym->arch (), sym->value_block ()->entry_pc ());
+
+		if (sal.pc == sym->value_block ()->entry_pc ()
+		    || (self->funfirstline && prologue_end.has_value ()
+			&& sal.pc == *prologue_end))
+		  continue;
+	      }
 
 	    if (self->funfirstline)
 	      skip_prologue_sal (&sal);
diff --git a/gdb/testsuite/gdb.linespec/bad-slide.c b/gdb/testsuite/gdb.linespec/bad-slide.c
new file mode 100644
index 00000000000..49d842c037d
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/bad-slide.c
@@ -0,0 +1,41 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2024 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/>.
+
+   Source code has intentionally been formatted not according to GNU style
+   to show issues with breakpoint propagation/sliding.  */
+
+/* break here one.  */
+void one () { int var = 0;
+}
+
+/* break here two.  */
+void two () { {int var = 0;} } /* func line two.  */
+
+/* break here three.  */
+void three () { /* func line three.  */
+ {int var = 0;} /* func body three.  */
+}
+
+int
+main (void)
+{
+  one ();
+  two ();
+  three ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.linespec/bad-slide.exp b/gdb/testsuite/gdb.linespec/bad-slide.exp
new file mode 100644
index 00000000000..aa01da2b1f4
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/bad-slide.exp
@@ -0,0 +1,54 @@ 
+# Copyright (C) 2024 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/>.
+#
+# Test the breakpoint rejection for breakpoints between functions.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
+	  { debug }] } {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_test_no_output "set breakpoint pending off"
+
+set bp_line [gdb_get_line_number "break here one"]
+gdb_test "break $bp_line" \
+    ".*No compiled code for line $bp_line in the current file."
+
+set bp_line [gdb_get_line_number "break here two"]
+gdb_test "break $bp_line" \
+    ".*No compiled code for line $bp_line in the current file."
+
+set bp_line [gdb_get_line_number "func line two"]
+gdb_test "break $bp_line" \
+    ".*Breakpoint $decimal at .*$srcfile,.*line $bp_line.*"
+
+set bp_line [gdb_get_line_number "break here three"]
+gdb_test "break $bp_line" \
+    ".*No compiled code for line $bp_line in the current file."
+
+set bp_line [gdb_get_line_number "func line three"]
+set bp_line_prop [expr $bp_line + 1]
+gdb_test "break $bp_line" \
+    ".*Breakpoint $decimal at .*$srcfile,.*line $bp_line_prop.*"
+
+set bp_line [gdb_get_line_number "func body three"]
+gdb_test "break $bp_line" \
+    ".*Breakpoint $decimal at .*$srcfile,.*line $bp_line.*"