[PATCH 0/2] fix missed breakpoint upon jit re-registration

[PATCH 0/2] fix missed breakpoint upon jit re-registration

[Mihails Strasuns]

Same as the patch previously submitted on gerrit but rebased and with a test
case reworked based on changes recently merged. Without the fix the last
breakpoint hit test case will FAIL. Issue was originally found when debugging
an OpenCL program.

Some explanation of the issue is in the second commit. Alternative would be to
remove all breakpoint locations upon any destruction of an object file but I
don't understand the implications well enough to call for it myself.

Mihails Strasuns (2):
  [gdb/testsuite] optional flags for compile_and_download_n_jit_so
  jit: remove bp locations when unregistering jit code

 gdb/breakpoint.c                              | 44 +++++++++++
 gdb/breakpoint.h                              |  7 ++
 gdb/jit.c                                     | 36 ++++++---
 gdb/testsuite/gdb.base/jit-elf-reregister.c   | 66 ++++++++++++++++
 gdb/testsuite/gdb.base/jit-elf-reregister.exp | 77 +++++++++++++++++++
 gdb/testsuite/lib/jit-elf-helpers.exp         |  8 +-
 6 files changed, 227 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/jit-elf-reregister.c
 create mode 100644 gdb/testsuite/gdb.base/jit-elf-reregister.exp

-- 
2.26.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 1/2] [gdb/testsuite] optional flags for compile_and_download_n_jit_so

[Mihails Strasuns]

Allows to pass in optional extra compilation flags to
compile_and_download_n_jit_so function.

gdb/testsuite/ChangeLog:

2020-05-13  Mihails Strasuns  <mihails.strasuns@intel.com>

	* lib/jit-elf-helpers.exp (compile_and_download_n_jit_so): new
	  optional argument for compiler flags
---
 gdb/testsuite/lib/jit-elf-helpers.exp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/jit-elf-helpers.exp b/gdb/testsuite/lib/jit-elf-helpers.exp
index ab647abe50..2f6aa73414 100644
--- a/gdb/testsuite/lib/jit-elf-helpers.exp
+++ b/gdb/testsuite/lib/jit-elf-helpers.exp
@@ -72,9 +72,12 @@ proc compile_jit_elf_main_as_so {main_solib_srcfile main_solib_binfile options}
 # Compile jit-elf-solib.c as a shared library in multiple copies and
 # upload them to the target.
 #
+# EXTRA_OPTIONS may be passed as an optional argument to be added to
+# the compilation options.
+#
 # On success, return a list of target path to the shared libraries.
 # On failure, return -1.
-proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count} {
+proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count {extra_options}} {
     global jit_load_address jit_load_increment
     set binfiles_target {}
 
@@ -93,7 +96,8 @@ proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count}
 	set options [list \
 	    additional_flags=-DFUNCTION_NAME=[format "jit_function_%04d" $i] \
 	    additional_flags=-Xlinker \
-	    additional_flags=-Ttext-segment=$addr]
+	    additional_flags=-Ttext-segment=$addr \
+	    $extra_options]
 	if { [gdb_compile_shlib ${jit_solib_srcfile} ${binfile} $options] != "" } {
 	    untested "failed to compile ${jit_solib_basename}.c as a shared library"
 	    return -1
-- 
2.26.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/2] [gdb/testsuite] optional flags for compile_and_download_n_jit_so

[Simon Marchi]

On 2020-05-12 7:33 a.m., Mihails Strasuns via Gdb-patches wrote:
> Allows to pass in optional extra compilation flags to
> compile_and_download_n_jit_so function.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-05-13  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* lib/jit-elf-helpers.exp (compile_and_download_n_jit_so): new
> 	  optional argument for compiler flags
> ---
>  gdb/testsuite/lib/jit-elf-helpers.exp | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/jit-elf-helpers.exp b/gdb/testsuite/lib/jit-elf-helpers.exp
> index ab647abe50..2f6aa73414 100644
> --- a/gdb/testsuite/lib/jit-elf-helpers.exp
> +++ b/gdb/testsuite/lib/jit-elf-helpers.exp
> @@ -72,9 +72,12 @@ proc compile_jit_elf_main_as_so {main_solib_srcfile main_solib_binfile options}
>  # Compile jit-elf-solib.c as a shared library in multiple copies and
>  # upload them to the target.
>  #
> +# EXTRA_OPTIONS may be passed as an optional argument to be added to
> +# the compilation options.
> +#
>  # On success, return a list of target path to the shared libraries.
>  # On failure, return -1.
> -proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count} {
> +proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count {extra_options}} {
>      global jit_load_address jit_load_increment
>      set binfiles_target {}
>  
> @@ -93,7 +96,8 @@ proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count}
>  	set options [list \
>  	    additional_flags=-DFUNCTION_NAME=[format "jit_function_%04d" $i] \
>  	    additional_flags=-Xlinker \
> -	    additional_flags=-Ttext-segment=$addr]
> +	    additional_flags=-Ttext-segment=$addr \
> +	    $extra_options]

If extra_options is meant to be a list, I don't think that will work when
it will contain multiple items:

