[PATCH 1/2] gdb/testsuite: tweak compiler flag handling in jit-elf-helpers.exp

[PATCH 1/2] gdb/testsuite: tweak compiler flag handling in jit-elf-helpers.exp

[Mihails Strasuns]

db/testsuite/ChangeLog:
2020-06-16  Mihails Strasuns  <mihails.strasuns@intel.com>

	* lib/jit-elf-helpers.exp (compile_jit_elf_main_as_so): Remove
	unused $options parameter.
	(compile_and_download_n_jit_so): Add optional parameter for
	specifying extra compiler flags.
	* gdb.base/jit-elf-so.exp: Do not pass compiler flag argument,
	it is already added by the proc itself.
---
 gdb/testsuite/gdb.base/jit-elf-so.exp | 3 +--
 gdb/testsuite/lib/jit-elf-helpers.exp | 9 +++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.base/jit-elf-so.exp b/gdb/testsuite/gdb.base/jit-elf-so.exp
index 7e29a99e20..d55e393240 100644
--- a/gdb/testsuite/gdb.base/jit-elf-so.exp
+++ b/gdb/testsuite/gdb.base/jit-elf-so.exp
@@ -141,8 +141,7 @@ proc one_jit_test {solib_binfiles_target match_str} {
 }
 
 # Compile the main code (which loads the JIT objects) as a shared library.
