[review v3] jit: remove bp locations when unregistering jit code

["Mihails Strasuns (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

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`.

gdb/testsuite/ChangeLog:
2019-11-14  Mihails Strasuns  <mihails.strasuns@intel.com>
	* jit-reregister.exp, jit-reregister.c: new test case to verify that
	  breakpoint is hit even when JIT code is unregistered and registered
	  again.

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.base/jit-reregister.c
A gdb/testsuite/gdb.base/jit-reregister.exp
5 files changed, 181 insertions(+), 9 deletions(-)



diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 904abda..ab1139d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3066,6 +3066,50 @@
   }
 }
 
+/* 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 (is_addr_in_objfile (bl->address, 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 3197854..9105970 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1411,6 +1411,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 0dd11e1..6ed5f51 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -923,6 +923,32 @@
   return NULL;
 }
 
+/* This function unregisters JITed code and does the necessary cleanup.  */
+
+static void
+jit_unregister_code (struct gdbarch *gdbarch, CORE_ADDR entry_addr)
+{
+  struct objfile *objfile = nullptr;
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%s)\n",
+			host_address_to_string (objfile));
+  objfile = jit_find_objf_with_entry_addr (entry_addr);
+  if (objfile == NULL)
+    printf_unfiltered (_ ("Unable to find JITed code "
+			  "entry at address: %s\n"),
+		       paddress (gdbarch, entry_addr));
+  else
+    {
+      int count = forget_breakpoint_locations_obj (objfile);
+      if (jit_debug && count > 0)
+	fprintf_unfiltered (gdb_stdlog,
+			    "jit_unregister_code, ignoring %d breakpoint"
+			    "locations in the unregistered code\n",
+			    count);
+      objfile->unlink ();
+    }
+}
+
 /* This is called when a breakpoint is deleted.  It updates the
    inferior's cache, if needed.  */
 
@@ -1333,7 +1359,6 @@
   struct jit_descriptor descriptor;
   struct jit_code_entry code_entry;
   CORE_ADDR entry_addr;
-  struct objfile *objf;
 
   /* Read the descriptor from remote memory.  */
   if (!jit_read_descriptor (gdbarch, &descriptor,
@@ -1351,14 +1376,7 @@
       jit_register_code (gdbarch, entry_addr, &code_entry);
       break;
     case JIT_UNREGISTER:
-      objf = jit_find_objf_with_entry_addr (entry_addr);
-      if (objf == NULL)
-	printf_unfiltered (_("Unable to find JITed code "
-			     "entry at address: %s\n"),
-			   paddress (gdbarch, entry_addr));
-      else
-	objf->unlink ();
-
+      jit_unregister_code (gdbarch, entry_addr);
       break;
     default:
       error (_("Unknown action_flag value in JIT descriptor!"));
diff --git a/gdb/testsuite/gdb.base/jit-reregister.c b/gdb/testsuite/gdb.base/jit-reregister.c
new file mode 100644
index 0000000..3d63f14
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-reregister.c
@@ -0,0 +1,58 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#include "jit-defs.h"
+#include "jit-elf.h"
+
+int
+main (int argc, char *argv[])
+{
+  size_t obj_size;
+  ElfW (Addr) addr = load_elf (argv[1], &obj_size, 0);
+  update_locations (addr, -1);
+  int (*jit_function) ()
+      = (int (*) ()) load_symbol (addr, "jit_function_XXXX");
+
+  struct jit_code_entry *const entry
+      = (struct jit_code_entry *) calloc (1, sizeof (*entry));
+
+  entry->symfile_addr = (const char *) addr;
+  entry->symfile_size = obj_size;
+  entry->prev_entry = __jit_debug_descriptor.relevant_entry;
+  __jit_debug_descriptor.relevant_entry = entry;
+  __jit_debug_descriptor.first_entry = entry;
+  __jit_debug_descriptor.action_flag = JIT_REGISTER_FN;
+  __jit_debug_register_code ();
+
+  jit_function (); /* first-call */
+
+  __jit_debug_descriptor.action_flag = JIT_UNREGISTER_FN;
+  __jit_debug_register_code ();
+
+  addr = load_elf (argv[1], &obj_size, addr);
+  update_locations (addr, -1);
+  jit_function = (int (*) ()) load_symbol (addr, "jit_function_XXXX");
+
+  entry->symfile_addr = (const char *) addr;
+  entry->symfile_size = obj_size;
+  __jit_debug_descriptor.relevant_entry = entry;
+  __jit_debug_descriptor.first_entry = entry;
+  __jit_debug_descriptor.action_flag = JIT_REGISTER_FN;
+  __jit_debug_register_code ();
+
+  jit_function ();
+}
diff --git a/gdb/testsuite/gdb.base/jit-reregister.exp b/gdb/testsuite/gdb.base/jit-reregister.exp
new file mode 100644
index 0000000..23de587
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-reregister.exp
@@ -0,0 +1,45 @@
+# 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.
+
+standard_testfile .c
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set solib "jit-solib"
+set solib_src "${srcdir}/${subdir}/${solib}.c"
+set solib_bin [standard_output_file ${solib}.so]
+
+if { [gdb_compile_shlib "${solib_src}" "${solib_bin}" {debug}] != "" } {
+    untested "failed to compile jit shared library"
+    return -1
+}
+
+set solib_bin_target [gdb_remote_download target ${solib_bin}]
+
+clean_restart ${binfile}
+
+gdb_test "start ${solib_bin_target}"
+gdb_breakpoint [gdb_get_line_number "first-call"] {temporary}
+gdb_test "set debug jit 1"
+gdb_continue_to_breakpoint "first-call"
+
+gdb_breakpoint "jit_function_XXXX"
+gdb_continue_to_breakpoint "jit_function_XXXX"
+gdb_continue_to_breakpoint "jit_function_XXXX"
\ No newline at end of file

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 3
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset