[PATCH v6 0/3] Disable breakpoint locations in unloaded jit objects

[PATCH v6 0/3] Disable breakpoint locations in unloaded jit objects

[Mihails Strasuns]

This is an old patcch that I have forgotten about thinking it is already
merged. Sorry about that! Addressed last review comments now.


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 v6 1/3] gdb/testsuite: tweak compiler flag handling in jit-elf-helpers.exp

[Mihails Strasuns]

From: Mihails Strasuns via Gdb-patches <gdb-patches@sourceware.org>

- Unused $options parameter is removed for `compile_jit_elf_main_as_so`.
- New optional compiler flag parameter is added to
  `compile_and_download_n_jit_so`.  Will be used in the following commit.
- Do not pass compiler flag argument in a JIT test case,
  it is already added by the helper proc itself.

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

	* lib/jit-elf-helpers.exp (compile_jit_elf_main_as_so): Remove
	unused parameter.
	(compile_and_download_n_jit_so): Add optional parameter.
	* gdb.base/jit-elf-so.exp: Do not pass compiler flag argument.
---
 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 7e29a99e208..d55e393240e 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 f1e8ad77852..4b720bbb18c 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 v6 2/3] 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                              | 30 +++++--
 gdb/jit.c                                     | 31 +++++--
 gdb/observable.c                              |  1 +
 gdb/observable.h                              |  3 +
 gdb/testsuite/gdb.base/jit-elf-reregister.c   | 65 +++++++++++++++
 gdb/testsuite/gdb.base/jit-elf-reregister.exp | 81 +++++++++++++++++++
 6 files changed, 196 insertions(+), 15 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 881686ff7a0..84559069b6e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7631,22 +7631,25 @@ 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.  */
+/* Disable any breakpoints and tracepoints that are in OBJFILE.
+   Only apply to enabled breakpoints, disabled one can just stay disabled.
+   Used for both shared library and JIT module unloading.  */
 
 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;
 
+  if (objfile == nullptr)
+    return;
+
   ALL_BP_LOCATIONS (loc, locp_tmp)
   {
     /* 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
@@ -7654,7 +7657,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
@@ -7670,13 +7673,25 @@ 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;
       }
   }
 }
 
+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.  */
@@ -15683,6 +15698,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 e36b3c3b68a..34ad3f34f72 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -779,6 +779,28 @@ 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 = jit_find_objf_with_entry_addr (entry_addr);
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog, "%s: objfile = %s\n",
+			__func__,
+			host_address_to_string (objfile));
+
+  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.  */
 
@@ -1214,14 +1236,7 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
 
     case JIT_UNREGISTER:
       {
-	objfile *jited = jit_find_objf_with_entry_addr (entry_addr);
-	if (jited == nullptr)
-	  printf_unfiltered (_("Unable to find JITed code "
-			       "entry at address: %s\n"),
-			     paddress (gdbarch, entry_addr));
-	else
-	  jited->unlink ();
-
+	jit_unregister_code (gdbarch, entry_addr);
 	break;
       }
 
diff --git a/gdb/observable.c b/gdb/observable.c
index 231f955fa26..f329461469d 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -47,6 +47,7 @@ DEFINE_OBSERVABLE (inferior_execd);
 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 1dce6746ff3..f1e733e6682 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -114,6 +114,9 @@ extern observable<struct so_list */* solib */> solib_loaded;
    been unloaded yet, and thus are still available.  */
 extern observable<struct so_list */* solib */> solib_unloaded;
 
+/* The JIT module represented by OBJFILE has been unregistered.  */
+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 00000000000..37e8277babe
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-elf-reregister.c
@@ -0,0 +1,65 @@
+/* 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[])
+{
+  size_t obj_size;
+  void *load_addr = (void *) (size_t) LOAD_ADDRESS;
+  printf("HERE %s\n", argv[1]);
+  void *addr = load_elf (argv[1], &obj_size, load_addr);
+  printf("HEAR %p\n", 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 00000000000..6735ce2a191
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
@@ -0,0 +1,81 @@
+# 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/>.  */
+# 
+# Checks that breakpoints keep working if a JIT module is repeatedly
+# being registered and unregistered at the same base address.
+
+if {[use_gdb_stub]} {
+    return 0
+}
+
+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
+}
+
+# Prepare two inferiors using the same JIT module.  Hit an initial breakpoint
+# in a JIT-loaded function as a sanity check that basic functionality is working.
+proc test_inferior_initial {num} {
+    global jit_solibs_target main_srcfile main_binfile jit_solib_basename
+
+    gdb_test "inferior ${num}"
+    gdb_test_no_output "set args $jit_solibs_target" \
+	"passing solib list as an argument ($num)"
+
+    if { ![runto_main] } {
+	fail "can't run to main for the inferior ${num}"
+	return
+    }
+
+    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 a second inferior"
+test_inferior_initial 1
+test_inferior_initial 2
+# $main_srcfile is written so that the JIT module gets reloaded after
+# `jit_function_0001` finishes - and the function is called again. Test that
+# the breakpoint still remains valid (for both inferiors):
+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.*"
-- 
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 v6 3/3] gdb: make solib_contains_address_p static

[Mihails Strasuns]

After changes to jit.c this function is only used in solib.c where it is
defined - which means it can be turned into a static function for now.

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

	* solib.h (solib_contains_address_p): Removed.
	* solib.c (solib_contains_address_p): Changed to static.
---
 gdb/solib.c | 4 ++--
 gdb/solib.h | 4 ----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/solib.c b/gdb/solib.c
index 1f6e91599a5..c0971e16d2a 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1108,9 +1108,9 @@ info_sharedlibrary_command (const char *pattern, int from_tty)
     }
 }
 
-/* See solib.h.  */
+/* Return true if ADDR lies within SOLIB.  */
 
-bool
+static bool
 solib_contains_address_p (const struct so_list *const solib,
 			  CORE_ADDR address)
 {
diff --git a/gdb/solib.h b/gdb/solib.h
index 801965fcd53..8cfd402422d 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -49,10 +49,6 @@ extern void solib_create_inferior_hook (int from_tty);
 
 extern char *solib_name_from_address (struct program_space *, CORE_ADDR);
 
-/* Return true if ADDR lies within SOLIB.  */
-
-extern bool solib_contains_address_p (const struct so_list *, CORE_ADDR);
-
 /* Return whether the data starting at VADDR, size SIZE, must be kept
    in a core file for shared libraries loaded before "gcore" is used
    to be handled correctly when the core file is loaded.  This only
-- 
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 v6 0/3] Disable breakpoint locations in unloaded jit objects

[Strasuns, Mihails]

Kindly pinging.

> -----Original Message-----
> From: Gdb-patches <gdb-patches-bounces@sourceware.org> On Behalf Of
> Mihails Strasuns via Gdb-patches
> Sent: Thursday, December 10, 2020 8:28 PM
> To: gdb-patches@sourceware.org
> Subject: [PATCH v6 0/3] Disable breakpoint locations in unloaded jit objects
> 
> This is an old patcch that I have forgotten about thinking it is already merged.
> Sorry about that! Addressed last review comments now.
> 
> 
> 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 v6 1/3] gdb/testsuite: tweak compiler flag handling in jit-elf-helpers.exp

[Simon Marchi]

On 2020-12-10 2:28 p.m., Mihails Strasuns via Gdb-patches wrote:
> From: Mihails Strasuns via Gdb-patches <gdb-patches@sourceware.org>
> 
> - Unused $options parameter is removed for `compile_jit_elf_main_as_so`.
> - New optional compiler flag parameter is added to
>   `compile_and_download_n_jit_so`.  Will be used in the following commit.
> - Do not pass compiler flag argument in a JIT test case,
>   it is already added by the helper proc itself.
> 
> gdb/testsuite/ChangeLog:
> 2020-06-16  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* lib/jit-elf-helpers.exp (compile_jit_elf_main_as_so): Remove
> 	unused parameter.
> 	(compile_and_download_n_jit_so): Add optional parameter.
> 	* gdb.base/jit-elf-so.exp: Do not pass compiler flag argument.

LGTM, thanks.

Simon

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

[Simon Marchi]

On 2020-12-10 2:28 p.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 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>

Make sure to update those dates before pushing.  When I prepare ChangeLog
entries, I usually omit this line, since the date will likely change
anyway, and just add it to the ChangeLog file when pushing the patch.  In
fact, I use that little script:

  https://github.com/simark/gnu-changelog-tools/tree/master/changelog-git-amend

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

No need to put that much details in the ChangeLog, just use "New." if the file
is new.  All "human-readable" explanation should be put in the actual commit log.

> ---
>  gdb/breakpoint.c                              | 30 +++++--
>  gdb/jit.c                                     | 31 +++++--
>  gdb/observable.c                              |  1 +
>  gdb/observable.h                              |  3 +
>  gdb/testsuite/gdb.base/jit-elf-reregister.c   | 65 +++++++++++++++
>  gdb/testsuite/gdb.base/jit-elf-reregister.exp | 81 +++++++++++++++++++
>  6 files changed, 196 insertions(+), 15 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 881686ff7a0..84559069b6e 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7631,22 +7631,25 @@ 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.  */
> +/* Disable any breakpoints and tracepoints that are in OBJFILE.
> +   Only apply to enabled breakpoints, disabled one can just stay disabled.
> +   Used for both shared library and JIT module unloading.  */
>  
>  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;
>  
> +  if (objfile == nullptr)
> +    return;

Can you remind me when objfile is nullptr here?  It would be good to have a
comment to say when that happens.  Because intuitively, I would say that it
makes no sense to call this function with a nullptr argument (and I'd turn
that if into a gdb_assert), but maybe there's a good reason to.

> +
>    ALL_BP_LOCATIONS (loc, locp_tmp)
>    {
>      /* 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
> @@ -7654,7 +7657,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
> @@ -7670,13 +7673,25 @@ 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;
>        }
>    }
>  }
>  
> +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);
> +}

Please add a comment for these functions.

  /* solib_unloaded observer.  */

and

/* jit_object_unloaded observer.  */

would be enough.

> +
>  /* Disable any breakpoints and tracepoints in OBJFILE upon
>     notification of free_objfile.  Only apply to enabled breakpoints,
>     disabled ones can just stay disabled.  */
> @@ -15683,6 +15698,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 e36b3c3b68a..34ad3f34f72 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -779,6 +779,28 @@ 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 = jit_find_objf_with_entry_addr (entry_addr);
> +  if (jit_debug)
> +    fprintf_unfiltered (gdb_stdlog, "%s: objfile = %s\n",
> +			__func__,
> +			host_address_to_string (objfile));
> +
> +  if (objfile == NULL)
> +    printf_unfiltered (_ ("Unable to find JITed code "

We don't use a space for the "_" macro call, don't ask me why :).

> +			  "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.  */
>  
> @@ -1214,14 +1236,7 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
>  
>      case JIT_UNREGISTER:
>        {
> -	objfile *jited = jit_find_objf_with_entry_addr (entry_addr);
> -	if (jited == nullptr)
> -	  printf_unfiltered (_("Unable to find JITed code "
> -			       "entry at address: %s\n"),
> -			     paddress (gdbarch, entry_addr));
> -	else
> -	  jited->unlink ();
> -
> +	jit_unregister_code (gdbarch, entry_addr);
>  	break;
>        }
>  
> diff --git a/gdb/observable.c b/gdb/observable.c
> index 231f955fa26..f329461469d 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -47,6 +47,7 @@ DEFINE_OBSERVABLE (inferior_execd);
>  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 1dce6746ff3..f1e733e6682 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -114,6 +114,9 @@ extern observable<struct so_list */* solib */> solib_loaded;
>     been unloaded yet, and thus are still available.  */
>  extern observable<struct so_list */* solib */> solib_unloaded;
>  
> +/* The JIT module represented by OBJFILE has been unregistered.  */
> +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 00000000000..37e8277babe
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-elf-reregister.c
> @@ -0,0 +1,65 @@
> +/* 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[])
> +{
> +  size_t obj_size;
> +  void *load_addr = (void *) (size_t) LOAD_ADDRESS;
> +  printf("HERE %s\n", argv[1]);
> +  void *addr = load_elf (argv[1], &obj_size, load_addr);
> +  printf("HEAR %p\n", addr);

You can probably remove these printfs.  If not, add a space before
the parenthesis.

> +  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 00000000000..6735ce2a191
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
> @@ -0,0 +1,81 @@
> +# 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/>.  */
> +# 
> +# Checks that breakpoints keep working if a JIT module is repeatedly
> +# being registered and unregistered at the same base address.
> +
> +if {[use_gdb_stub]} {
> +    return 0
> +}
> +
> +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
> +}
> +
> +# Prepare two inferiors using the same JIT module.  Hit an initial breakpoint
> +# in a JIT-loaded function as a sanity check that basic functionality is working.
> +proc test_inferior_initial {num} {
> +    global jit_solibs_target main_srcfile main_binfile jit_solib_basename
> +
> +    gdb_test "inferior ${num}"
> +    gdb_test_no_output "set args $jit_solibs_target" \
> +	"passing solib list as an argument ($num)"

Don't finish the test name with parenthesis, as the trailing parenthesis
is not considered part of the test name:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

Also, say "pass" instead of "passing".

I suggest that you define this proc with "proc_with_prefix", that will
make it easier to map test names to the code.  And at the top-level, use
foreach_with_prefix to de-duplicate the two runs.  That avoids having to
add "for inferior $num" everywhere.  So:

proc_with_prefix test_inferior_initial {num} {
    global jit_solibs_target main_srcfile main_binfile jit_solib_basename

    gdb_test "inferior ${num}"
    gdb_test_no_output "set args $jit_solibs_target" \
	"pass solib list as an argument"

    if { ![runto_main] } {
	fail "can't run to main"
	return
    }

    gdb_breakpoint [gdb_get_line_number "first-call" $main_srcfile] {temporary}
    gdb_continue_to_breakpoint "first-call"
    gdb_breakpoint "jit_function_0001"
    gdb_continue_to_breakpoint "hit before reload" ".*$jit_solib_basename.*"
}

...

foreach_with_prefix inferior {1 2} {
    test_inferior_initial $inferior
}


And instead of having the intermediary breakpoint at "first-call", you could
set a pending breakpoint on jit_function_0001:

    gdb_breakpoint "jit_function_0001" allow-pending
    gdb_continue_to_breakpoint "hit before reload" ".*$jit_solib_basename.*"

> +clean_restart ${main_binfile}
> +gdb_test "add-inferior -exec ${main_binfile}" "Added inferior 2.*" \
> +    "Add a second inferior"
> +test_inferior_initial 1
> +test_inferior_initial 2
> +# $main_srcfile is written so that the JIT module gets reloaded after
> +# `jit_function_0001` finishes - and the function is called again. Test that
> +# the breakpoint still remains valid (for both inferiors):
> +gdb_continue_to_breakpoint "hit after reload inferior 2" ".*$jit_solib_basename.*"
> +gdb_test "inferior 1" "Switching to inferior 1.*" "finishing another inferior"

This can become just

  gdb_test "inferior 1" "Switching to inferior 1.*"

with my suggestion above, as the test names in test_inferior_initial will be
scoped.

Simon

Re: [PATCH v6 3/3] gdb: make solib_contains_address_p static

[Simon Marchi]

On 2020-12-10 2:28 p.m., Mihails Strasuns via Gdb-patches wrote:
> After changes to jit.c this function is only used in solib.c where it is
> defined - which means it can be turned into a static function for now.
> 
> gdb/ChangeLog:
> 2020-12-10  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* solib.h (solib_contains_address_p): Removed.
> 	* solib.c (solib_contains_address_p): Changed to static.
> ---
>  gdb/solib.c | 4 ++--
>  gdb/solib.h | 4 ----
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 1f6e91599a5..c0971e16d2a 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1108,9 +1108,9 @@ info_sharedlibrary_command (const char *pattern, int from_tty)
>      }
>  }
>  
> -/* See solib.h.  */
> +/* Return true if ADDR lies within SOLIB.  */
>  
> -bool
> +static bool
>  solib_contains_address_p (const struct so_list *const solib,
>  			  CORE_ADDR address)
>  {
> diff --git a/gdb/solib.h b/gdb/solib.h
> index 801965fcd53..8cfd402422d 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -49,10 +49,6 @@ extern void solib_create_inferior_hook (int from_tty);
>  
>  extern char *solib_name_from_address (struct program_space *, CORE_ADDR);
>  
> -/* Return true if ADDR lies within SOLIB.  */
> -
> -extern bool solib_contains_address_p (const struct so_list *, CORE_ADDR);
> -
>  /* Return whether the data starting at VADDR, size SIZE, must be kept
>     in a core file for shared libraries loaded before "gcore" is used
>     to be handled correctly when the core file is loaded.  This only
> 

This is ok (although it depends on the previous patch).

Simon

RE: [PATCH v6 0/3] Disable breakpoint locations in unloaded jit objects

[Strasuns, Mihails]

Ping.

> -----Original Message-----
> From: Gdb-patches <gdb-patches-bounces@sourceware.org> On Behalf Of
> Strasuns, Mihails via Gdb-patches
> Sent: Sunday, January 3, 2021 2:03 PM
> To: Mihails Strasuns via Gdb-patches <gdb-patches@sourceware.org>
> Subject: RE: [PATCH v6 0/3] Disable breakpoint locations in unloaded jit
> objects
> 
> Kindly pinging.
> 
> > -----Original Message-----
> > From: Gdb-patches <gdb-patches-bounces@sourceware.org> On Behalf
> Of
> > Mihails Strasuns via Gdb-patches
> > Sent: Thursday, December 10, 2020 8:28 PM
> > To: gdb-patches@sourceware.org
> > Subject: [PATCH v6 0/3] Disable breakpoint locations in unloaded jit
> > objects
> >
> > This is an old patcch that I have forgotten about thinking it is already
> merged.
> > Sorry about that! Addressed last review comments now.
> >
> >
> > 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

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928