[PATCH v2 0/3] Handling multiple JITers

[PATCH v2 0/3] Handling multiple JITers

[Tankut Baris Aktemur]

Hi,

This is v2 of the series originally submitted at

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

The updates in this revision are as follows:

1. I attempted to address Simon's comments on v1.

2. A separate patch has been added to make 'jit_even_handler' to take
   the JITer objfile as an argument (patch #1).

3. A separate patch has been added to convert two functions' return types
   from 'int' to 'bool' (patch #2).

4. Patch #1 and #2 in v1 have been merged into a single patch (#3 in v2).
   Because of the update mentioned in item (2) above, I don't think it's
   necessary to do a split.

Regards.
Baris


Tankut Baris Aktemur (3):
  gdb/jit: pass the jiter objfile as an argument to jit_event_handler
  gdb/jit: return bool in jit_breakpoint_re_set_internal and
    jit_read_descriptor
  gdb/jit: enable tracking multiple jitter objfiles

 gdb/breakpoint.c                             |   3 +-
 gdb/jit.c                                    | 205 +++++++++++--------
 gdb/jit.h                                    |   4 +-
 gdb/testsuite/gdb.base/jit-reader-simple.exp |  43 +++-
 4 files changed, 167 insertions(+), 88 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] gdb/jit: pass the jiter objfile as an argument to jit_event_handler

[Tankut Baris Aktemur]

This is a refactoring that adds a new parameter to the `jit_event_handler`
function: the JITer objfile.  The goal is to distinguish which JITer
triggered the JIT event, in case there are multiple JITers -- a capability
that is added in a subsequent patch.

gdb/ChangeLog:
2020-06-15  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* jit.h: Include "objfiles.h".
	(jit_event_handler): Add a second parameter, the JITer objfile.
	* breakpoint.c (handle_jit_event): Update the call to
	jit_event_handler to pass the JITer objfile.
	* jit.c (jit_read_descriptor): Change the signature to take the
	JITer objfile as an argument instead of the jit_program_space_data.
	(jit_inferior_init): Update the call to jit_read_descriptor.
	(jit_event_handler): Use the new JITer objfile argument when calling
	jit_read_descriptor.
---
 gdb/breakpoint.c |  3 ++-
 gdb/jit.c        | 27 +++++++++++++++------------
 gdb/jit.h        |  4 +++-
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index aead882acd8..da7e910dfc2 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5448,8 +5448,9 @@ handle_jit_event (void)
 
   frame = get_current_frame ();
   gdbarch = get_frame_arch (frame);
+  objfile *jiter = symbol_objfile (get_frame_function (frame));
 
-  jit_event_handler (gdbarch);
+  jit_event_handler (gdbarch, jiter);
 
   target_terminal::inferior ();
 }
diff --git a/gdb/jit.c b/gdb/jit.c
index 1b5ef46469e..1bfc80455b9 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -332,9 +332,9 @@ get_jit_program_space_data ()
    memory.  Returns 1 if all went well, 0 otherwise.  */
 
 static int