% set extra_options {debug something-else}
debug something-else
% set options [list hello $extra_options]
hello {debug something-else}

You can do it with concat:

% set extra_options {debug something-else}
debug something-else
% set options [list hello]
hello
% set options [concat $options $extra_options]
hello debug something-else

Otherwise, this patch LGTM.

Simon

[PATCH 2/2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns]

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.

gdb/ChangeLog:
2020-05-12  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:
2020-05-12  Mihails Strasuns  <mihails.strasuns@intel.com>
	* jit-elf-reregister.exp, jit-elf-reregister.c: new test case to verify
	  that breakpoint is hit even when JIT code is unregistered and
	  registered again.
---
 gdb/breakpoint.c                              | 44 +++++++++++
 gdb/breakpoint.h                              |  7 ++
 gdb/jit.c                                     | 36 ++++++---
 gdb/testsuite/gdb.base/jit-elf-reregister.c   | 66 ++++++++++++++++
 gdb/testsuite/gdb.base/jit-elf-reregister.exp | 77 +++++++++++++++++++
 5 files changed, 221 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/jit-elf-reregister.c
 create mode 100644 gdb/testsuite/gdb.base/jit-elf-reregister.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 22ddb3d5e8..33d93b8bc0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3068,6 +3068,50 @@ remove_breakpoints_inf (inferior *inf)
   }
 }
 
+/* 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 347aeb75f3..c609ee4f26 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1406,6 +1406,13 @@ extern int remove_breakpoints (void);
 
 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 07e9ce7ae2..a76fc8adf1 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -925,6 +925,32 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
   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.  */
 
@@ -1335,7 +1361,6 @@ jit_event_handler (struct gdbarch *gdbarch)
   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,
@@ -1353,14 +1378,7 @@ jit_event_handler (struct gdbarch *gdbarch)
       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-elf-reregister.c b/gdb/testsuite/gdb.base/jit-elf-reregister.c
new file mode 100644
index 0000000000..2416259dcf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-elf-reregister.c
@@ -0,0 +1,66 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2020 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-protocol.h"
+#include "jit-elf-util.h"
+
+/* Must be defined by .exp file when compiling to know
+   what address to map the ELF binary to.  */
+#ifndef LOAD_ADDRESS
+#error "Must define LOAD_ADDRESS"
+#endif
+
+int
+main (int argc, char *argv[])
+{
+  /* Used as backing storage for GDB to populate argv.  */
+  char *fake_argv[2];
+  
+  size_t obj_size;
+  void *load_addr = (void *) (size_t) LOAD_ADDRESS;
+  void *addr = load_elf (argv[1], &obj_size, load_addr);
+  int (*jit_function) ()
+      = (int (*) ()) load_symbol (addr, "jit_function_0001");
+
+  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;
+  __jit_debug_register_code ();
+
+  jit_function (); /* first-call */
+
+  __jit_debug_descriptor.action_flag = JIT_UNREGISTER;
+  __jit_debug_register_code ();
+
+  addr = load_elf (argv[1], &obj_size, addr);
+  jit_function = (int (*) ()) load_symbol (addr, "jit_function_0001");
+
+  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;
+  __jit_debug_register_code ();
+
+  jit_function ();
+}
diff --git a/gdb/testsuite/gdb.base/jit-elf-reregister.exp b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
new file mode 100644
index 0000000000..76eb6e0c75
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
@@ -0,0 +1,77 @@
+# 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.
+
+if {[skip_shlib_tests]} {
+    untested "skipping shared library tests"
+    return -1
+}
+
+if {[get_compiler_info]} {
+    untested "could not get compiler info"
+    return 1
+}
+
+load_lib jit-elf-helpers.exp
+
+# The main code that loads and registers JIT objects.
+set main_basename "jit-elf-reregister"
+set main_srcfile ${srcdir}/${subdir}/${main_basename}.c
+set main_binfile [standard_output_file ${main_basename}]
+
+# The shared library that gets loaded as JIT objects.
+set jit_solib_basename jit-elf-solib
+set jit_solib_srcfile ${srcdir}/${subdir}/${jit_solib_basename}.c
+set solib "jit-solib"
+set solib_src "${srcdir}/${subdir}/${solib}.c"
+set solib_bin [standard_output_file ${solib}.so]
+
+# Compile one shared library to use as JIT object.
+set jit_solibs_target [compile_and_download_n_jit_so \
+		      $jit_solib_basename $jit_solib_srcfile 1 {debug}]
+if { $jit_solibs_target == -1 } {
+    return
+}
+
+# Compile the main code (which loads the JIT objects).
+if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] == 0 } {
+    clean_restart ${main_binfile}
+
+    if {$verbose > 0} {
+	gdb_test "set debug jit 1"
+     }
+
+    if { ![runto_main] } {
+	fail "can't run to main"
+	return
+    }
+
+    # Poke desired values directly into inferior instead of using "set args"
+    # because "set args" does not work under gdbserver.
+    gdb_test_no_output "set var argc=1" "forging argc"
+    gdb_test_no_output "set var argv=fake_argv" "forging argv"
+    set jit_solib_target [lindex $jit_solibs_target 0]
+    gdb_test_no_output "set var argv\[1\]=\"${jit_solib_target}\"" \
+    "forging argv\[1\]"
+
+    gdb_breakpoint [gdb_get_line_number "first-call" $main_srcfile] {temporary}
+    gdb_continue_to_breakpoint "first-call"
+    gdb_breakpoint "jit_function_0001"
+    # not using gdb_continue_to_breakpoint to avoid duplicate test output
+    gdb_test "continue" "Breakpoint.*jit_function_0001.*" "hit before reload"
+    gdb_test "continue" "Breakpoint.*jit_function_0001.*" "hit after reload"
+}
\ No newline at end of file
-- 
2.26.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code

