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

Gerlicher, Klaus
2024-08-07T08:28:37Z

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