-jit_read_descriptor (struct gdbarch *gdbarch,
-		     struct jit_descriptor *descriptor,
-		     struct jit_program_space_data *ps_data)
+jit_read_descriptor (gdbarch *gdbarch,
+		     jit_descriptor *descriptor,
+		     objfile *jiter)
 {
   int err;
   struct type *ptr_type;
@@ -344,16 +344,16 @@ jit_read_descriptor (struct gdbarch *gdbarch,
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   struct jit_objfile_data *objf_data;
 
-  if (ps_data->objfile == NULL)
-    return 0;
-  objf_data = get_jit_objfile_data (ps_data->objfile);
+  gdb_assert (jiter != nullptr);
+
+  objf_data = get_jit_objfile_data (jiter);
   if (objf_data->descriptor == NULL)
     return 0;
 
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
 			"jit_read_descriptor, descriptor_addr = %s\n",
-			paddress (gdbarch, MSYMBOL_VALUE_ADDRESS (ps_data->objfile,
+			paddress (gdbarch, MSYMBOL_VALUE_ADDRESS (jiter,
 								  objf_data->descriptor)));
 
   /* Figure out how big the descriptor is on the remote and how to read it.  */
@@ -363,7 +363,7 @@ jit_read_descriptor (struct gdbarch *gdbarch,
   desc_buf = (gdb_byte *) alloca (desc_size);
 
   /* Read the descriptor.  */
-  err = target_read_memory (MSYMBOL_VALUE_ADDRESS (ps_data->objfile,
+  err = target_read_memory (MSYMBOL_VALUE_ADDRESS (jiter,
 						   objf_data->descriptor),
 			    desc_buf, desc_size);
   if (err)
@@ -1255,9 +1255,13 @@ jit_inferior_init (struct gdbarch *gdbarch)
   if (jit_breakpoint_re_set_internal (gdbarch, ps_data) != 0)
     return;
 
+  /* There must be a JITer registered, otherwise we would exit early
+     above.  */
+  objfile *jiter = ps_data->objfile;
+
   /* Read the descriptor so we can check the version number and load
      any already JITed functions.  */
-  if (!jit_read_descriptor (gdbarch, &descriptor, ps_data))
+  if (!jit_read_descriptor (gdbarch, &descriptor, jiter))
     return;
 
   /* Check that the version number agrees with that we support.  */
@@ -1330,7 +1334,7 @@ jit_inferior_exit_hook (struct inferior *inf)
 }
 
 void
-jit_event_handler (struct gdbarch *gdbarch)
+jit_event_handler (gdbarch *gdbarch, objfile *jiter)
 {
   struct jit_descriptor descriptor;
   struct jit_code_entry code_entry;
@@ -1338,8 +1342,7 @@ jit_event_handler (struct gdbarch *gdbarch)
   struct objfile *objf;
 
   /* Read the descriptor from remote memory.  */
-  if (!jit_read_descriptor (gdbarch, &descriptor,
-			    get_jit_program_space_data ()))
+  if (!jit_read_descriptor (gdbarch, &descriptor, jiter))
     return;
   entry_addr = descriptor.relevant_entry;
 
diff --git a/gdb/jit.h b/gdb/jit.h
index cc135037812..dddded0236f 100644
--- a/gdb/jit.h
+++ b/gdb/jit.h
@@ -20,6 +20,8 @@
 #ifndef JIT_H
 #define JIT_H
 
+#include "objfiles.h"
+
 /* When the JIT breakpoint fires, the inferior wants us to take one of
    these actions.  These values are used by the inferior, so the
    values of these enums cannot be changed.  */
@@ -78,6 +80,6 @@ extern void jit_breakpoint_re_set (void);
 /* This function is called by handle_inferior_event when it decides
    that the JIT event breakpoint has fired.  */
 
-extern void jit_event_handler (struct gdbarch *gdbarch);
+extern void jit_event_handler (gdbarch *gdbarch, objfile *jiter);
 
 #endif /* JIT_H */
-- 
2.17.1

[PATCH v2 2/3] gdb/jit: return bool in jit_breakpoint_re_set_internal and jit_read_descriptor

[Tankut Baris Aktemur]

This is a minor refactoring that converts the return type of
jit_read_descriptor and jit_breakpoint_re_set_internal functions
from 'int' to 'bool'.

gdb/ChangeLog:
2020-06-15  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* jit.c (jit_read_descriptor): Use bool as the return type.
	(jit_breakpoint_re_set_internal): Ditto.
	(jit_inferior_init): Update the call to
	jit_breakpoint_re_set_internal.
---
 gdb/jit.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 1bfc80455b9..5da0601b024 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -329,9 +329,9 @@ get_jit_program_space_data ()
 }
 
 /* Helper function for reading the global JIT descriptor from remote
-   memory.  Returns 1 if all went well, 0 otherwise.  */
+   memory.  Returns true if all went well, false otherwise.  */
 
-static int
+static bool
 jit_read_descriptor (gdbarch *gdbarch,
 		     jit_descriptor *descriptor,
 		     objfile *jiter)
@@ -348,7 +348,7 @@ jit_read_descriptor (gdbarch *gdbarch,
 
   objf_data = get_jit_objfile_data (jiter);
   if (objf_data->descriptor == NULL)
-    return 0;
+    return false;
 
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
@@ -370,7 +370,7 @@ jit_read_descriptor (gdbarch *gdbarch,
     {
       printf_unfiltered (_("Unable to read JIT descriptor from "
 			   "remote memory\n"));
-      return 0;
+      return false;
     }
 
   /* Fix the endianness to match the host.  */
@@ -381,7 +381,7 @@ jit_read_descriptor (gdbarch *gdbarch,
   descriptor->first_entry =
       extract_typed_address (&desc_buf[8 + ptr_size], ptr_type);
 
-  return 1;
+  return true;
 }
 
 /* Helper function for reading a JITed code entry from remote memory.  */
@@ -950,9 +950,9 @@ jit_breakpoint_deleted (struct breakpoint *b)
 }
 
 /* (Re-)Initialize the jit breakpoint if necessary.
-   Return 0 if the jit breakpoint has been successfully initialized.  */
+   Return true if the jit breakpoint has been successfully initialized.  */
 
-static int
+static bool
 jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 				struct jit_program_space_data *ps_data)
 {
@@ -968,13 +968,13 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
       reg_symbol = lookup_bound_minimal_symbol (jit_break_name);
       if (reg_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (reg_symbol) == 0)
-	return 1;
+	return false;
 
       desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL,
 					   reg_symbol.objfile);
       if (desc_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
-	return 1;
+	return false;
 
       objf_data = get_jit_objfile_data (reg_symbol.objfile);
       objf_data->register_code = reg_symbol.minsym;
@@ -994,7 +994,7 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 			paddress (gdbarch, addr));
 
   if (ps_data->cached_code_address == addr)
-    return 0;
+    return true;
 
   /* Delete the old breakpoint.  */
   if (ps_data->jit_breakpoint != NULL)
@@ -1004,7 +1004,7 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
   ps_data->cached_code_address = addr;
   ps_data->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
 
-  return 0;
+  return true;
 }
 
 /* The private data passed around in the frame unwind callback
@@ -1252,7 +1252,7 @@ jit_inferior_init (struct gdbarch *gdbarch)
   jit_prepend_unwinder (gdbarch);
 
   ps_data = get_jit_program_space_data ();
-  if (jit_breakpoint_re_set_internal (gdbarch, ps_data) != 0)
+  if (!jit_breakpoint_re_set_internal (gdbarch, ps_data))
     return;
 
   /* There must be a JITer registered, otherwise we would exit early
-- 
2.17.1

[PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Tankut Baris Aktemur]

GDB's JIT handler stores an objfile (and data associated with it) per
program space to keep track of JIT breakpoint information.  This
assumes that there is at most one JITer objfile in the program space.
However, there may be multiple.  If so, only the first JITer's hook
breakpoints would be realized and the JIT events from the other JITers
would be missed.  This patch replaces the single objfile pointer with
a list so that multiple objfiles can be tracked per program space.

This patch is viewed better with 'git diff -w'.

Regression-tested on X86_64 Linux.

gdb/ChangeLog:
2020-06-15  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* jit.c: (struct jiter_and_bp): New struct.
	(struct jit_program_space_data): Enable storing multiple objfiles.
	(jit_breakpoint_re_set_internal): Iterate over all objfiles in the
	program space to look for JIT symbols.
	(jit_breakpoint_deleted): Update to iterate over multiple jiters.
	(free_objfile_data): Ditto.
	(jit_inferior_init): Ditto.

gdb/testsuite/ChangeLog:
2020-06-15  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.base/jit-reader-simple.exp: Add a scenario for a binary that
	loads two JITers.
---
 gdb/jit.c                                    | 172 +++++++++++--------
 gdb/testsuite/gdb.base/jit-reader-simple.exp |  43 ++++-
 2 files changed, 144 insertions(+), 71 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 5da0601b024..2a15aac9e70 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -241,17 +241,15 @@ jit_reader_unload_command (const char *args, int from_tty)
   loaded_jit_reader = NULL;
 }
 
-/* Per-program space structure recording which objfile has the JIT
-   symbols.  */
+/* Structure recording JIT breakpoints per JITer objfile.  */
 
-struct jit_program_space_data
+struct jiter_and_bp
 {
-  /* The objfile.  This is NULL if no objfile holds the JIT
-     symbols.  */
+  /* The JITer objfile.  */
 
-  struct objfile *objfile = nullptr;
+  objfile *jiter = nullptr;
 
-  /* If this program space has __jit_debug_register_code, this is the
+  /* If this objfile has __jit_debug_register_code, this is the
      cached address from the minimal symbol.  This is used to detect
      relocations requiring the breakpoint to be re-created.  */
 
@@ -260,7 +258,17 @@ struct jit_program_space_data
   /* This is the JIT event breakpoint, or NULL if it has not been
      set.  */
 
-  struct breakpoint *jit_breakpoint = nullptr;
+  breakpoint *jit_breakpoint = nullptr;
+};
+
+/* Per-program space structure recording the objfiles and their JIT
+   symbols.  */
+
+struct jit_program_space_data
+{
+  /* The JIT breakpoint information associated with JITer objfiles.  */
+
+  std::forward_list<jiter_and_bp> jiter_and_bps;
 };
 
 static program_space_key<jit_program_space_data> jit_program_space_key;
@@ -941,16 +949,18 @@ jit_breakpoint_deleted (struct breakpoint *b)
       struct jit_program_space_data *ps_data;
 
       ps_data = jit_program_space_key.get (iter->pspace);
-      if (ps_data != NULL && ps_data->jit_breakpoint == iter->owner)
-	{
-	  ps_data->cached_code_address = 0;
-	  ps_data->jit_breakpoint = NULL;
-	}
+      if (ps_data != nullptr)
+	for (auto &info : ps_data->jiter_and_bps)
+	  if (info.jit_breakpoint == iter->owner)
+	    {
+	      info.cached_code_address = 0;
+	      info.jit_breakpoint = nullptr;
+	    }
     }
 }
 
-/* (Re-)Initialize the jit breakpoint if necessary.
-   Return true if the jit breakpoint has been successfully initialized.  */
+/* (Re-)Initialize the jit breakpoint if necessary.  Return true if
+   one or more jit breakpoints have been successfully initialized.  */
 
 static bool
 jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
@@ -961,50 +971,62 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
   struct jit_objfile_data *objf_data;
   CORE_ADDR addr;
 
-  if (ps_data->objfile == NULL)
+  for (objfile *the_objfile : current_program_space->objfiles ())
     {
       /* Lookup the registration symbol.  If it is missing, then we
 	 assume we are not attached to a JIT.  */
-      reg_symbol = lookup_bound_minimal_symbol (jit_break_name);
+      reg_symbol = lookup_minimal_symbol (jit_break_name, nullptr,
+					  the_objfile);
       if (reg_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (reg_symbol) == 0)
-	return false;
+	continue;
 
       desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL,
 					   reg_symbol.objfile);
       if (desc_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
-	return false;
+	continue;
 
       objf_data = get_jit_objfile_data (reg_symbol.objfile);
       objf_data->register_code = reg_symbol.minsym;
       objf_data->descriptor = desc_symbol.minsym;
 
-      ps_data->objfile = reg_symbol.objfile;
-    }
-  else
-    objf_data = get_jit_objfile_data (ps_data->objfile);
+      the_objfile = reg_symbol.objfile;
 
-  addr = MSYMBOL_VALUE_ADDRESS (ps_data->objfile, objf_data->register_code);
+      addr = MSYMBOL_VALUE_ADDRESS (the_objfile, objf_data->register_code);
 
-  if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog,
-			"jit_breakpoint_re_set_internal, "
-			"breakpoint_addr = %s\n",
-			paddress (gdbarch, addr));
+      if (jit_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "jit_breakpoint_re_set_internal, "
+			    "breakpoint_addr = %s\n",
+			    paddress (gdbarch, addr));
 
-  if (ps_data->cached_code_address == addr)
-    return true;
+      /* Search for an existing entry, or insert new if necessary.  */
+      auto iter = ps_data->jiter_and_bps.begin ();
+      for (; iter != ps_data->jiter_and_bps.end (); iter++)
+	if (iter->jiter == the_objfile)
+	  break;
 
-  /* Delete the old breakpoint.  */
-  if (ps_data->jit_breakpoint != NULL)
-    delete_breakpoint (ps_data->jit_breakpoint);
+      if (iter == ps_data->jiter_and_bps.end ())
+	{
+	  ps_data->jiter_and_bps.emplace_front ();
+	  iter = ps_data->jiter_and_bps.begin ();
+	  iter->jiter = the_objfile;
+	}
 
-  /* Put a breakpoint in the registration symbol.  */
-  ps_data->cached_code_address = addr;
-  ps_data->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
+      if (iter->cached_code_address == addr)
+	continue;
 
-  return true;
+      /* Delete the old breakpoint.  */
+      if (iter->jit_breakpoint != nullptr)
+	delete_breakpoint (iter->jit_breakpoint);
+
+      /* Put a breakpoint in the registration symbol.  */
+      iter->cached_code_address = addr;
+      iter->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
+    }
+
+  return !ps_data->jiter_and_bps.empty ();
 }
 
 /* The private data passed around in the frame unwind callback
@@ -1255,38 +1277,40 @@ jit_inferior_init (struct gdbarch *gdbarch)
   if (!jit_breakpoint_re_set_internal (gdbarch, ps_data))
     return;
 
-  /* There must be a JITer registered, otherwise we would exit early
-     above.  */
-  objfile *jiter = ps_data->objfile;
+  for (auto &info : ps_data->jiter_and_bps)
+    {
+      objfile *jiter = info.jiter;
 
-  /* Read the descriptor so we can check the version number and load
-     any already JITed functions.  */
-  if (!jit_read_descriptor (gdbarch, &descriptor, jiter))
-    return;
+      /* Read the descriptor so we can check the version number and load
+	 any already JITed functions.  */
+      if (!jit_read_descriptor (gdbarch, &descriptor, jiter))
+	continue;
 
-  /* Check that the version number agrees with that we support.  */
-  if (descriptor.version != 1)
-    {
-      printf_unfiltered (_("Unsupported JIT protocol version %ld "
-			   "in descriptor (expected 1)\n"),
-			 (long) descriptor.version);
-      return;
-    }
+      /* Check that the version number agrees with that we support.  */
+      if (descriptor.version != 1)
+	{
+	  printf_unfiltered (_("Unsupported JIT protocol version %ld "
+			       "in descriptor (expected 1)\n"),
+			     (long) descriptor.version);
+	  continue;
+	}
 
-  /* If we've attached to a running program, we need to check the descriptor
-     to register any functions that were already generated.  */
-  for (cur_entry_addr = descriptor.first_entry;
-       cur_entry_addr != 0;
-       cur_entry_addr = cur_entry.next_entry)
-    {
-      jit_read_code_entry (gdbarch, cur_entry_addr, &cur_entry);
+      /* If we've attached to a running program, we need to check the
+	 descriptor to register any functions that were already
+	 generated.  */
+      for (cur_entry_addr = descriptor.first_entry;
+	   cur_entry_addr != 0;
+	   cur_entry_addr = cur_entry.next_entry)
+	{
+	  jit_read_code_entry (gdbarch, cur_entry_addr, &cur_entry);
 
-      /* This hook may be called many times during setup, so make sure we don't
-	 add the same symbol file twice.  */
-      if (jit_find_objf_with_entry_addr (cur_entry_addr) != NULL)
-	continue;
+	  /* This hook may be called many times during setup, so make sure
+	     we don't add the same symbol file twice.  */
+	  if (jit_find_objf_with_entry_addr (cur_entry_addr) != NULL)
+	    continue;
 
-      jit_register_code (gdbarch, cur_entry_addr, &cur_entry);
+	  jit_register_code (gdbarch, cur_entry_addr, &cur_entry);
+	}
     }
 }
 
@@ -1383,12 +1407,20 @@ free_objfile_data (struct objfile *objfile, void *data)
       struct jit_program_space_data *ps_data;
 
       ps_data = jit_program_space_key.get (objfile->pspace);
-      if (ps_data != NULL && ps_data->objfile == objfile)
+      if (ps_data != nullptr)
 	{
-	  ps_data->objfile = NULL;
-	  if (ps_data->jit_breakpoint != NULL)
-	    delete_breakpoint (ps_data->jit_breakpoint);
-	  ps_data->cached_code_address = 0;
+	  /* Remove the data kept for objfile, but delete the
+	     breakpoint, too.  */
+	  ps_data->jiter_and_bps.remove_if ([=] (jiter_and_bp &info)
+	    {
+	      if (info.jiter == objfile)
+		{
+		  if (info.jit_breakpoint != nullptr)
+		    delete_breakpoint (info.jit_breakpoint);
+		  return true;
+		}
+	      return false;
+	    });
 	}
     }
 
diff --git a/gdb/testsuite/gdb.base/jit-reader-simple.exp b/gdb/testsuite/gdb.base/jit-reader-simple.exp
index 48bd326b533..d8a54a37ce7 100644
--- a/gdb/testsuite/gdb.base/jit-reader-simple.exp
+++ b/gdb/testsuite/gdb.base/jit-reader-simple.exp
@@ -34,6 +34,7 @@ standard_testfile
 set libname $testfile-jit
 set srcfile_lib $srcdir/$subdir/$libname.c
 set binfile_lib [standard_output_file $libname.so]
+set binfile_lib2 [standard_output_file ${libname}2.so]
 
 # Build a standalone JIT binary.
 
@@ -53,12 +54,15 @@ proc build_standalone_jit {{options ""}} {
 
 proc build_shared_jit {{options ""}} {
     global testfile
-    global srcfile_lib binfile_lib
+    global srcfile_lib binfile_lib binfile_lib2
 
     lappend options "debug additional_flags=-fPIC"
     if { [gdb_compile_shlib $srcfile_lib $binfile_lib $options] != "" } {
 	return -1
     }
+    if { [gdb_compile_shlib $srcfile_lib $binfile_lib2 $options] != "" } {
+	return -1
+    }
 
     return 0
 }
@@ -83,6 +87,15 @@ if {[gdb_compile ${srcdir}/${subdir}/${srcfile_dl} $binfile_dl executable \
     return -1
 }
 
+# Build the program that loads *two* JIT libraries.
+set binfile_dl2 $binfile-dl2
+set options [list debug shlib=${binfile_lib} shlib=${binfile_lib2}]
+if {[gdb_compile ${srcdir}/${subdir}/${srcfile_dl} $binfile_dl2 executable \
+	 $options] == -1 } {
+    untested "failed to compile two-jitter binary"
+    return -1
+}
+
 # STANDALONE is true when the JIT reader is included directly in the
 # main program.  False when the JIT reader is in a separate shared
 # library.  If CHANGE_ADDR is true, force changing the JIT descriptor
@@ -160,3 +173,31 @@ foreach standalone {1 0} {
 	}
     }
 }
+
+# Now start the program that loads two JITer libraries and expect to
+# see JIT breakpoints defined for both.
+
+with_test_prefix "two JITers" {
+    clean_restart $binfile_dl2
+
+    if {![runto_main]} {
+	untested "could not run to main for the two-JITer test"
+	return -1
+    }
+
+    set num_bps 0
+    set ws "\[ \t\]+"
+    gdb_test_multiple "maint info breakpoints" "have two jit breakpoints" {
+	-re "jit events${ws}keep y${ws}$hex <__jit_debug_register_code> inf 1\r\n" {
+	    incr num_bps
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	    if {$num_bps == 2} {
+		pass $gdb_test_name
+	    } else {
+		fail $gdb_test_name
+	    }
+	}
+    }
+}
-- 
2.17.1

Re: [PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Simon Marchi]

On 2020-06-16 5:49 a.m., Tankut Baris Aktemur wrote:
>        objf_data = get_jit_objfile_data (reg_symbol.objfile);
>        objf_data->register_code = reg_symbol.minsym;
>        objf_data->descriptor = desc_symbol.minsym;

This made me realize that we now have per-JITer-objfile information in both
jit_objfile_data and the new jiter_and_bp structure.  I think it would be more
consistent to bring it all in one of them.

I see two way forward:

1. Move register_code and descriptor from jit_objfile_data to jiter_and_bp.
   jiter_and_bp becomes the source of truth for information about JITer
   objfiles.  jit_objfile_data would only be used to decribe JITed objfiles.
2. Move the content of jiter_and_bp to jit_objfile_data.
   If we still think it's useful to have a list of JITer objfiles,
   jit_program_space_data::jiter_and_bps would become a list of objfiles
   known to be JITers:

     /* Objfiles in this program space that define the JIT interface symbols.  */
     std::forward_list<objfile *> jiters;

I would prefer #2, because using registries is kind of our standard way to keep
per-stuff data (where stuff is objfile, program_space, inferior, etc).

And instead of having the "jiters" list, when we want to iterate on jiters, maybe
we can just iterate on progspace->objfiles () and check those objfiles whose
jit_objfile_data indicate they are JITers.  The tradeoff is: it's a bit less
efficient, but the code is simpler (one less list to maintain, that is possibly
out of sync).  If the operations that require iterating on the existing JITers are
not on some hot path, it should be fine.

While digging into this, there are two improvements I'd make (orthogonal to this
patch):

- Make jit.c use the newer / type-safe / c++-friendly objfile_key instead of objfile_data.
- Split jit_objfile_data in two: one type for the JITers and one type for the JITed.  Even
  today, it is used for both, and that's kind of confusing.

I ended up trying all of this to make sure it made sense, instead of just throwing ideas
out there, so I took the time to make a "clean" branch.  See here:

  https://github.com/simark/binutils-gdb/tree/multiple-jiters

Please tell me what you think about this.  If you agree with the direction, I could
officially post it to the list.

There's one more thing I would improve (and I think it would also apply to your version
of the patch): in jit_breakpoint_re_set_internal, we keep looking up symbols in the same
libraries over and over.  Maybe we could mark the objfiles to say that we have already
looked up the symbols, as there's no point in looking them up again in the future.  Either
an objfile doesn't have them, and therefore will never have them.  Or it has them and we
already have them.  We could then just skip to the step where we check if the relocated
address has changed.

Simon

Re: [PATCH v2 2/3] gdb/jit: return bool in jit_breakpoint_re_set_internal and jit_read_descriptor

[Simon Marchi]

On 2020-06-16 5:49 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> This is a minor refactoring that converts the return type of
> jit_read_descriptor and jit_breakpoint_re_set_internal functions
> from 'int' to 'bool'.

Please mention that you are reversnig the return value logic of jit_breakpoint_re_set_internal,
otherwise at first glance it can look like a mistake.

Otherwise this LGTM, I think you can push it right away.

Simon

RE: [PATCH v2 2/3] gdb/jit: return bool in jit_breakpoint_re_set_internal and jit_read_descriptor

[Aktemur, Tankut Baris]

On Sunday, June 21, 2020 5:44 AM, Simon Marchi wrote:
> On 2020-06-16 5:49 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> > This is a minor refactoring that converts the return type of
> > jit_read_descriptor and jit_breakpoint_re_set_internal functions
> > from 'int' to 'bool'.
> 
> Please mention that you are reversnig the return value logic of jit_breakpoint_re_set_internal,
> otherwise at first glance it can look like a mistake.
> 
> Otherwise this LGTM, I think you can push it right away.
> 
> Simon

Pushed with that note in the commit message and ChangeLog entry.

Thanks.
-Baris


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 v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Pedro Alves]

On 6/21/20 4:32 AM, Simon Marchi wrote:
> 
> I would prefer #2, because using registries is kind of our standard way to keep
> per-stuff data (where stuff is objfile, program_space, inferior, etc).

I'd just like to point out that the main point of the registry mechanism
is dynamic registration, which is useful when you have parts of the
debugger that may or not be present in the final build.  E.g.,
per-stuff data that is only used by some -tdep.c file.  Otherwise,
if we're talking about data used by some module that is _always_
included in the built, then IMO the registry stuff is an unnecessary
abstraction.

Thanks,
Pedro Alves

Re: [PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Simon Marchi]

On 2020-06-22 12:28 p.m., Pedro Alves wrote:
> On 6/21/20 4:32 AM, Simon Marchi wrote:
>>
>> I would prefer #2, because using registries is kind of our standard way to keep
>> per-stuff data (where stuff is objfile, program_space, inferior, etc).
> 
> I'd just like to point out that the main point of the registry mechanism
> is dynamic registration, which is useful when you have parts of the
> debugger that may or not be present in the final build.  E.g.,
> per-stuff data that is only used by some -tdep.c file.  Otherwise,
> if we're talking about data used by some module that is _always_
> included in the built, then IMO the registry stuff is an unnecessary
> abstraction.

What would you use instead, an explicit field in the objfile structure?

Simon

RE: [PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Aktemur, Tankut Baris]

Hi Simon,

On Sunday, June 21, 2020 5:33 AM, Simon Marchi wrote:
> On 2020-06-16 5:49 a.m., Tankut Baris Aktemur wrote:
> >        objf_data = get_jit_objfile_data (reg_symbol.objfile);
> >        objf_data->register_code = reg_symbol.minsym;
> >        objf_data->descriptor = desc_symbol.minsym;
> 
> This made me realize that we now have per-JITer-objfile information in both
> jit_objfile_data and the new jiter_and_bp structure.  I think it would be more
> consistent to bring it all in one of them.
> 
> I see two way forward:
> 
> 1. Move register_code and descriptor from jit_objfile_data to jiter_and_bp.
>    jiter_and_bp becomes the source of truth for information about JITer
>    objfiles.  jit_objfile_data would only be used to decribe JITed objfiles.
> 2. Move the content of jiter_and_bp to jit_objfile_data.
>    If we still think it's useful to have a list of JITer objfiles,
>    jit_program_space_data::jiter_and_bps would become a list of objfiles
>    known to be JITers:
> 
>      /* Objfiles in this program space that define the JIT interface symbols.  */
>      std::forward_list<objfile *> jiters;
> 
> I would prefer #2, because using registries is kind of our standard way to keep
> per-stuff data (where stuff is objfile, program_space, inferior, etc).
> 
> And instead of having the "jiters" list, when we want to iterate on jiters, maybe
> we can just iterate on progspace->objfiles () and check those objfiles whose
> jit_objfile_data indicate they are JITers.  The tradeoff is: it's a bit less
> efficient, but the code is simpler (one less list to maintain, that is possibly
> out of sync).  If the operations that require iterating on the existing JITers are
> not on some hot path, it should be fine.
> 
> While digging into this, there are two improvements I'd make (orthogonal to this
> patch):
> 
> - Make jit.c use the newer / type-safe / c++-friendly objfile_key instead of objfile_data.
> - Split jit_objfile_data in two: one type for the JITers and one type for the JITed.  Even
>   today, it is used for both, and that's kind of confusing.
> 
> I ended up trying all of this to make sure it made sense, instead of just throwing ideas
> out there, so I took the time to make a "clean" branch.  See here:
> 
>   https://github.com/simark/binutils-gdb/tree/multiple-jiters
> 
> Please tell me what you think about this.  If you agree with the direction, I could
> officially post it to the list.

I looked at the patches.  It leads to a much wider cleanup.  Thank you.
This direction makes sense to me.

-Baris


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 v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Aktemur, Tankut Baris]

On Monday, June 22, 2020 6:40 PM, Simon Marchi wrote:
> On 2020-06-22 12:28 p.m., Pedro Alves wrote:
> > On 6/21/20 4:32 AM, Simon Marchi wrote:
> >>
> >> I would prefer #2, because using registries is kind of our standard way to keep
> >> per-stuff data (where stuff is objfile, program_space, inferior, etc).
> >
> > I'd just like to point out that the main point of the registry mechanism
> > is dynamic registration, which is useful when you have parts of the
> > debugger that may or not be present in the final build.  E.g.,
> > per-stuff data that is only used by some -tdep.c file.  Otherwise,
> > if we're talking about data used by some module that is _always_
> > included in the built, then IMO the registry stuff is an unnecessary
> > abstraction.
> 
> What would you use instead, an explicit field in the objfile structure?
> 
> Simon

Can we use an std::unordered_map instead of

  static const struct objfile_key<jit_objfile_data> jit_per_objfile;

?

-Baris


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 v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Pedro Alves]

On 6/22/20 5:39 PM, Simon Marchi wrote:
> On 2020-06-22 12:28 p.m., Pedro Alves wrote:
>> On 6/21/20 4:32 AM, Simon Marchi wrote:
>>>
>>> I would prefer #2, because using registries is kind of our standard way to keep
>>> per-stuff data (where stuff is objfile, program_space, inferior, etc).
>>
>> I'd just like to point out that the main point of the registry mechanism
>> is dynamic registration, which is useful when you have parts of the
>> debugger that may or not be present in the final build.  E.g.,
>> per-stuff data that is only used by some -tdep.c file.  Otherwise,
>> if we're talking about data used by some module that is _always_
>> included in the built, then IMO the registry stuff is an unnecessary
>> abstraction.
> 
> What would you use instead, an explicit field in the objfile structure?

Yes.

Thanks,
Pedro Alves

Re: [PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Simon Marchi]

On 2020-06-22 12:53 p.m., Aktemur, Tankut Baris via Gdb-patches wrote:
> On Monday, June 22, 2020 6:40 PM, Simon Marchi wrote:
>> On 2020-06-22 12:28 p.m., Pedro Alves wrote:
>>> On 6/21/20 4:32 AM, Simon Marchi wrote:
>>>>
>>>> I would prefer #2, because using registries is kind of our standard way to keep
>>>> per-stuff data (where stuff is objfile, program_space, inferior, etc).
>>>
>>> I'd just like to point out that the main point of the registry mechanism
>>> is dynamic registration, which is useful when you have parts of the
>>> debugger that may or not be present in the final build.  E.g.,
>>> per-stuff data that is only used by some -tdep.c file.  Otherwise,
>>> if we're talking about data used by some module that is _always_
>>> included in the built, then IMO the registry stuff is an unnecessary
>>> abstraction.
>>
>> What would you use instead, an explicit field in the objfile structure?
>>
>> Simon
> 
> Can we use an std::unordered_map instead of
> 
>   static const struct objfile_key<jit_objfile_data> jit_per_objfile;
> 
> ?
> 
> -Baris

An std::unordered_map with the objfile* as the key?  Isn't that pretty much
what objfile_key does, except that now you must manage it manually (make sure
the remove the entry when an objfile is deleted)?

Simon

RE: [PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Aktemur, Tankut Baris]

On Monday, June 22, 2020 7:00 PM, Simon Marchi wrote:
> On 2020-06-22 12:53 p.m., Aktemur, Tankut Baris via Gdb-patches wrote:
> > On Monday, June 22, 2020 6:40 PM, Simon Marchi wrote:
> >> On 2020-06-22 12:28 p.m., Pedro Alves wrote:
> >>> On 6/21/20 4:32 AM, Simon Marchi wrote:
> >>>>
> >>>> I would prefer #2, because using registries is kind of our standard way to keep
> >>>> per-stuff data (where stuff is objfile, program_space, inferior, etc).
> >>>
> >>> I'd just like to point out that the main point of the registry mechanism
> >>> is dynamic registration, which is useful when you have parts of the
> >>> debugger that may or not be present in the final build.  E.g.,
> >>> per-stuff data that is only used by some -tdep.c file.  Otherwise,
> >>> if we're talking about data used by some module that is _always_
> >>> included in the built, then IMO the registry stuff is an unnecessary
> >>> abstraction.
> >>
> >> What would you use instead, an explicit field in the objfile structure?
> >>
> >> Simon
> >
> > Can we use an std::unordered_map instead of
> >
> >   static const struct objfile_key<jit_objfile_data> jit_per_objfile;
> >
> > ?
> >
> > -Baris
> 
> An std::unordered_map with the objfile* as the key?  Isn't that pretty much
> what objfile_key does, except that now you must manage it manually (make sure
> the remove the entry when an objfile is deleted)?
> 
> Simon

That's what I meant, but it was because of a confusion with per-architecture data
(e.g. gdbarch_data).  Please ignore.

-Baris


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 v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Aktemur, Tankut Baris]

Hi Simon,

On Sunday, June 21, 2020 5:33 AM, Simon Marchi wrote:
> Please tell me what you think about this.  If you agree with the direction, I could
> officially post it to the list.

Any plan to post the series these days?

-Baris


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 v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Simon Marchi]

On 2020-06-30 4:17 a.m., Aktemur, Tankut Baris wrote:
> Hi Simon,
> 
> On Sunday, June 21, 2020 5:33 AM, Simon Marchi wrote:
>> Please tell me what you think about this.  If you agree with the direction, I could
>> officially post it to the list.
> 
> Any plan to post the series these days?
> 
> -Baris

Arf, I don't think I'll have time to polish and post it soon :(.  If this is on your critical
path, would you mind doing it?

One thing I didn't like and wanted to re-visit before posting is the fact that in
jit_breakpoint_re_set_internal, we always loop over all objfiles.  This means that when a new
objfile appears and we call jit_breakpoint_re_set_internal, we look up the magic symbols in all
the previously existing objfiles, even we have already looked them up earlier.  If we haven't
found the symbols in a a given objfile before, there's no need to search in that objfile again.

Or maybe that's not really a problem in practice.

Simon

RE: [PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles

[Aktemur, Tankut Baris]

On Friday, July 3, 2020 4:20 AM, Simon Marchi wrote:
> On 2020-06-30 4:17 a.m., Aktemur, Tankut Baris wrote:
> > Hi Simon,
> >
> > On Sunday, June 21, 2020 5:33 AM, Simon Marchi wrote:
> >> Please tell me what you think about this.  If you agree with the direction, I could
> >> officially post it to the list.
> >
> > Any plan to post the series these days?
> >
> > -Baris
> 
> Arf, I don't think I'll have time to polish and post it soon :(.  If this is on your
> critical
> path, would you mind doing it?

Sure, I'll work on it.

> One thing I didn't like and wanted to re-visit before posting is the fact that in
> jit_breakpoint_re_set_internal, we always loop over all objfiles.  This means that when a
> new
> objfile appears and we call jit_breakpoint_re_set_internal, we look up the magic symbols in
> all
> the previously existing objfiles, even we have already looked them up earlier.  If we
> haven't
> found the symbols in a a given objfile before, there's no need to search in that objfile
> again.

I'll look into this as well.

Thanks.
-Baris


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