[Simon Marchi]

Hi Mihails,

On 2020-05-12 7:33 a.m., Mihails Strasuns via Gdb-patches wrote:
> 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.


I'm not very familiar with how breakpoint locations are computed and updated
(I just know it's complicated). But I confirm that the test shows the problem
without the fix applied and passes with the fix applied.  I'd like if some
other maintainer more familiar with this area could review this part.

I'd be really curious to understand how GDB handles this when it comes to
shared libraries.  Let's imagine the same case, but using shared libraries.
When a shared library is unloaded, the breakpoint locations that were in
that library must be forgotten somehow?  If so through which hook / mechanism?

Meanwhile, here are some minor comments.

> gdb/ChangeLog:
> 2020-05-12  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:
> 2020-05-12  Mihails Strasuns  <mihails.strasuns@intel.com>
> 	* jit-elf-reregister.exp, jit-elf-reregister.c: new test case to verify
> 	  that breakpoint is hit even when JIT code is unregistered and
> 	  registered again.

Please capitalize the letters at the beginning of the sentences.

In the last entry, the paths should start with `gdb.base/`.

> +/* 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;
> +      }
> +  }

Indent the braces like this (as if it was a for loop):

ALL_BP_LOCATIONS (bl, blp)
  {
    ...
  }

> +
> +  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 347aeb75f3..c609ee4f26 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1406,6 +1406,13 @@ extern int remove_breakpoints (void);
>  
>  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);

Space before asterisk.

> +
>  /* 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 07e9ce7ae2..a76fc8adf1 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -925,6 +925,32 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
>    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

Let's use early return here toi avoid indenting the "normal" path:

  if (objfile == NULL)
    {
      printf_unfiltered (_ ("Unable to find JITed code "
			    "entry at address: %s\n"),
			    paddress (gdbarch, entry_addr));
      return;
    }

> diff --git a/gdb/testsuite/gdb.base/jit-elf-reregister.exp b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
> new file mode 100644
> index 0000000000..76eb6e0c75
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
> @@ -0,0 +1,77 @@
> +# 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.
> +
> +if {[skip_shlib_tests]} {
> +    untested "skipping shared library tests"
> +    return -1
> +}
> +
> +if {[get_compiler_info]} {
> +    untested "could not get compiler info"
> +    return 1
> +}

Is this needed for this test case?  Usually this is needed when we need
to check which compiler or compiler version is used.

> +
> +load_lib jit-elf-helpers.exp
> +
> +# The main code that loads and registers JIT objects.
> +set main_basename "jit-elf-reregister"
> +set main_srcfile ${srcdir}/${subdir}/${main_basename}.c
> +set main_binfile [standard_output_file ${main_basename}]
> +
> +# The shared library that gets loaded as JIT objects.
> +set jit_solib_basename jit-elf-solib
> +set jit_solib_srcfile ${srcdir}/${subdir}/${jit_solib_basename}.c
> +set solib "jit-solib"
> +set solib_src "${srcdir}/${subdir}/${solib}.c"
> +set solib_bin [standard_output_file ${solib}.so]

The last three are not used it seems.

> +
> +# Compile one shared library to use as JIT object.
> +set jit_solibs_target [compile_and_download_n_jit_so \
> +		      $jit_solib_basename $jit_solib_srcfile 1 {debug}]
> +if { $jit_solibs_target == -1 } {
> +    return
> +}
> +
> +# Compile the main code (which loads the JIT objects).
> +if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] == 0 } {

Usually, we do early returns, which avoids indenting the rest of the file:

if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] == -1 } {  # or != 0
    return
}

> +    clean_restart ${main_binfile}
> +
> +    if {$verbose > 0} {
> +	gdb_test "set debug jit 1"
> +     }
> +
> +    if { ![runto_main] } {
> +	fail "can't run to main"
> +	return
> +    }
> +
> +    # Poke desired values directly into inferior instead of using "set args"
> +    # because "set args" does not work under gdbserver.
> +    gdb_test_no_output "set var argc=1" "forging argc"
> +    gdb_test_no_output "set var argv=fake_argv" "forging argv"

The situation might change with:

  https://sourceware.org/pipermail/gdb-patches/2020-May/168441.html

Although there's nothing wrong with using `set var`, it works.  Just that the
comment will soon be incorrect.

> +    set jit_solib_target [lindex $jit_solibs_target 0]
> +    gdb_test_no_output "set var argv\[1\]=\"${jit_solib_target}\"" \
> +    "forging argv\[1\]"
> +
> +    gdb_breakpoint [gdb_get_line_number "first-call" $main_srcfile] {temporary}
> +    gdb_continue_to_breakpoint "first-call"
> +    gdb_breakpoint "jit_function_0001"
> +    # not using gdb_continue_to_breakpoint to avoid duplicate test output
> +    gdb_test "continue" "Breakpoint.*jit_function_0001.*" "hit before reload"
> +    gdb_test "continue" "Breakpoint.*jit_function_0001.*" "hit after reload"

gdb_continue_to_breakpoint has a `name` argument used for customizing the test
name, doesn't it?

Simon

RE: [PATCH 2/2] jit: remove bp locations when unregistering jit code

[Strasuns, Mihails]

Hello,

I will focus on the design related question for now:

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Sunday, May 24, 2020 5:02 PM
> To: Strasuns, Mihails <mihails.strasuns@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code
> 
> I'm not very familiar with how breakpoint locations are computed and
> updated (I just know it's complicated). But I confirm that the test shows the
> problem without the fix applied and passes with the fix applied.  I'd like if
> some other maintainer more familiar with this area could review this part.
> 
> I'd be really curious to understand how GDB handles this when it comes to
> shared libraries.  Let's imagine the same case, but using shared libraries.
> When a shared library is unloaded, the breakpoint locations that were in that
> library must be forgotten somehow?  If so through which hook / mechanism?

This is something I have initially looked into myself didn't find a clear answer in GDB sources :-)

bp_location defines this field:

---
  /* This location's address is in an unloaded solib, and so this
     location should not be inserted.  It will be automatically
     enabled when that solib is loaded.  */
  bool shlib_disabled = false;
---

What I don't understand though is that once set to true/1, this field is never reset back to 0. My guess is that this is intended to work in a similar way to my fix, i.e. that a new breakpoint location gets created for the new load rather than the old one being recreated. However I am not entirely sure.

There is a fair amount of differences between semantics of jit and shared libraries, so I decided to start with an independent fix and ask in this mail list for someone who knows better.

Two important notes:

- there is a `disable_breakpoints_in_freed_objfile` hook in breakpoint.c which looks like it should have covered this case but somehow it is not enough.
- the issue in the test case only manifests if reloaded object file uses exactly the same base address and thus breakpoint location looks the same to GDB

BR,
Mihails

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 2/2] jit: remove bp locations when unregistering jit code

[Strasuns, Mihails]

Ping.

> -----Original Message-----
> From: Gdb-patches <gdb-patches-bounces@sourceware.org> On Behalf Of
> Strasuns, Mihails via Gdb-patches
> Sent: Monday, May 25, 2020 12:07 PM
> To: Simon Marchi <simark@simark.ca>; gdb-patches@sourceware.org
> Subject: RE: [PATCH 2/2] jit: remove bp locations when unregistering jit code
> 
> Hello,
> 
> I will focus on the design related question for now:
> 
> > -----Original Message-----
> > From: Simon Marchi <simark@simark.ca>
> > Sent: Sunday, May 24, 2020 5:02 PM
> > To: Strasuns, Mihails <mihails.strasuns@intel.com>; gdb-
> > patches@sourceware.org
> > Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering
> > jit code
> >
> > I'm not very familiar with how breakpoint locations are computed and
> > updated (I just know it's complicated). But I confirm that the test
> > shows the problem without the fix applied and passes with the fix
> > applied.  I'd like if some other maintainer more familiar with this area could
> review this part.
> >
> > I'd be really curious to understand how GDB handles this when it comes
> > to shared libraries.  Let's imagine the same case, but using shared libraries.
> > When a shared library is unloaded, the breakpoint locations that were
> > in that library must be forgotten somehow?  If so through which hook /
> mechanism?
> 
> This is something I have initially looked into myself didn't find a clear answer
> in GDB sources :-)
> 
> bp_location defines this field:
> 
> ---
>   /* This location's address is in an unloaded solib, and so this
>      location should not be inserted.  It will be automatically
>      enabled when that solib is loaded.  */
>   bool shlib_disabled = false;
> ---
> 
> What I don't understand though is that once set to true/1, this field is never
> reset back to 0. My guess is that this is intended to work in a similar way to
> my fix, i.e. that a new breakpoint location gets created for the new load
> rather than the old one being recreated. However I am not entirely sure.
> 
> There is a fair amount of differences between semantics of jit and shared
> libraries, so I decided to start with an independent fix and ask in this mail list
> for someone who knows better.
> 
> Two important notes:
> 
> - there is a `disable_breakpoints_in_freed_objfile` hook in breakpoint.c
> which looks like it should have covered this case but somehow it is not
> enough.
> - the issue in the test case only manifests if reloaded object file uses exactly
> the same base address and thus breakpoint location looks the same to GDB
> 
> BR,
> Mihails
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw Chairperson of the
> Supervisory Board: Nicole Lau Registered Office: Munich Commercial
> Register: Amtsgericht Muenchen HRB 186928
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code

[Simon Marchi]

On 2020-05-25 6:07 a.m., Strasuns, Mihails via Gdb-patches wrote:
> Hello,
> 
> I will focus on the design related question for now:
> 
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Sunday, May 24, 2020 5:02 PM
>> To: Strasuns, Mihails <mihails.strasuns@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code
>>
>> I'm not very familiar with how breakpoint locations are computed and
>> updated (I just know it's complicated). But I confirm that the test shows the
>> problem without the fix applied and passes with the fix applied.  I'd like if
>> some other maintainer more familiar with this area could review this part.
>>
>> I'd be really curious to understand how GDB handles this when it comes to
>> shared libraries.  Let's imagine the same case, but using shared libraries.
>> When a shared library is unloaded, the breakpoint locations that were in that
>> library must be forgotten somehow?  If so through which hook / mechanism?
> 
> This is something I have initially looked into myself didn't find a clear answer in GDB sources :-)
> 
> bp_location defines this field:
> 
> ---
>   /* This location's address is in an unloaded solib, and so this
>      location should not be inserted.  It will be automatically
>      enabled when that solib is loaded.  */
>   bool shlib_disabled = false;
> ---
> 
> What I don't understand though is that once set to true/1, this field is never reset back to 0. My guess is that this is intended to work in a similar way to my fix, i.e. that a new breakpoint location gets created for the new load rather than the old one being recreated. However I am not entirely sure.

I really don't know why, when a shared library containing a location gets
unloaded, we just mark the location as "disabled" and leave it there.  I
really don't see the point of leaving it there.  Maybe there's a comment
somewhere that explains why we do this instead of removing it, but I didn't
find any.

I also find it odd that when a breakpoint location gets disabled, we print
the message "Temporarily disabling breakpoints for $solib".  Why "Temporarily"?
If the same shared lib gets re-loaded, it might or might not be at the same
address, so it's not like the breakpoint location is going to be reused anyway.

I think that's something we can investigate separately from the current patch,
maybe for now we can just disable the locations instead of removing them, to
keep it consistent with shared libs.

> There is a fair amount of differences between semantics of jit and shared libraries, so I decided to start with an independent fix and ask in this mail list for someone who knows better.
> 
> Two important notes:
> 
> - there is a `disable_breakpoints_in_freed_objfile` hook in breakpoint.c which looks like it should have covered this case but somehow it is not enough.

Indeed, that sounds like a promising function that sounds like it should
do what we want (and it does already get called when a jit object gets
unloaded).  But it returns early because of this:

  /* OBJF_SHARED|OBJF_USERLOADED objfiles are dynamic modules manually
     managed by the user with add-symbol-file/remove-symbol-file.
     Similarly to how breakpoints in shared libraries are handled in
     response to "nosharedlibrary", mark breakpoints in such modules
     shlib_disabled so they end up uninserted on the next global
     location list update.  Shared libraries not loaded by the user
     aren't handled here -- they're already handled in
     disable_breakpoints_in_unloaded_shlib, called by solib.c's
     solib_unloaded observer.  We skip objfiles that are not
     OBJF_SHARED as those aren't considered dynamic objects (e.g. the
     main objfile).  */
  if ((objfile->flags & OBJF_SHARED) == 0
      || (objfile->flags & OBJF_USERLOADED) == 0)
    return;

So it sounds like this function considers that because shared libraries
where already taken care of in disable_breakpoints_in_unloaded_shlib, all
that remains is user-loaded objfiles.  But that's not true, there are
jit-loaded objfiles, which are neither user-loaded nor shared libraries.

Looking at disable_breakpoints_in_unloaded_shlib and
disable_breakpoints_in_freed_objfile, I noticed they do make sure that the
loc's pspace is the same as the unloaded shlib / freed objfile's pspace, which
you don't do in your patch.  I just tried with your patch:

- Start two inferiors with one jit object each
- Create a breakpoint with two locations (one in each jit object)
- Step one inferior so that it unloads its jit object

It ended up deleting the breakpoint locations for the two inferiors, whereas
it should have left the location for the inferior that still has its jit object
loaded.  It would be good to enhance the new test case to test that case.

To avoid trying to re-invent the wheel and making the same subtle mistakes that
are (hopefully) already solved for the shared library case, I think we should
try to re-use the same code path for disabling breakpoint locations when either
a shared library or jit object is unloaded.

What about something like this, which makes unloaded jit objects go through
the same breakpoint location disabling function as the unloaded shared libs?


From 6e3a377dc8181ec1ce430711d48037f513b153be Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sat, 6 Jun 2020 22:47:31 -0400
Subject: [PATCH] Disable breakpoint locations in unloaded jit objects

Change-Id: I06ff4f3f02b3c348165ed451339e6effba6e3e97
---
 gdb/breakpoint.c | 29 +++++++++++++++++++++--------
 gdb/jit.c        | 31 ++++++++++++++++++++++---------
 gdb/observable.c |  1 +
 gdb/observable.h |  2 ++
 4 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index aead882acd88..cc15c4730364 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7487,12 +7487,8 @@ disable_breakpoints_in_shlibs (void)
   }
 }

-/* Disable any breakpoints and tracepoints that are in SOLIB upon
-   notification of unloaded_shlib.  Only apply to enabled breakpoints,
-   disabled ones can just stay disabled.  */
-
 static void
-disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
+disable_breakpoints_in_unloaded_objfile (objfile *objfile)
 {
   struct bp_location *loc, **locp_tmp;
   int disabled_shlib_breaks = 0;
@@ -7502,7 +7498,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
     /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
     struct breakpoint *b = loc->owner;

-    if (solib->pspace == loc->pspace
+    if (objfile->pspace == loc->pspace
 	&& !loc->shlib_disabled
 	&& (((b->type == bp_breakpoint
 	      || b->type == bp_jit_event
@@ -7510,7 +7506,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 	     && (loc->loc_type == bp_loc_hardware_breakpoint
 		 || loc->loc_type == bp_loc_software_breakpoint))
 	    || is_tracepoint (b))
-	&& solib_contains_address_p (solib, loc->address))
+	&& is_addr_in_objfile (loc->address, objfile))
       {
 	loc->shlib_disabled = 1;
 	/* At this point, we cannot rely on remove_breakpoint
@@ -7526,13 +7522,29 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 	    target_terminal::ours_for_output ();
 	    warning (_("Temporarily disabling breakpoints "
 		       "for unloaded shared library \"%s\""),
-		     solib->so_name);
+		     objfile->original_name);
 	  }
 	disabled_shlib_breaks = 1;
       }
   }
 }

+/* Disable any breakpoints and tracepoints that are in SOLIB upon
+   notification of unloaded_shlib.  Only apply to enabled breakpoints,
+   disabled ones can just stay disabled.  */
+
+static void
+disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
+{
+  disable_breakpoints_in_unloaded_objfile (solib->objfile);
+}
+
+static void
+disable_breakpoints_in_unloaded_jit_object (objfile *objfile)
+{
+  disable_breakpoints_in_unloaded_objfile (objfile);
+}
+
 /* Disable any breakpoints and tracepoints in OBJFILE upon
    notification of free_objfile.  Only apply to enabled breakpoints,
    disabled ones can just stay disabled.  */
@@ -15435,6 +15447,7 @@ _initialize_breakpoint ()
   initialize_breakpoint_ops ();

   gdb::observers::solib_unloaded.attach (disable_breakpoints_in_unloaded_shlib);
+  gdb::observers::jit_object_unloaded.attach (disable_breakpoints_in_unloaded_jit_object);
   gdb::observers::free_objfile.attach (disable_breakpoints_in_freed_objfile);
   gdb::observers::memory_changed.attach (invalidate_bp_value_on_memory_change);

diff --git a/gdb/jit.c b/gdb/jit.c
index 1b5ef46469e0..16e38078bbd0 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -925,6 +925,27 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
   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
+    {
+      gdb::observers::jit_object_unloaded.notify (objfile);
+      objfile->unlink ();
+    }
+}
+
 /* This is called when a breakpoint is deleted.  It updates the
    inferior's cache, if needed.  */

@@ -1335,7 +1356,6 @@ jit_event_handler (struct gdbarch *gdbarch)
   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,
@@ -1353,14 +1373,7 @@ jit_event_handler (struct gdbarch *gdbarch)
       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/observable.c b/gdb/observable.c
index 81aa392cc21f..92354c64a366 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -46,6 +46,7 @@ DEFINE_OBSERVABLE (inferior_created);
 DEFINE_OBSERVABLE (record_changed);
 DEFINE_OBSERVABLE (solib_loaded);
 DEFINE_OBSERVABLE (solib_unloaded);
+DEFINE_OBSERVABLE (jit_object_unloaded);
 DEFINE_OBSERVABLE (new_objfile);
 DEFINE_OBSERVABLE (free_objfile);
 DEFINE_OBSERVABLE (new_thread);
diff --git a/gdb/observable.h b/gdb/observable.h
index 070cf0f18e51..b60ee7f3efe6 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -111,6 +111,8 @@ extern observable<struct so_list */* solib */> solib_loaded;
    been unloaded yet, and thus are still available.  */
 extern observable<struct so_list */* solib */> solib_unloaded;

+extern observable<objfile */* objfile */> jit_object_unloaded;
+
 /* The symbol file specified by OBJFILE has been loaded.  Called
    with OBJFILE equal to NULL to indicate previously loaded symbol
    table data has now been invalidated.  */
-- 
2.27.0

RE: [PATCH 2/2] jit: remove bp locations when unregistering jit code

[Strasuns, Mihails]

Hello,

I have tested your proposed fix and it works perfectly for my original issue too. I have submitted new patch series which switches to your fix and modifies the test case to take into account multi-inferior scenario you have mentioned previously.

Thanks!

Mihails

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Sunday, June 7, 2020 5:42 AM
> To: Strasuns, Mihails <mihails.strasuns@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code
> 
> On 2020-05-25 6:07 a.m., Strasuns, Mihails via Gdb-patches wrote:
> > Hello,
> >
> > I will focus on the design related question for now:
> >
> >> -----Original Message-----
> >> From: Simon Marchi <simark@simark.ca>
> >> Sent: Sunday, May 24, 2020 5:02 PM
> >> To: Strasuns, Mihails <mihails.strasuns@intel.com>; gdb-
> >> patches@sourceware.org
> >> Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering
> >> jit code
> >>
> >> I'm not very familiar with how breakpoint locations are computed and
> >> updated (I just know it's complicated). But I confirm that the test
> >> shows the problem without the fix applied and passes with the fix
> >> applied.  I'd like if some other maintainer more familiar with this area
> could review this part.
> >>
> >> I'd be really curious to understand how GDB handles this when it
> >> comes to shared libraries.  Let's imagine the same case, but using shared
> libraries.
> >> When a shared library is unloaded, the breakpoint locations that were
> >> in that library must be forgotten somehow?  If so through which hook /
> mechanism?
> >
> > This is something I have initially looked into myself didn't find a
> > clear answer in GDB sources :-)
> >
> > bp_location defines this field:
> >
> > ---
> >   /* This location's address is in an unloaded solib, and so this
> >      location should not be inserted.  It will be automatically
> >      enabled when that solib is loaded.  */
> >   bool shlib_disabled = false;
> > ---
> >
> > What I don't understand though is that once set to true/1, this field is never
> reset back to 0. My guess is that this is intended to work in a similar way to
> my fix, i.e. that a new breakpoint location gets created for the new load
> rather than the old one being recreated. However I am not entirely sure.
> 
> I really don't know why, when a shared library containing a location gets
> unloaded, we just mark the location as "disabled" and leave it there.  I really
> don't see the point of leaving it there.  Maybe there's a comment
> somewhere that explains why we do this instead of removing it, but I didn't
> find any.
> 
> I also find it odd that when a breakpoint location gets disabled, we print the
> message "Temporarily disabling breakpoints for $solib".  Why "Temporarily"?
> If the same shared lib gets re-loaded, it might or might not be at the same
> address, so it's not like the breakpoint location is going to be reused anyway.
> 
> I think that's something we can investigate separately from the current
> patch, maybe for now we can just disable the locations instead of removing
> them, to keep it consistent with shared libs.
> 
> > There is a fair amount of differences between semantics of jit and shared
> libraries, so I decided to start with an independent fix and ask in this mail list
> for someone who knows better.
> >
> > Two important notes:
> >
> > - there is a `disable_breakpoints_in_freed_objfile` hook in breakpoint.c
> which looks like it should have covered this case but somehow it is not
> enough.
> 
> Indeed, that sounds like a promising function that sounds like it should do
> what we want (and it does already get called when a jit object gets
> unloaded).  But it returns early because of this:
> 
>   /* OBJF_SHARED|OBJF_USERLOADED objfiles are dynamic modules
> manually
>      managed by the user with add-symbol-file/remove-symbol-file.
>      Similarly to how breakpoints in shared libraries are handled in
>      response to "nosharedlibrary", mark breakpoints in such modules
>      shlib_disabled so they end up uninserted on the next global
>      location list update.  Shared libraries not loaded by the user
>      aren't handled here -- they're already handled in
>      disable_breakpoints_in_unloaded_shlib, called by solib.c's
>      solib_unloaded observer.  We skip objfiles that are not
>      OBJF_SHARED as those aren't considered dynamic objects (e.g. the
>      main objfile).  */
>   if ((objfile->flags & OBJF_SHARED) == 0
>       || (objfile->flags & OBJF_USERLOADED) == 0)
>     return;
> 
> So it sounds like this function considers that because shared libraries where
> already taken care of in disable_breakpoints_in_unloaded_shlib, all that
> remains is user-loaded objfiles.  But that's not true, there are jit-loaded
> objfiles, which are neither user-loaded nor shared libraries.
> 
> Looking at disable_breakpoints_in_unloaded_shlib and
> disable_breakpoints_in_freed_objfile, I noticed they do make sure that the
> loc's pspace is the same as the unloaded shlib / freed objfile's pspace, which
> you don't do in your patch.  I just tried with your patch:
> 
> - Start two inferiors with one jit object each
> - Create a breakpoint with two locations (one in each jit object)
> - Step one inferior so that it unloads its jit object
> 
> It ended up deleting the breakpoint locations for the two inferiors, whereas
> it should have left the location for the inferior that still has its jit object
> loaded.  It would be good to enhance the new test case to test that case.
> 
> To avoid trying to re-invent the wheel and making the same subtle mistakes
> that are (hopefully) already solved for the shared library case, I think we
> should try to re-use the same code path for disabling breakpoint locations
> when either a shared library or jit object is unloaded.
> 
> What about something like this, which makes unloaded jit objects go through
> the same breakpoint location disabling function as the unloaded shared libs?
> 
> 
> From 6e3a377dc8181ec1ce430711d48037f513b153be Mon Sep 17 00:00:00
> 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Sat, 6 Jun 2020 22:47:31 -0400
> Subject: [PATCH] Disable breakpoint locations in unloaded jit objects
> 
> Change-Id: I06ff4f3f02b3c348165ed451339e6effba6e3e97
> ---
>  gdb/breakpoint.c | 29 +++++++++++++++++++++--------
>  gdb/jit.c        | 31 ++++++++++++++++++++++---------
>  gdb/observable.c |  1 +
>  gdb/observable.h |  2 ++
>  4 files changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index
> aead882acd88..cc15c4730364 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7487,12 +7487,8 @@ disable_breakpoints_in_shlibs (void)
>    }
>  }
> 
> -/* Disable any breakpoints and tracepoints that are in SOLIB upon
> -   notification of unloaded_shlib.  Only apply to enabled breakpoints,
> -   disabled ones can just stay disabled.  */
> -
>  static void
> -disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
> +disable_breakpoints_in_unloaded_objfile (objfile *objfile)
>  {
>    struct bp_location *loc, **locp_tmp;
>    int disabled_shlib_breaks = 0;
> @@ -7502,7 +7498,7 @@ disable_breakpoints_in_unloaded_shlib (struct
> so_list *solib)
>      /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.
> */
>      struct breakpoint *b = loc->owner;
> 
> -    if (solib->pspace == loc->pspace
> +    if (objfile->pspace == loc->pspace
>  	&& !loc->shlib_disabled
>  	&& (((b->type == bp_breakpoint
>  	      || b->type == bp_jit_event
> @@ -7510,7 +7506,7 @@ disable_breakpoints_in_unloaded_shlib (struct
> so_list *solib)
>  	     && (loc->loc_type == bp_loc_hardware_breakpoint
>  		 || loc->loc_type == bp_loc_software_breakpoint))
>  	    || is_tracepoint (b))
> -	&& solib_contains_address_p (solib, loc->address))
> +	&& is_addr_in_objfile (loc->address, objfile))
>        {
>  	loc->shlib_disabled = 1;
>  	/* At this point, we cannot rely on remove_breakpoint @@ -7526,13
> +7522,29 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>  	    target_terminal::ours_for_output ();
>  	    warning (_("Temporarily disabling breakpoints "
>  		       "for unloaded shared library \"%s\""),
> -		     solib->so_name);
> +		     objfile->original_name);
>  	  }
>  	disabled_shlib_breaks = 1;
>        }
>    }
>  }
> 
> +/* Disable any breakpoints and tracepoints that are in SOLIB upon
> +   notification of unloaded_shlib.  Only apply to enabled breakpoints,
> +   disabled ones can just stay disabled.  */
> +
> +static void
> +disable_breakpoints_in_unloaded_shlib (struct so_list *solib) {
> +  disable_breakpoints_in_unloaded_objfile (solib->objfile); }
> +
> +static void
> +disable_breakpoints_in_unloaded_jit_object (objfile *objfile) {
> +  disable_breakpoints_in_unloaded_objfile (objfile); }
> +
>  /* Disable any breakpoints and tracepoints in OBJFILE upon
>     notification of free_objfile.  Only apply to enabled breakpoints,
>     disabled ones can just stay disabled.  */ @@ -15435,6 +15447,7 @@
> _initialize_breakpoint ()
>    initialize_breakpoint_ops ();
> 
>    gdb::observers::solib_unloaded.attach
> (disable_breakpoints_in_unloaded_shlib);
> +  gdb::observers::jit_object_unloaded.attach
> + (disable_breakpoints_in_unloaded_jit_object);
>    gdb::observers::free_objfile.attach
> (disable_breakpoints_in_freed_objfile);
>    gdb::observers::memory_changed.attach
> (invalidate_bp_value_on_memory_change);
> 
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 1b5ef46469e0..16e38078bbd0 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -925,6 +925,27 @@ jit_find_objf_with_entry_addr (CORE_ADDR
> entry_addr)
>    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
> +    {
> +      gdb::observers::jit_object_unloaded.notify (objfile);
> +      objfile->unlink ();
> +    }
> +}
> +
>  /* This is called when a breakpoint is deleted.  It updates the
>     inferior's cache, if needed.  */
> 
> @@ -1335,7 +1356,6 @@ jit_event_handler (struct gdbarch *gdbarch)
>    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, @@ -1353,14 +1373,7 @@
> jit_event_handler (struct gdbarch *gdbarch)
>        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/observable.c b/gdb/observable.c index 81aa392cc21f..92354c64a366
> 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -46,6 +46,7 @@ DEFINE_OBSERVABLE (inferior_created);
> DEFINE_OBSERVABLE (record_changed);  DEFINE_OBSERVABLE
> (solib_loaded);  DEFINE_OBSERVABLE (solib_unloaded);
> +DEFINE_OBSERVABLE (jit_object_unloaded);
>  DEFINE_OBSERVABLE (new_objfile);
>  DEFINE_OBSERVABLE (free_objfile);
>  DEFINE_OBSERVABLE (new_thread);
> diff --git a/gdb/observable.h b/gdb/observable.h index
> 070cf0f18e51..b60ee7f3efe6 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -111,6 +111,8 @@ extern observable<struct so_list */* solib */>
> solib_loaded;
>     been unloaded yet, and thus are still available.  */  extern
> observable<struct so_list */* solib */> solib_unloaded;
> 
> +extern observable<objfile */* objfile */> jit_object_unloaded;
> +
>  /* The symbol file specified by OBJFILE has been loaded.  Called
>     with OBJFILE equal to NULL to indicate previously loaded symbol
>     table data has now been invalidated.  */
> --
> 2.27.0

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928