[review] jit: remove bp locations when unregistering jit code
Mihails Strasuns (Code Review)
gerrit@gnutoolchain-gerrit.osci.io
Mon Nov 25 12:54:00 GMT 2019
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................
jit: remove bp locations when unregistering jit code
Fixes the rare problem when identical JIT code is registered and
unregistered several times and its symbols happen to be relocated to the
exactly the same addresses as previous time. In such situation gdb would
still think that old breakpoint locations are still inserted (because
`inserted` flag is set and objfile/address is the same) despite the fact
that code memory was overwritten and does not contain a breakpoint trap
anymore.
This solution removes such breakpoint location from the respective
breakpoint location list. That way next time
`update_breakpoint_locations` is called, it will re-create a new
bp_location instance if that JIT code was loaded again, and won't try
reusing the previous one. It is a conservative solution which only
applies to object files coming from JIT.
Added test case tries to reproduce the problem using the opencl test
suite (because current jit test suite doesn't actually execute the
code). Without this patch the breakpoint would only trigger during the first
kernel run and get completely ignored second run.
gdb/ChangeLog:
2019-11-14 Mihails Strasuns <mihails.strasuns@intel.com>
* breakpoint.h, breakpoint.c (forget_breakpoint_locations_obj): new
function to forget all breakpoint locations for a given objfile.
* jit.c (jit_unregister_code): call `forget_breakpoint_locations_obj`.
Signed-off-by: Mihails Strasuns <mihails.strasuns@intel.com>
Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
---
M gdb/breakpoint.c
M gdb/breakpoint.h
M gdb/jit.c
A gdb/testsuite/gdb.opencl/multirun.exp
M gdb/testsuite/lib/opencl_hostapp.c
5 files changed, 149 insertions(+), 2 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 583f46d..a184e46 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3063,6 +3063,48 @@
}
}
+/* Tries to find `bp` in the linked list of `b` locations
+ and if found, removes it. In practice that means that breakpoint
+ will forget about this location and won't try to re-insert it. */
+
+static bool
+remove_location_from_bp (breakpoint *b, bp_location *bp)
+{
+ if (b->loc == bp)
+ {
+ b->loc = nullptr;
+ return true;
+ }
+
+ bp_location *i;
+ for (i = b->loc; i != nullptr && i->next != bp; i = i->next)
+ ;
+ if (i == nullptr)
+ return false;
+ i->next = i->next->next;
+ return true;
+}
+
+/* See breakpoint.h. */
+
+int
+forget_breakpoint_locations_obj (objfile *obj)
+{
+ struct bp_location **blp, *bl;
+
+ int count = 0;
+
+ ALL_BP_LOCATIONS (bl, blp)
+ if (bl->symtab != NULL && SYMTAB_OBJFILE (bl->symtab) == obj)
+ {
+ bool ret = remove_location_from_bp (bl->owner, bl);
+ gdb_assert (ret);
+ ++count;
+ }
+
+ return count;
+}
+
static int internal_breakpoint_number = -1;
/* Set the breakpoint number of B, depending on the value of INTERNAL.
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5c8f17c..5fa99e9 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1405,6 +1405,13 @@
extern void remove_breakpoints_inf (inferior *inf);
+/* Forgets all breakpoint locations that are inserted
+ for a given objfile by removing them from the owning breakpoint
+ linked list. Will not affect actual breakpoints and
+ if the object file remains these may be re-created during next
+ `update_breakpoint_locations` call. */
+extern int forget_breakpoint_locations_obj (objfile* obj);
+
/* This function can be used to update the breakpoint package's state
after an exec() system call has been executed.
diff --git a/gdb/jit.c b/gdb/jit.c
index 5014dfd..9c5a92c 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -960,6 +960,11 @@
{
if (jit_debug)
fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%p)\n", objfile);
+ int count = forget_breakpoint_locations_obj (objfile);
+ if (jit_debug > 1 && count > 0)
+ fprintf_unfiltered (
+ gdb_stdlog,
+ "ignoring %d breakpoint locations in the unregistered code\n", count);
delete objfile;
}
diff --git a/gdb/testsuite/gdb.opencl/multirun.exp b/gdb/testsuite/gdb.opencl/multirun.exp
new file mode 100644
index 0000000..48e270c
--- /dev/null
+++ b/gdb/testsuite/gdb.opencl/multirun.exp
@@ -0,0 +1,83 @@
+# Copyright 2019 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/>. */
+#
+# Tests global symbol lookup in case a symbol's name in the kernel
+# coincides with another in the main executable.
+
+load_lib opencl.exp
+
+if { [skip_opencl_tests] } {
+ return 0
+}
+
+set name "opencl_kernel"
+set app [remote_download target ${srcdir}/lib/${name}.cl]
+if { [gdb_compile_opencl_hostapp "${app}" "${name}" "additional_flags=-DRUN_COUNT=2" ] != "" } {
+ untested "failed to compile"
+ return -1
+}
+
+clean_restart ${name}
+
+# Get to the kernel first
+gdb_breakpoint "testkernel" {allow-pending} {temporary}
+gdb_test "set debug jit 2"
+gdb_test "start"
+gdb_continue_to_breakpoint "testkernel"
+
+# First run
+gdb_breakpoint "testkernel"
+set count_first 1
+while 1 {
+ gdb_test_multiple "continue" "first run" {
+ -re "Breakpoint" {
+ incr count_first +1
+ pass "expected breakpount"
+ }
+ -re "jit_unregister_code" {
+ pass "end of first run"
+ break
+ }
+ -re "exited" {
+ fail "unexpected temrination"
+ break
+ }
+ }
+}
+
+# Second run
+set count_second 0
+while 1 {
+ gdb_test_multiple "continue" "second run" {
+ -re "Breakpoint" {
+ incr count_second +1
+ pass "expected breakpount"
+ }
+ -re "jit_unregister_code" {
+ pass "end of second run"
+ break
+ }
+ -re "exited" {
+ fail "unexpected temrination"
+ break
+ }
+ }
+}
+
+if [expr $count_first==$count_second] then {
+ pass "same count of breakpoint hits between two runs"
+} else {
+ fail "${count_first} != ${count_second}"
+}
\ No newline at end of file
diff --git a/gdb/testsuite/lib/opencl_hostapp.c b/gdb/testsuite/lib/opencl_hostapp.c
index 9d6ce0e..7d63d09 100644
--- a/gdb/testsuite/lib/opencl_hostapp.c
+++ b/gdb/testsuite/lib/opencl_hostapp.c
@@ -32,13 +32,17 @@
#error "Please specify the OpenCL source file using the CL_SOURCE define"
#endif
+#ifndef RUN_COUNT
+#define RUN_COUNT 1
+#endif
+
#define STRINGIFY(S) _STRINGIFY(S)
#define _STRINGIFY(S) #S
#define SIZE 16
-int
-main ()
+static void
+run_once ()
{
int err, i;
cl_platform_id platform;
@@ -163,6 +167,12 @@
CHK (clReleaseCommandQueue (queue));
CHK (clReleaseContext (context));
free (data);
+}
+int
+main ()
+{
+ for (int i = 0; i < RUN_COUNT; ++i)
+ run_once();
return 0;
}
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-MessageType: newchange
More information about the Gdb-patches
mailing list