RE: [PATCH] gdb: reject inserting breakpoints between functions

["Gerlicher, Klaus" <klaus.gerlicher@intel.com>]

Hi Simon,

Kindly reminding again about your breakpoint sliding patch. 

I worked a bit with it, I have had one additional issue that would be fixed with below patch but of course since your original patch hasn't landed yet I can't post this as a real patch. 

So if you want to integrate this to your patch or treat this in another way, please let me know.

Thanks
Klaus

From 4027e5cd0fafe0079ec27efb65f1b073fa951fb5 Mon Sep 17 00:00:00 2001
From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
Date: Wed, 24 Jul 2024 13:46:34 +0000
Subject: [PATCH] gdb, linespec: reject inserting breakpoints for both entry
 and prologue end PC

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

diff --git a/gdb/linespec.c b/gdb/linespec.c
index e6e3228395e..f51df07342e 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2193,13 +2193,48 @@ create_sals_line_offset (struct linespec_state *self,
 	       on line 11 and 12 will not result on 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.  */
+	       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;
+	      {
+		gdb::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.*"
-- 
2.34.1






> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Wednesday, May 1, 2024 8:12 PM
> To: Andrew Burgess <aburgess@redhat.com>; Gerlicher, Klaus
> <klaus.gerlicher@intel.com>; simon.marchi@efficios.com
> Cc: Simon.Marchi@amd.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH] gdb: reject inserting breakpoints between functions
> 
> 
> 
> On 2024-05-01 05:47, Andrew Burgess wrote:
> > Klaus Gerlicher <klaus.gerlicher@intel.com> writes:
> >
> >> Hi Simon,
> >>
> >> I verified that your patch addresses many of the issues we would also
> >> like to solve in this area.
> >>
> >> It appears this has not published yet and it seems to be more than 2
> >> years old.
> >>
> >> Could you please tell me if there are any plans to commit this?
> >
> > Given I reviewed this once upon a time, I still had my review branch
> > kicking around.
> >
> > I rebased onto something close to HEAD of master.  I addressed the minor
> > nits I pointed out in my review.  I'd be happy to see this merged once
> > my full regression run (still on going) has completed.
> >
> > I think I'd like Simon to give a +1 before I pushed this though.
> >
> > Thanks,
> > Andrew
> 
> Thanks for reminding me about this, it clearly fell through the cracks.
> And thanks Andrew for following up on it.  What you posted looks good to
> me.  We discussed it internally, Pedro suggested to maybe wait after the
> GDB 15 branch is created before pushing this one, to give it more
> testing time in master before it reaches a release.
> 
> Simon
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928