-if { [compile_jit_elf_main_as_so $main_solib_srcfile $main_solib_binfile \
-	{additional_flags="-DMAIN=jit_dl_main"}] < 0 } {
+if { [compile_jit_elf_main_as_so $main_solib_srcfile $main_solib_binfile] < 0 } {
     return
 }
 
diff --git a/gdb/testsuite/lib/jit-elf-helpers.exp b/gdb/testsuite/lib/jit-elf-helpers.exp
index f1e8ad7785..4b720bbb18 100644
--- a/gdb/testsuite/lib/jit-elf-helpers.exp
+++ b/gdb/testsuite/lib/jit-elf-helpers.exp
@@ -52,7 +52,7 @@ proc compile_jit_main {main_srcfile main_binfile options} {
 #
 # On success, return 0.
 # On failure, return -1.
-proc compile_jit_elf_main_as_so {main_solib_srcfile main_solib_binfile options} {
+proc compile_jit_elf_main_as_so {main_solib_srcfile main_solib_binfile} {
     global jit_load_address jit_load_increment
 
     set options [list \
@@ -76,7 +76,7 @@ proc compile_jit_elf_main_as_so {main_solib_srcfile main_solib_binfile 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 {options {}}} {
     global jit_load_address jit_load_increment
     set binfiles_target {}
 
@@ -92,12 +92,13 @@ proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count}
 	# in the compiled shared library against a fixed base address.  Combined
 	# with mapping the resulting binary to the same fixed base it allows
 	# to dynamically execute functions from it without any further adjustments.
-	set options [list \
+	set so_options [concat \
+	    $options \
 	    additional_flags=-DFUNCTION_NAME=[format "jit_function_%04d" $i] \
 	    additional_flags=-Xlinker \
 	    additional_flags=-Ttext-segment=$addr]
 	if { [gdb_compile_shlib ${jit_solib_srcfile} ${binfile} \
-		  $options] != "" } {
+		  $so_options] != "" } {
 	    set f [file tail $binfile]
 	    untested "failed to compile shared library $f"
 	    return -1
-- 
2.17.1

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 2/2] Disable breakpoint locations in unloaded jit objects

[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 fix was suggested by Simon Marchi <simark@simark.ca> and extends
`disable_breakpoints_in_unloaded_shlib` to work on any object file and
uses it to disable breakpoints for both shared libraries and jit
objects.

gdb/ChangeLog:
2020-06-16  Mihails Strasuns  <mihails.strasuns@intel.com>

	* breakpoint.c (disable_breakpoints_in_unloaded_shlib): Renamed
	to 'disable_breakpoints_in_unloaded_objfile` and changed to
	accept an objfile as an argument.
	(disable_breakpoints_in_unloaded_shlib): New function.
	disable_breakpoints_in_unloaded_jit_objects): New function.
	* jit.c (jit_unregister_code): New function, called to handle
	JIT_UNREGISTER and discard a jit objfile properly.
	* observable.c, observable.h: New observable, 'jit_object_unloaded'.

gdb/testsuite/ChangeLog:
2020-06-16  Mihails Strasuns  <mihails.strasuns@intel.com>

	* gdb.base/jit-elf-reregister.c, gdb.base/jit-elf-reregister.exp: New
	test case, verifies that breakpoints still hit if a jit object
	that contains them gets reloaded and mapped again to the same
	base address.
---
 gdb/breakpoint.c                              | 29 +++++--
 gdb/jit.c                                     | 31 +++++---
 gdb/observable.c                              |  1 +
 gdb/observable.h                              |  2 +
 gdb/testsuite/gdb.base/jit-elf-reregister.c   | 66 ++++++++++++++++
 gdb/testsuite/gdb.base/jit-elf-reregister.exp | 78 +++++++++++++++++++
 6 files changed, 190 insertions(+), 17 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 aead882acd..cc15c47303 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 1b5ef46469..16e38078bb 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 81aa392cc2..92354c64a3 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 070cf0f18e..b60ee7f3ef 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.  */
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..614b7b9795
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
@@ -0,0 +1,78 @@
+# 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
+}
+
+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
+
+# 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
+}
+
+proc test_inferior_initial {num} {
+    global jit_solibs_target main_srcfile main_binfile jit_solib_basename
+
+    gdb_test "inferior ${num}"
+
+    if { ![runto_main] } {
+	fail "can't run to main for the inferior ${num}"
+	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 in inferior ${num}"
+    gdb_test_no_output "set var argv=fake_argv" "forging argv in inferior ${num}"
+    set jit_solib_target [lindex $jit_solibs_target 0]
+    gdb_test_no_output "set var argv\[1\]=\"${jit_solib_target}\"" \
+    "forging argv\[1\] in inferior ${num}"
+
+    gdb_breakpoint [gdb_get_line_number "first-call" $main_srcfile] {temporary}
+    gdb_continue_to_breakpoint "first-call in inferior ${num}"
+    gdb_breakpoint "jit_function_0001"
+    gdb_continue_to_breakpoint "hit before reload inferior ${num}" ".*$jit_solib_basename.*"
+}
+
+# Compile the main code (which loads the JIT objects).
+if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] != 0 } {
+    return
+}
+
+clean_restart ${main_binfile}
+gdb_test "add-inferior -exec ${main_binfile}" "Added inferior 2.*" \
+"Add second inferior"
+test_inferior_initial 1
+test_inferior_initial 2
+gdb_continue_to_breakpoint "hit after reload inferior 2" ".*$jit_solib_basename.*"
+gdb_test "inferior 1" "Switching to inferior 1.*" "finishing another inferior"
+gdb_continue_to_breakpoint "hit after reload inferior 1" ".*$jit_solib_basename.*"
\ No newline at end of file
-- 
2.17.1

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] Disable breakpoint locations in unloaded jit objects

[Strasuns, Mihails]

Ping.

> -----Original Message-----
> From: Mihails Strasuns <mihails.strasuns@intel.com>
> Sent: Tuesday, June 16, 2020 1:46 PM
> To: gdb-patches@sourceware.org
> Cc: Strasuns, Mihails <mihails.strasuns@intel.com>
> Subject: [PATCH 2/2] Disable breakpoint locations in unloaded jit objects
> 
> 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 fix was suggested by Simon Marchi <simark@simark.ca> and extends
> `disable_breakpoints_in_unloaded_shlib` to work on any object file and uses
> it to disable breakpoints for both shared libraries and jit objects.
> 
> gdb/ChangeLog:
> 2020-06-16  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* breakpoint.c (disable_breakpoints_in_unloaded_shlib): Renamed
> 	to 'disable_breakpoints_in_unloaded_objfile` and changed to
> 	accept an objfile as an argument.
> 	(disable_breakpoints_in_unloaded_shlib): New function.
> 	disable_breakpoints_in_unloaded_jit_objects): New function.
> 	* jit.c (jit_unregister_code): New function, called to handle
> 	JIT_UNREGISTER and discard a jit objfile properly.
> 	* observable.c, observable.h: New observable,
> 'jit_object_unloaded'.
> 
> gdb/testsuite/ChangeLog:
> 2020-06-16  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* gdb.base/jit-elf-reregister.c, gdb.base/jit-elf-reregister.exp: New
> 	test case, verifies that breakpoints still hit if a jit object
> 	that contains them gets reloaded and mapped again to the same
> 	base address.
> ---
>  gdb/breakpoint.c                              | 29 +++++--
>  gdb/jit.c                                     | 31 +++++---
>  gdb/observable.c                              |  1 +
>  gdb/observable.h                              |  2 +
>  gdb/testsuite/gdb.base/jit-elf-reregister.c   | 66 ++++++++++++++++
>  gdb/testsuite/gdb.base/jit-elf-reregister.exp | 78 +++++++++++++++++++
>  6 files changed, 190 insertions(+), 17 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
> aead882acd..cc15c47303 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 1b5ef46469..16e38078bb 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 81aa392cc2..92354c64a3
> 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
> 070cf0f18e..b60ee7f3ef 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.  */ 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..614b7b9795
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
> @@ -0,0 +1,78 @@
> +# 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
> +}
> +
> +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
> +
> +# 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
> +}
> +
> +proc test_inferior_initial {num} {
> +    global jit_solibs_target main_srcfile main_binfile
> +jit_solib_basename
> +
> +    gdb_test "inferior ${num}"
> +
> +    if { ![runto_main] } {
> +	fail "can't run to main for the inferior ${num}"
> +	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 in inferior ${num}"
> +    gdb_test_no_output "set var argv=fake_argv" "forging argv in inferior
> ${num}"
> +    set jit_solib_target [lindex $jit_solibs_target 0]
> +    gdb_test_no_output "set var argv\[1\]=\"${jit_solib_target}\"" \
> +    "forging argv\[1\] in inferior ${num}"
> +
> +    gdb_breakpoint [gdb_get_line_number "first-call" $main_srcfile]
> {temporary}
> +    gdb_continue_to_breakpoint "first-call in inferior ${num}"
> +    gdb_breakpoint "jit_function_0001"
> +    gdb_continue_to_breakpoint "hit before reload inferior ${num}"
> ".*$jit_solib_basename.*"
> +}
> +
> +# Compile the main code (which loads the JIT objects).
> +if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] != 0 } {
> +    return
> +}
> +
> +clean_restart ${main_binfile}
> +gdb_test "add-inferior -exec ${main_binfile}" "Added inferior 2.*" \
> +"Add second inferior"
> +test_inferior_initial 1
> +test_inferior_initial 2
> +gdb_continue_to_breakpoint "hit after reload inferior 2"
> ".*$jit_solib_basename.*"
> +gdb_test "inferior 1" "Switching to inferior 1.*" "finishing another inferior"
> +gdb_continue_to_breakpoint "hit after reload inferior 1"
> ".*$jit_solib_basename.*"
> \ No newline at end of file
> --
> 2.17.1

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: tweak compiler flag handling in jit-elf-helpers.exp

[Simon Marchi]

On 2020-06-16 7:45 a.m., Mihails Strasuns via Gdb-patches wrote:
> db/testsuite/ChangeLog:
> 2020-06-16  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* lib/jit-elf-helpers.exp (compile_jit_elf_main_as_so): Remove
> 	unused $options parameter.
> 	(compile_and_download_n_jit_so): Add optional parameter for
> 	specifying extra compiler flags.
> 	* gdb.base/jit-elf-so.exp: Do not pass compiler flag argument,
> 	it is already added by the proc itself.

Thanks that LGTM.  I would perhaps have added the new options argument in the next
patch, that actually uses it but... not a big deal.

Btw, just a nit.  Normally, the ChangeLog entry should just say very briefly what
changed not why, or go into explanations.  We prefer to put the longer explanation
about the change, the rationale, etc, in the commit message itself.  It's easier
to write something descriptive in prose there than in the ChangeLog entry, which,
by its nature, restricts the format.

So for the ChangeLog entry, you can be very brief, like:

 	* lib/jit-elf-helpers.exp (compile_jit_elf_main_as_so): Remove
 	$options parameter.
 	(compile_and_download_n_jit_so): Add optional options parameter.
 	* gdb.base/jit-elf-so.exp: Do not pass compiler flag argument.

Simon

Re: [PATCH 2/2] Disable breakpoint locations in unloaded jit objects

[Simon Marchi]

On 2020-06-16 7:45 a.m., Mihails Strasuns via Gdb-patches wrote:
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index aead882acd..cc15c47303 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.  */
> -

I think that this comment should stay attached to the function that actually
does the work, that is shared for shlibs and jit objects.  It perhaps just
need to be adapted to not be specific to solibs.

>  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))

With this change, solib_contains_address_p becomes only used in solib.c, the file it's
defined in.  So another patch on top that makes it static would be nice.

> diff --git a/gdb/jit.c b/gdb/jit.c
> index 1b5ef46469..16e38078bb 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;

Declare on first use, below.

> +  if (jit_debug)
> +    fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%s)\n",

If this is meant to refer to the function name, use __func__, in case
the function changes name.  Also, maybe give a name to the information
you are printing, otherwise it just appears as an address and we don't
really know what it is.  So, something like:

  "%s: objfile=%s", __func__, host_address_to_string (objfile)

I would avoid the parenthesis, unless you are showing the values of the
arguments passed to the funciton (which is not the case here).  And in
fact, won't `objfile` always be nullptr there?  The debug print should
probably be moved lower.

> +			host_address_to_string (objfile));

Newline after the `if`.

> diff --git a/gdb/observable.c b/gdb/observable.c
> index 81aa392cc2..92354c64a3 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 070cf0f18e..b60ee7f3ef 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;

We'll need some doc here.

> 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..614b7b9795
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
> @@ -0,0 +1,78 @@
> +# Copyright 2019 Free Software Foundation, Inc.

2020, unless you copied significant portions of an existing test case, in which
case you should copy the copyright too (whose range should finish in 2020 in any
case).

> +
> +# 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.

That comment is unrelated.  Please add one that's related to this test case.

> +
> +if {[skip_shlib_tests]} {
> +    untested "skipping shared library tests"
> +    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
> +
> +# 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
> +}
> +
> +proc test_inferior_initial {num} {

Please add a comment to say what this proc does.

> +    global jit_solibs_target main_srcfile main_binfile jit_solib_basename
> +
> +    gdb_test "inferior ${num}"
> +
> +    if { ![runto_main] } {
> +	fail "can't run to main for the inferior ${num}"
> +	return
> +    }
> +
> +    # Poke desired values directly into inferior instead of using "set args"
> +    # because "set args" does not work under gdbserver.

I don't think that's true anymore?

> +    gdb_test_no_output "set var argc=1" "forging argc in inferior ${num}"
> +    gdb_test_no_output "set var argv=fake_argv" "forging argv in inferior ${num}"
> +    set jit_solib_target [lindex $jit_solibs_target 0]
> +    gdb_test_no_output "set var argv\[1\]=\"${jit_solib_target}\"" \
> +    "forging argv\[1\] in inferior ${num}"
> +
> +    gdb_breakpoint [gdb_get_line_number "first-call" $main_srcfile] {temporary}
> +    gdb_continue_to_breakpoint "first-call in inferior ${num}"
> +    gdb_breakpoint "jit_function_0001"
> +    gdb_continue_to_breakpoint "hit before reload inferior ${num}" ".*$jit_solib_basename.*"
> +}
> +
> +# Compile the main code (which loads the JIT objects).
> +if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] != 0 } {
> +    return
> +}
> +
> +clean_restart ${main_binfile}
> +gdb_test "add-inferior -exec ${main_binfile}" "Added inferior 2.*" \
> +"Add second inferior"

Missing indentation.

Simon