[PATCH v3 0/9] Handling multiple JITers

[PATCH v3 0/9] Handling multiple JITers

[Tankut Baris Aktemur]

Hi,

This is v3 of the series originally submitted at

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

The v2 is at

  https://sourceware.org/pipermail/gdb-patches/2020-June/169539.html

Upon that revision, Simon Marchi had proposed

  https://sourceware.org/pipermail/gdb-patches/2020-June/169758.html

The updates in this revision v3 are as follows:

1. The patches mentioned by Simon in the link above have been cleaned up.

2. The 'jiter_data' and 'jited_data' fields have been added directly
   to the objfile struct as per Pedro's suggestion at
   https://sourceware.org/pipermail/gdb-patches/2020-June/169796.html

3. A final commit addresses Simon's comment of skipping JIT symbol
   lookup in an objfile if a lookup was already done and the symbols
   were not found.

Regards.
Baris

Simon Marchi (7):
  gdb/jit: link to jit_objfile_data directly from the objfile struct
  gdb/jit: split jit_objfile_data in two
  gdb/jit: apply some simplifications and assertions
  gdb/jit: move cached_code_address and jit_breakpoint to
    jiter_objfile_data
  gdb/jit: remove jiter_objfile_data -> objfile back-link
  gdb/jit: apply minor cleanup and modernization
  gdb/jit: skip jit symbol lookup if already detected the symbols don't
    exist

Tankut Baris Aktemur (2):
  gdb/jit: pass the jiter objfile as an argument to jit_event_handler
  gdb/jit: enable tracking multiple JITer objfiles

 gdb/breakpoint.c                             |   3 +-
 gdb/jit.c                                    | 357 +++++++------------
 gdb/jit.h                                    |   4 +-
 gdb/objfiles.h                               |  50 +++
 gdb/testsuite/gdb.base/jit-reader-simple.exp |  43 ++-
 5 files changed, 234 insertions(+), 223 deletions(-)

-- 
2.17.1

[PATCH v3 1/9] 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 6d81323dd92..414208469f9 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 e8a843de390..41ed81ab4b0 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -332,9 +332,9 @@ get_jit_program_space_data ()
    memory.  Returns true if all went well, false otherwise.  */
 
 static bool
-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 false;
-  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 false;
 
   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))
     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

Re: [PATCH v3 1/9] gdb/jit: pass the jiter objfile as an argument to jit_event_handler

[Simon Marchi]

On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> 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 6d81323dd92..414208469f9 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 e8a843de390..41ed81ab4b0 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -332,9 +332,9 @@ get_jit_program_space_data ()
>     memory.  Returns true if all went well, false otherwise.  */
>  
>  static bool
> -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 false;
> -  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 false;
>  
>    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))
>      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"

It's preferable to use forward declarations when possible:

struct objfile;

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

Can you document the new parameter?  I suppose it would be something like
"JITER is the objfiles whose JIT event breakpoint has been hit".

Simon

RE: [PATCH v3 1/9] gdb/jit: pass the jiter objfile as an argument to jit_event_handler

[Aktemur, Tankut Baris]

On Wednesday, July 15, 2020 3:49 PM, Simon Marchi wrote:
> On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> > 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"
> 
> It's preferable to use forward declarations when possible:
> 
> struct objfile;

Fixed in v4.

> > +
> >  /* 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.  */
> 
> Can you document the new parameter?  I suppose it would be something like
> "JITER is the objfiles whose JIT event breakpoint has been hit".

Added the comment in v4.

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

[PATCH v3 2/9] gdb/jit: link to jit_objfile_data directly from the objfile struct

[Tankut Baris Aktemur]

From: Simon Marchi <simon.marchi@polymtl.ca>

Remove the use of objfile_data to associate a jit_objfile_data with an
objfile.  Instead, directly link to a jit_objfile_data from an objfile
struct.  The goal is to eliminate unnecessary abstraction.

The free_objfile_data function naturally becomes the destructor of
jit_objfile_data.  However, free_objfile_data accesses the objfile to
which the data is attached, which the destructor of jit_objfile_data
doesn't have access to.  To work around this, add a backlink to the
owning objfile in jit_objfile_data.  This is however temporary, it goes
away in a subsequent patch.

gdb/ChangeLog:
2020-MM-DD  Simon Marchi  <simon.marchi@polymtl.ca>

	* objfiles.h (struct jit_objfile_data): Migrate to here from
	jit.c; also add a constructor, destructor, and an objfile* field.
	(struct objfile) <jit_data>: New field.
	* jit.c (jit_objfile_data): Remove.
	(struct jit_objfile_data): Migrate from here to objfiles.h.
	(jit_objfile_data::~jit_objfile_data): New destructor
	implementation with code moved from free_objfile_data.
	(free_objfile_data): Delete.
	(get_jit_objfile_data): Update to use the jit_data field of objfile.
	(jit_find_objf_with_entry_addr): Ditto.
	(jit_inferior_exit_hook): Ditto.
	(_initialize_jit): Remove the call to
	register_objfile_data_with_cleanup.
---
 gdb/jit.c      | 89 +++++++++++++-------------------------------------
 gdb/objfiles.h | 31 ++++++++++++++++++
 2 files changed, 54 insertions(+), 66 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 41ed81ab4b0..af0c9299708 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -45,8 +45,6 @@
 
 static std::string jit_reader_dir;
 
-static const struct objfile_data *jit_objfile_data;
-
 static const char *const jit_break_name = "__jit_debug_register_code";
 
 static const char *const jit_descriptor_name = "__jit_debug_descriptor";
@@ -265,24 +263,25 @@ struct jit_program_space_data
 
 static program_space_key<jit_program_space_data> jit_program_space_key;
 
-/* Per-objfile structure recording the addresses in the program space.
-   This object serves two purposes: for ordinary objfiles, it may
-   cache some symbols related to the JIT interface; and for
-   JIT-created objfiles, it holds some information about the
-   jit_code_entry.  */
+/* Destructor for jit_objfile_data.  */
 
-struct jit_objfile_data
+jit_objfile_data::~jit_objfile_data ()
 {
-  /* Symbol for __jit_debug_register_code.  */
-  struct minimal_symbol *register_code;
-
-  /* Symbol for __jit_debug_descriptor.  */
-  struct minimal_symbol *descriptor;
+  /* Free the data allocated in the jit_program_space_data slot.  */
+  if (this->register_code != NULL)
+    {
+      struct jit_program_space_data *ps_data;
 
-  /* Address of struct jit_code_entry in this objfile.  This is only
-     non-zero for objfiles that represent code created by the JIT.  */
-  CORE_ADDR addr;
-};
+      ps_data = jit_program_space_key.get (this->objfile->pspace);
+      if (ps_data != NULL && ps_data->objfile == this->objfile)
+	{
+	  ps_data->objfile = NULL;
+	  if (ps_data->jit_breakpoint != NULL)
+	    delete_breakpoint (ps_data->jit_breakpoint);
+	  ps_data->cached_code_address = 0;
+	}
+    }
+}
 
 /* Fetch the jit_objfile_data associated with OBJF.  If no data exists
    yet, make a new structure and attach it.  */
@@ -290,16 +289,10 @@ struct jit_objfile_data
 static struct jit_objfile_data *
 get_jit_objfile_data (struct objfile *objf)
 {
-  struct jit_objfile_data *objf_data;
-
-  objf_data = (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
-  if (objf_data == NULL)
-    {
-      objf_data = XCNEW (struct jit_objfile_data);
-      set_objfile_data (objf, jit_objfile_data, objf_data);
-    }
+  if (objf->jit_data == nullptr)
+    objf->jit_data.reset (new jit_objfile_data (objf));
 
-  return objf_data;
+  return objf->jit_data.get ();
 }
 
 /* Remember OBJFILE has been created for struct jit_code_entry located
@@ -914,14 +907,9 @@ static struct objfile *
 jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
 {
   for (objfile *objf : current_program_space->objfiles ())
-    {
-      struct jit_objfile_data *objf_data;
+    if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
+      return objf;
 
-      objf_data
-	= (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
-      if (objf_data != NULL && objf_data->addr == entry_addr)
-	return objf;
-    }
   return NULL;
 }
 
@@ -1324,13 +1312,8 @@ static void
 jit_inferior_exit_hook (struct inferior *inf)
 {
   for (objfile *objf : current_program_space->objfiles_safe ())
-    {
-      struct jit_objfile_data *objf_data
-	= (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
-
-      if (objf_data != NULL && objf_data->addr != 0)
-	objf->unlink ();
-    }
+    if (objf->jit_data != nullptr && objf->jit_data->addr != 0)
+      objf->unlink ();
 }
 
 void
@@ -1371,30 +1354,6 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
     }
 }
 
-/* Called to free the data allocated to the jit_program_space_data slot.  */
-
-static void
-free_objfile_data (struct objfile *objfile, void *data)
-{
-  struct jit_objfile_data *objf_data = (struct jit_objfile_data *) data;
-
-  if (objf_data->register_code != NULL)
-    {
-      struct jit_program_space_data *ps_data;
-
-      ps_data = jit_program_space_key.get (objfile->pspace);
-      if (ps_data != NULL && ps_data->objfile == objfile)
-	{
-	  ps_data->objfile = NULL;
-	  if (ps_data->jit_breakpoint != NULL)
-	    delete_breakpoint (ps_data->jit_breakpoint);
-	  ps_data->cached_code_address = 0;
-	}
-    }
-
-  xfree (data);
-}
-
 /* Initialize the jit_gdbarch_data slot with an instance of struct
    jit_gdbarch_data_type */
 
@@ -1427,8 +1386,6 @@ _initialize_jit ()
   gdb::observers::inferior_exit.attach (jit_inferior_exit_hook);
   gdb::observers::breakpoint_deleted.attach (jit_breakpoint_deleted);
 
-  jit_objfile_data =
-    register_objfile_data_with_cleanup (NULL, free_objfile_data);
   jit_gdbarch_data = gdbarch_data_register_pre_init (jit_gdbarch_data_init);
   if (is_dl_available ())
     {
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 56ff52119dc..6bb4dd0a1d6 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -407,6 +407,34 @@ class separate_debug_range
   struct objfile *m_objfile;
 };
 
+/* Per-objfile structure recording the addresses in the program space.
+   This object serves two purposes: for ordinary objfiles, it may
+   cache some symbols related to the JIT interface; and for
+   JIT-created objfiles, it holds some information about the
+   jit_code_entry.  */
+
+struct jit_objfile_data
+{
+  jit_objfile_data (struct objfile *objfile)
+    : objfile (objfile)
+  {}
+
+  ~jit_objfile_data ();
+
+  /* Back-link to the objfile. */
+  struct objfile *objfile;
+
+  /* Symbol for __jit_debug_register_code.  */
+  minimal_symbol *register_code = nullptr;
+
+  /* Symbol for __jit_debug_descriptor.  */
+  minimal_symbol *descriptor = nullptr;
+
+  /* Address of struct jit_code_entry in this objfile.  This is only
+     non-zero for objfiles that represent code created by the JIT.  */
+  CORE_ADDR addr = 0;
+};
+
 /* Master structure for keeping track of each file from which
    gdb reads symbols.  There are several ways these get allocated: 1.
    The main symbol file, symfile_objfile, set by the symbol-file command,
@@ -697,6 +725,9 @@ struct objfile
      store these here rather than in struct block.  Static links must be
      allocated on the objfile's obstack.  */
   htab_up static_links;
+
+  /* JIT-related data for this objfile.  */
+  std::unique_ptr<jit_objfile_data> jit_data = nullptr;
 };
 
 /* A deleter for objfile.  */
-- 
2.17.1

Re: [PATCH v3 2/9] gdb/jit: link to jit_objfile_data directly from the objfile struct

[Simon Marchi]

On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> Remove the use of objfile_data to associate a jit_objfile_data with an
> objfile.  Instead, directly link to a jit_objfile_data from an objfile
> struct.  The goal is to eliminate unnecessary abstraction.
> 
> The free_objfile_data function naturally becomes the destructor of
> jit_objfile_data.  However, free_objfile_data accesses the objfile to
> which the data is attached, which the destructor of jit_objfile_data
> doesn't have access to.  To work around this, add a backlink to the
> owning objfile in jit_objfile_data.  This is however temporary, it goes
> away in a subsequent patch.
> 
> gdb/ChangeLog:
> 2020-MM-DD  Simon Marchi  <simon.marchi@polymtl.ca>
> 
> 	* objfiles.h (struct jit_objfile_data): Migrate to here from
> 	jit.c; also add a constructor, destructor, and an objfile* field.
> 	(struct objfile) <jit_data>: New field.
> 	* jit.c (jit_objfile_data): Remove.
> 	(struct jit_objfile_data): Migrate from here to objfiles.h.
> 	(jit_objfile_data::~jit_objfile_data): New destructor
> 	implementation with code moved from free_objfile_data.
> 	(free_objfile_data): Delete.
> 	(get_jit_objfile_data): Update to use the jit_data field of objfile.
> 	(jit_find_objf_with_entry_addr): Ditto.
> 	(jit_inferior_exit_hook): Ditto.
> 	(_initialize_jit): Remove the call to
> 	register_objfile_data_with_cleanup.
> ---
>  gdb/jit.c      | 89 +++++++++++++-------------------------------------
>  gdb/objfiles.h | 31 ++++++++++++++++++
>  2 files changed, 54 insertions(+), 66 deletions(-)
> 
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 41ed81ab4b0..af0c9299708 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -45,8 +45,6 @@
>  
>  static std::string jit_reader_dir;
>  
> -static const struct objfile_data *jit_objfile_data;
> -
>  static const char *const jit_break_name = "__jit_debug_register_code";
>  
>  static const char *const jit_descriptor_name = "__jit_debug_descriptor";
> @@ -265,24 +263,25 @@ struct jit_program_space_data
>  
>  static program_space_key<jit_program_space_data> jit_program_space_key;
>  
> -/* Per-objfile structure recording the addresses in the program space.
> -   This object serves two purposes: for ordinary objfiles, it may
> -   cache some symbols related to the JIT interface; and for
> -   JIT-created objfiles, it holds some information about the
> -   jit_code_entry.  */
> +/* Destructor for jit_objfile_data.  */
>  
> -struct jit_objfile_data
> +jit_objfile_data::~jit_objfile_data ()
>  {
> -  /* Symbol for __jit_debug_register_code.  */
> -  struct minimal_symbol *register_code;
> -
> -  /* Symbol for __jit_debug_descriptor.  */
> -  struct minimal_symbol *descriptor;
> +  /* Free the data allocated in the jit_program_space_data slot.  */
> +  if (this->register_code != NULL)
> +    {
> +      struct jit_program_space_data *ps_data;
>  
> -  /* Address of struct jit_code_entry in this objfile.  This is only
> -     non-zero for objfiles that represent code created by the JIT.  */
> -  CORE_ADDR addr;
> -};
> +      ps_data = jit_program_space_key.get (this->objfile->pspace);
> +      if (ps_data != NULL && ps_data->objfile == this->objfile)
> +	{
> +	  ps_data->objfile = NULL;
> +	  if (ps_data->jit_breakpoint != NULL)
> +	    delete_breakpoint (ps_data->jit_breakpoint);
> +	  ps_data->cached_code_address = 0;
> +	}
> +    }
> +}
>  
>  /* Fetch the jit_objfile_data associated with OBJF.  If no data exists
>     yet, make a new structure and attach it.  */
> @@ -290,16 +289,10 @@ struct jit_objfile_data
>  static struct jit_objfile_data *
>  get_jit_objfile_data (struct objfile *objf)
>  {
> -  struct jit_objfile_data *objf_data;
> -
> -  objf_data = (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> -  if (objf_data == NULL)
> -    {
> -      objf_data = XCNEW (struct jit_objfile_data);
> -      set_objfile_data (objf, jit_objfile_data, objf_data);
> -    }
> +  if (objf->jit_data == nullptr)
> +    objf->jit_data.reset (new jit_objfile_data (objf));
>  
> -  return objf_data;
> +  return objf->jit_data.get ();
>  }
>  
>  /* Remember OBJFILE has been created for struct jit_code_entry located
> @@ -914,14 +907,9 @@ static struct objfile *
>  jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
>  {
>    for (objfile *objf : current_program_space->objfiles ())
> -    {
> -      struct jit_objfile_data *objf_data;
> +    if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
> +      return objf;

Keep the braces for the for here:

  for (objfile *objf : current_program_space->objfiles ())
    {
      if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
	return objf;
    }

as described somewhere here:

https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#Syntactic-Conventions

>  
> -      objf_data
> -	= (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> -      if (objf_data != NULL && objf_data->addr == entry_addr)
> -	return objf;
> -    }
>    return NULL;
>  }
>  
> @@ -1324,13 +1312,8 @@ static void
>  jit_inferior_exit_hook (struct inferior *inf)
>  {
>    for (objfile *objf : current_program_space->objfiles_safe ())
> -    {
> -      struct jit_objfile_data *objf_data
> -	= (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> -
> -      if (objf_data != NULL && objf_data->addr != 0)
> -	objf->unlink ();
> -    }
> +    if (objf->jit_data != nullptr && objf->jit_data->addr != 0)
> +      objf->unlink ();

Same here.

>  }
>  
>  void
> @@ -1371,30 +1354,6 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
>      }
>  }
>  
> -/* Called to free the data allocated to the jit_program_space_data slot.  */
> -
> -static void
> -free_objfile_data (struct objfile *objfile, void *data)
> -{
> -  struct jit_objfile_data *objf_data = (struct jit_objfile_data *) data;
> -
> -  if (objf_data->register_code != NULL)
> -    {
> -      struct jit_program_space_data *ps_data;
> -
> -      ps_data = jit_program_space_key.get (objfile->pspace);
> -      if (ps_data != NULL && ps_data->objfile == objfile)
> -	{
> -	  ps_data->objfile = NULL;
> -	  if (ps_data->jit_breakpoint != NULL)
> -	    delete_breakpoint (ps_data->jit_breakpoint);
> -	  ps_data->cached_code_address = 0;
> -	}
> -    }
> -
> -  xfree (data);
> -}
> -
>  /* Initialize the jit_gdbarch_data slot with an instance of struct
>     jit_gdbarch_data_type */
>  
> @@ -1427,8 +1386,6 @@ _initialize_jit ()
>    gdb::observers::inferior_exit.attach (jit_inferior_exit_hook);
>    gdb::observers::breakpoint_deleted.attach (jit_breakpoint_deleted);
>  
> -  jit_objfile_data =
> -    register_objfile_data_with_cleanup (NULL, free_objfile_data);
>    jit_gdbarch_data = gdbarch_data_register_pre_init (jit_gdbarch_data_init);
>    if (is_dl_available ())
>      {
> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index 56ff52119dc..6bb4dd0a1d6 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -407,6 +407,34 @@ class separate_debug_range
>    struct objfile *m_objfile;
>  };
>  
> +/* Per-objfile structure recording the addresses in the program space.
> +   This object serves two purposes: for ordinary objfiles, it may
> +   cache some symbols related to the JIT interface; and for
> +   JIT-created objfiles, it holds some information about the
> +   jit_code_entry.  */
> +
> +struct jit_objfile_data
> +{
> +  jit_objfile_data (struct objfile *objfile)
> +    : objfile (objfile)
> +  {}
> +
> +  ~jit_objfile_data ();
> +
> +  /* Back-link to the objfile. */
> +  struct objfile *objfile;
> +
> +  /* Symbol for __jit_debug_register_code.  */
> +  minimal_symbol *register_code = nullptr;
> +
> +  /* Symbol for __jit_debug_descriptor.  */
> +  minimal_symbol *descriptor = nullptr;
> +
> +  /* Address of struct jit_code_entry in this objfile.  This is only
> +     non-zero for objfiles that represent code created by the JIT.  */
> +  CORE_ADDR addr = 0;
> +};

This could reside in jit.h, if you don't include `objfiles.h` in `jit.h`
(which I don't think you need).

I'm wondering if the JIT data now needs to be explicitly reset when the objfile's
symbols get re-read (reread_symbols), or when re-running and the base address
of the objfile changes. "free_objfile_data" might have been automatically invoked
in either or both of these cases.

A good test would be, with `set disable-randomization off`, try running a JITer program
twice.  On the second run, will the `jit_objfile_data` still hold addresses of the first
run, which are not good anymore?

And as a follow up test, try rebuilding the JITer program in between the two runs (witout
restarting GDB) so that addresses of __jit_debug_register_code and __jit_debug_descriptor.
GDB should call reread_symbols, does it all work as expected?

Simon

RE: [PATCH v3 2/9] gdb/jit: link to jit_objfile_data directly from the objfile struct

[Aktemur, Tankut Baris]

On Wednesday, July 15, 2020 4:17 PM, Simon Marchi wrote:
> On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> > From: Simon Marchi <simon.marchi@polymtl.ca>
> >
> > Remove the use of objfile_data to associate a jit_objfile_data with an
> > objfile.  Instead, directly link to a jit_objfile_data from an objfile
> > struct.  The goal is to eliminate unnecessary abstraction.
> >
> > The free_objfile_data function naturally becomes the destructor of
> > jit_objfile_data.  However, free_objfile_data accesses the objfile to
> > which the data is attached, which the destructor of jit_objfile_data
> > doesn't have access to.  To work around this, add a backlink to the
> > owning objfile in jit_objfile_data.  This is however temporary, it goes
> > away in a subsequent patch.
> >
> > gdb/ChangeLog:
> > 2020-MM-DD  Simon Marchi  <simon.marchi@polymtl.ca>
> >
> > 	* objfiles.h (struct jit_objfile_data): Migrate to here from
> > 	jit.c; also add a constructor, destructor, and an objfile* field.
> > 	(struct objfile) <jit_data>: New field.
> > 	* jit.c (jit_objfile_data): Remove.
> > 	(struct jit_objfile_data): Migrate from here to objfiles.h.
> > 	(jit_objfile_data::~jit_objfile_data): New destructor
> > 	implementation with code moved from free_objfile_data.
> > 	(free_objfile_data): Delete.
> > 	(get_jit_objfile_data): Update to use the jit_data field of objfile.
> > 	(jit_find_objf_with_entry_addr): Ditto.
> > 	(jit_inferior_exit_hook): Ditto.
> > 	(_initialize_jit): Remove the call to
> > 	register_objfile_data_with_cleanup.
> > ---
> >  gdb/jit.c      | 89 +++++++++++++-------------------------------------
> >  gdb/objfiles.h | 31 ++++++++++++++++++
> >  2 files changed, 54 insertions(+), 66 deletions(-)
> >
> > diff --git a/gdb/jit.c b/gdb/jit.c
> > index 41ed81ab4b0..af0c9299708 100644
> > --- a/gdb/jit.c
> > +++ b/gdb/jit.c
> > @@ -45,8 +45,6 @@
> >
> >  static std::string jit_reader_dir;
> >
> > -static const struct objfile_data *jit_objfile_data;
> > -
> >  static const char *const jit_break_name = "__jit_debug_register_code";
> >
> >  static const char *const jit_descriptor_name = "__jit_debug_descriptor";
> > @@ -265,24 +263,25 @@ struct jit_program_space_data
> >
> >  static program_space_key<jit_program_space_data> jit_program_space_key;
> >
> > -/* Per-objfile structure recording the addresses in the program space.
> > -   This object serves two purposes: for ordinary objfiles, it may
> > -   cache some symbols related to the JIT interface; and for
> > -   JIT-created objfiles, it holds some information about the
> > -   jit_code_entry.  */
> > +/* Destructor for jit_objfile_data.  */
> >
> > -struct jit_objfile_data
> > +jit_objfile_data::~jit_objfile_data ()
> >  {
> > -  /* Symbol for __jit_debug_register_code.  */
> > -  struct minimal_symbol *register_code;
> > -
> > -  /* Symbol for __jit_debug_descriptor.  */
> > -  struct minimal_symbol *descriptor;
> > +  /* Free the data allocated in the jit_program_space_data slot.  */
> > +  if (this->register_code != NULL)
> > +    {
> > +      struct jit_program_space_data *ps_data;
> >
> > -  /* Address of struct jit_code_entry in this objfile.  This is only
> > -     non-zero for objfiles that represent code created by the JIT.  */
> > -  CORE_ADDR addr;
> > -};
> > +      ps_data = jit_program_space_key.get (this->objfile->pspace);
> > +      if (ps_data != NULL && ps_data->objfile == this->objfile)
> > +	{
> > +	  ps_data->objfile = NULL;
> > +	  if (ps_data->jit_breakpoint != NULL)
> > +	    delete_breakpoint (ps_data->jit_breakpoint);
> > +	  ps_data->cached_code_address = 0;
> > +	}
> > +    }
> > +}
> >
> >  /* Fetch the jit_objfile_data associated with OBJF.  If no data exists
> >     yet, make a new structure and attach it.  */
> > @@ -290,16 +289,10 @@ struct jit_objfile_data
> >  static struct jit_objfile_data *
> >  get_jit_objfile_data (struct objfile *objf)
> >  {
> > -  struct jit_objfile_data *objf_data;
> > -
> > -  objf_data = (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> > -  if (objf_data == NULL)
> > -    {
> > -      objf_data = XCNEW (struct jit_objfile_data);
> > -      set_objfile_data (objf, jit_objfile_data, objf_data);
> > -    }
> > +  if (objf->jit_data == nullptr)
> > +    objf->jit_data.reset (new jit_objfile_data (objf));
> >
> > -  return objf_data;
> > +  return objf->jit_data.get ();
> >  }
> >
> >  /* Remember OBJFILE has been created for struct jit_code_entry located
> > @@ -914,14 +907,9 @@ static struct objfile *
> >  jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
> >  {
> >    for (objfile *objf : current_program_space->objfiles ())
> > -    {
> > -      struct jit_objfile_data *objf_data;
> > +    if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
> > +      return objf;
> 
> Keep the braces for the for here:
> 
>   for (objfile *objf : current_program_space->objfiles ())
>     {
>       if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
> 	return objf;
>     }
> 
> as described somewhere here:
> 
> https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#Syntactic-
> Conventions

I thought this convention was specifically about nested if-statements.
Fixed in v4.

> >
> > -      objf_data
> > -	= (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> > -      if (objf_data != NULL && objf_data->addr == entry_addr)
> > -	return objf;
> > -    }
> >    return NULL;
> >  }
> >
> > @@ -1324,13 +1312,8 @@ static void
> >  jit_inferior_exit_hook (struct inferior *inf)
> >  {
> >    for (objfile *objf : current_program_space->objfiles_safe ())
> > -    {
> > -      struct jit_objfile_data *objf_data
> > -	= (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> > -
> > -      if (objf_data != NULL && objf_data->addr != 0)
> > -	objf->unlink ();
> > -    }
> > +    if (objf->jit_data != nullptr && objf->jit_data->addr != 0)
> > +      objf->unlink ();
> 
> Same here.

Fixed in v4.

> >  }
> >
> >  void
> > @@ -1371,30 +1354,6 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
> >      }
> >  }
> >
> > -/* Called to free the data allocated to the jit_program_space_data slot.  */
> > -
> > -static void
> > -free_objfile_data (struct objfile *objfile, void *data)
> > -{
> > -  struct jit_objfile_data *objf_data = (struct jit_objfile_data *) data;
> > -
> > -  if (objf_data->register_code != NULL)
> > -    {
> > -      struct jit_program_space_data *ps_data;
> > -
> > -      ps_data = jit_program_space_key.get (objfile->pspace);
> > -      if (ps_data != NULL && ps_data->objfile == objfile)
> > -	{
> > -	  ps_data->objfile = NULL;
> > -	  if (ps_data->jit_breakpoint != NULL)
> > -	    delete_breakpoint (ps_data->jit_breakpoint);
> > -	  ps_data->cached_code_address = 0;
> > -	}
> > -    }
> > -
> > -  xfree (data);
> > -}
> > -
> >  /* Initialize the jit_gdbarch_data slot with an instance of struct
> >     jit_gdbarch_data_type */
> >
> > @@ -1427,8 +1386,6 @@ _initialize_jit ()
> >    gdb::observers::inferior_exit.attach (jit_inferior_exit_hook);
> >    gdb::observers::breakpoint_deleted.attach (jit_breakpoint_deleted);
> >
> > -  jit_objfile_data =
> > -    register_objfile_data_with_cleanup (NULL, free_objfile_data);
> >    jit_gdbarch_data = gdbarch_data_register_pre_init (jit_gdbarch_data_init);
> >    if (is_dl_available ())
> >      {
> > diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> > index 56ff52119dc..6bb4dd0a1d6 100644
> > --- a/gdb/objfiles.h
> > +++ b/gdb/objfiles.h
> > @@ -407,6 +407,34 @@ class separate_debug_range
> >    struct objfile *m_objfile;
> >  };
> >
> > +/* Per-objfile structure recording the addresses in the program space.
> > +   This object serves two purposes: for ordinary objfiles, it may
> > +   cache some symbols related to the JIT interface; and for
> > +   JIT-created objfiles, it holds some information about the
> > +   jit_code_entry.  */
> > +
> > +struct jit_objfile_data
> > +{
> > +  jit_objfile_data (struct objfile *objfile)
> > +    : objfile (objfile)
> > +  {}
> > +
> > +  ~jit_objfile_data ();
> > +
> > +  /* Back-link to the objfile. */
> > +  struct objfile *objfile;
> > +
> > +  /* Symbol for __jit_debug_register_code.  */
> > +  minimal_symbol *register_code = nullptr;
> > +
> > +  /* Symbol for __jit_debug_descriptor.  */
> > +  minimal_symbol *descriptor = nullptr;
> > +
> > +  /* Address of struct jit_code_entry in this objfile.  This is only
> > +     non-zero for objfiles that represent code created by the JIT.  */
> > +  CORE_ADDR addr = 0;
> > +};
> 
> This could reside in jit.h, if you don't include `objfiles.h` in `jit.h`
> (which I don't think you need).

I moved this to `jit.h` in v4.  Please note that `objfiles.h` will now include
`jit.h` because it needs to have access to `jit_objfile_data` due to the
new unique_ptr field in objfile struct.  Since objfile is much of a central
concept and a container of JIT data, this seemed reasonable to me.

> I'm wondering if the JIT data now needs to be explicitly reset when the objfile's
> symbols get re-read (reread_symbols), or when re-running and the base address
> of the objfile changes. "free_objfile_data" might have been automatically invoked
> in either or both of these cases.
> 
> A good test would be, with `set disable-randomization off`, try running a JITer program
> twice.  On the second run, will the `jit_objfile_data` still hold addresses of the first
> run, which are not good anymore?
> 
> And as a follow up test, try rebuilding the JITer program in between the two runs (witout
> restarting GDB) so that addresses of __jit_debug_register_code and __jit_debug_descriptor.
> GDB should call reread_symbols, does it all work as expected?

I did these tests manually.  Here is the status.

If the JITer is a linked solib file, the `target_pre_inferior` indirectly causes the 
objfile to be unlinked and thus the `jit_objfile_data` associated with it to be
destructed.  So, for both scenarios, right before the second run, the JITer objfile is
initialized from fresh.

If the JITer is the main objfile, it is not unlinked.  So, its `jit_objfile_data` is
not destroyed.  For the second scenario (i.e. reread_symbols), the `objfiles_changed`
function is invoked.  It might be a proper spot to reset the JIT data.  For the first
scenario, I'm not sure what the best spot for a reset would be.

However, I don't think a reset is needed at all.  The `jit_breakpoint_re_set_internal`
function is invoked each time an objfile is loaded.  This means, at the beginning of
a run and later when a JITer or a non-JITer solib file is loaded, the symbols will be
looked up again and the addresses will be overwritten.  Or am I missing something?

I'll post v4 after this is clarified.
 
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 v3 2/9] gdb/jit: link to jit_objfile_data directly from the objfile struct

[Simon Marchi]

On 2020-07-21 12:25 p.m., Aktemur, Tankut Baris wrote:
> However, I don't think a reset is needed at all.  The `jit_breakpoint_re_set_internal`
> function is invoked each time an objfile is loaded.  This means, at the beginning of
> a run and later when a JITer or a non-JITer solib file is loaded, the symbols will be
> looked up again and the addresses will be overwritten.  Or am I missing something?

That makes sense, I just wanted to make sure we considered this case.

Simon

[PATCH v3 3/9] gdb/jit: split jit_objfile_data in two

[Tankut Baris Aktemur]

From: Simon Marchi <simon.marchi@polymtl.ca>

The jit_objfile_data is currently used to hold information about both
objfiles that are the result of JIT compilation (JITed) and objfiles
that can produce JITed objfiles (JITers).  I think that this double use
of the type is confusing, and that things would be more obvious if we
had one type for each role.

This patch splits it into:

- jited_objfile_data: for data about an objfile that is the result of a
  JIT compilation
- jiter_objfile_data: for data about an objfile which produces JITed
  objfiles

There are now two JIT-related fields in an objfile, one for each kind.
With this change, the following invariants hold:

- an objfile has a non-null `jiter_data` field iff it defines the required
  symbols of the JIT interface
- an objfile has a non-null `jited_data` field iff it is the product of
  JIT compilation (has been produced by some JITer)

gdb/ChangeLog:
2020-MM-DD  Simon Marchi  <simon.marchi@polymtl.ca>

	* objfiles.h (struct jit_objfile_data):  Split into...
	(struct jiter_objfile_data): ... this ...
	(struct jited_objfile_data): ... and this.
	(struct objfile) <jit_data>: Remove.
	<jiter_data, jited_data>: New fields.
	* jit.c (jit_objfile_data::~jit_objfile_data): Rename to ...
	(jiter_objfile_data::~jiter_objfile_data): ... this.
	(get_jit_objfile_data): Rename to ...
	(get_jiter_objfile_data): ... this.
	(add_objfile_entry): Update.
	(jit_read_descriptor): Use get_jiter_objfile_data.
	(jit_find_objf_with_entry_addr): Use objfile's jited_data field.
	(jit_breakpoint_re_set_internal): Use get_jiter_objfile_data.
	(jit_inferior_exit_hook): Use objfile's jited_data field.
---
 gdb/jit.c      | 34 ++++++++++++++++------------------
 gdb/objfiles.h | 36 ++++++++++++++++++++++++------------
 2 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index af0c9299708..218871ed745 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -263,9 +263,9 @@ struct jit_program_space_data
 
 static program_space_key<jit_program_space_data> jit_program_space_key;
 
-/* Destructor for jit_objfile_data.  */
+/* Destructor for jiter_objfile_data.  */
 
-jit_objfile_data::~jit_objfile_data ()
+jiter_objfile_data::~jiter_objfile_data ()
 {
   /* Free the data allocated in the jit_program_space_data slot.  */
   if (this->register_code != NULL)
@@ -283,16 +283,16 @@ jit_objfile_data::~jit_objfile_data ()
     }
 }
 
-/* Fetch the jit_objfile_data associated with OBJF.  If no data exists
+/* Fetch the jiter_objfile_data associated with OBJF.  If no data exists
    yet, make a new structure and attach it.  */
 
-static struct jit_objfile_data *
-get_jit_objfile_data (struct objfile *objf)
+static jiter_objfile_data *
+get_jiter_objfile_data (objfile *objf)
 {
-  if (objf->jit_data == nullptr)
-    objf->jit_data.reset (new jit_objfile_data (objf));
+  if (objf->jiter_data == nullptr)
+    objf->jiter_data.reset (new jiter_objfile_data (objf));
 
-  return objf->jit_data.get ();
+  return objf->jiter_data.get ();
 }
 
 /* Remember OBJFILE has been created for struct jit_code_entry located
@@ -301,10 +301,9 @@ get_jit_objfile_data (struct objfile *objf)
 static void
 add_objfile_entry (struct objfile *objfile, CORE_ADDR entry)
 {
-  struct jit_objfile_data *objf_data;
+  gdb_assert (objfile->jited_data == nullptr);
 
-  objf_data = get_jit_objfile_data (objfile);
-  objf_data->addr = entry;
+  objfile->jited_data.reset (new jited_objfile_data (entry));
 }
 
 /* Return jit_program_space_data for current program space.  Allocate
@@ -335,10 +334,9 @@ jit_read_descriptor (gdbarch *gdbarch,
   int desc_size;
   gdb_byte *desc_buf;
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  struct jit_objfile_data *objf_data;
 
   gdb_assert (jiter != nullptr);
-  objf_data = get_jit_objfile_data (jiter);
+  jiter_objfile_data *objf_data = get_jiter_objfile_data (jiter);
 
   if (objf_data->descriptor == NULL)
     return false;
@@ -907,7 +905,7 @@ static struct objfile *
 jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
 {
   for (objfile *objf : current_program_space->objfiles ())
-    if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
+    if (objf->jited_data != nullptr && objf->jited_data->addr == entry_addr)
       return objf;
 
   return NULL;
@@ -946,7 +944,7 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 {
   struct bound_minimal_symbol reg_symbol;
   struct bound_minimal_symbol desc_symbol;
-  struct jit_objfile_data *objf_data;
+  jiter_objfile_data *objf_data;
   CORE_ADDR addr;
 
   if (ps_data->objfile == NULL)
@@ -964,14 +962,14 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 	  || BMSYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
 	return false;
 
-      objf_data = get_jit_objfile_data (reg_symbol.objfile);
+      objf_data = get_jiter_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);
+    objf_data = get_jiter_objfile_data (ps_data->objfile);
 
   addr = MSYMBOL_VALUE_ADDRESS (ps_data->objfile, objf_data->register_code);
 
@@ -1312,7 +1310,7 @@ static void
 jit_inferior_exit_hook (struct inferior *inf)
 {
   for (objfile *objf : current_program_space->objfiles_safe ())
-    if (objf->jit_data != nullptr && objf->jit_data->addr != 0)
+    if (objf->jited_data != nullptr && objf->jited_data->addr != 0)
       objf->unlink ();
 }
 
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 6bb4dd0a1d6..89cc3a20366 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -407,19 +407,16 @@ class separate_debug_range
   struct objfile *m_objfile;
 };
 
-/* Per-objfile structure recording the addresses in the program space.
-   This object serves two purposes: for ordinary objfiles, it may
-   cache some symbols related to the JIT interface; and for
-   JIT-created objfiles, it holds some information about the
-   jit_code_entry.  */
+/* An objfile that defines the required symbols of the JIT interface has an
+   instance of this type attached to it.  */
 
-struct jit_objfile_data
+struct jiter_objfile_data
 {
-  jit_objfile_data (struct objfile *objfile)
+  jiter_objfile_data (struct objfile *objfile)
     : objfile (objfile)
   {}
 
-  ~jit_objfile_data ();
+  ~jiter_objfile_data ();
 
   /* Back-link to the objfile. */
   struct objfile *objfile;
@@ -429,10 +426,20 @@ struct jit_objfile_data
 
   /* Symbol for __jit_debug_descriptor.  */
   minimal_symbol *descriptor = nullptr;
+};
+
+/* An objfile that is the product of JIT compilation and was registered
+   using the JIT interface has an instance of this type attached to it.  */
+
+struct jited_objfile_data
+{
+  jited_objfile_data (CORE_ADDR addr)
+    : addr (addr)
+  {}
 
-  /* Address of struct jit_code_entry in this objfile.  This is only
+  /* Address of struct jit_code_entry for this objfile.  This is only
      non-zero for objfiles that represent code created by the JIT.  */
-  CORE_ADDR addr = 0;
+  CORE_ADDR addr;
 };
 
 /* Master structure for keeping track of each file from which
@@ -726,8 +733,13 @@ struct objfile
      allocated on the objfile's obstack.  */
   htab_up static_links;
 
-  /* JIT-related data for this objfile.  */
-  std::unique_ptr<jit_objfile_data> jit_data = nullptr;
+  /* JIT-related data for this objfile, if the objfile is a JITer;
+     that is, it produces JITed objfiles.  */
+  std::unique_ptr<jiter_objfile_data> jiter_data = nullptr;
+
+  /* JIT-related data for this objfile, if the objfile is JITed;
+     that is, it was produced by a JITer.  */
+  std::unique_ptr<jited_objfile_data> jited_data = nullptr;
 };
 
 /* A deleter for objfile.  */
-- 
2.17.1

Re: [PATCH v3 3/9] gdb/jit: split jit_objfile_data in two

[Simon Marchi]

On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> @@ -429,10 +426,20 @@ struct jit_objfile_data
>  
>    /* Symbol for __jit_debug_descriptor.  */
>    minimal_symbol *descriptor = nullptr;
> +};
> +
> +/* An objfile that is the product of JIT compilation and was registered
> +   using the JIT interface has an instance of this type attached to it.  */
> +
> +struct jited_objfile_data
> +{
> +  jited_objfile_data (CORE_ADDR addr)
> +    : addr (addr)
> +  {}
>  
> -  /* Address of struct jit_code_entry in this objfile.  This is only
> +  /* Address of struct jit_code_entry for this objfile.  This is only
>       non-zero for objfiles that represent code created by the JIT.  */
> -  CORE_ADDR addr = 0;
> +  CORE_ADDR addr;

I think the "This is only non-zero..." part of the comment can be removed?

Simon

RE: [PATCH v3 3/9] gdb/jit: split jit_objfile_data in two

[Aktemur, Tankut Baris]

On Wednesday, July 15, 2020 4:19 PM, Simon Marchi wrote:
> On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> > @@ -429,10 +426,20 @@ struct jit_objfile_data
> >
> >    /* Symbol for __jit_debug_descriptor.  */
> >    minimal_symbol *descriptor = nullptr;
> > +};
> > +
> > +/* An objfile that is the product of JIT compilation and was registered
> > +   using the JIT interface has an instance of this type attached to it.  */
> > +
> > +struct jited_objfile_data
> > +{
> > +  jited_objfile_data (CORE_ADDR addr)
> > +    : addr (addr)
> > +  {}
> >
> > -  /* Address of struct jit_code_entry in this objfile.  This is only
> > +  /* Address of struct jit_code_entry for this objfile.  This is only
> >       non-zero for objfiles that represent code created by the JIT.  */
> > -  CORE_ADDR addr = 0;
> > +  CORE_ADDR addr;
> 
> I think the "This is only non-zero..." part of the comment can be removed?

I agree.  Removed the comment in v4.

-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

[PATCH v3 4/9] gdb/jit: apply some simplifications and assertions

[Tankut Baris Aktemur]

From: Simon Marchi <simon.marchi@polymtl.ca>

Following patch "gdb/jit: split jit_objfile_data in two", there are some
simplifications we can make.  The invariants described there mean that
we can assume / assert some things instead of checking them using
conditionals.

If an instance of jiter_objfile_data exists for a given objfile, it's
because the required JIT interface symbols were found.  Therefore, in
~jiter_objfile_data, the `register_code` field can't be NULL.  It was
previously used to differentiate a jit_objfile_data object used for a
JITer vs a JITed.  We can remove that check.

If an instance of jiter_objfile_data exists for a given objfile, it's
because it's the sole JITer objfile in the scope of its program space
(jit_program_space_data::objfile points to it).  At the moment,
jit_breakpoint_re_set_internal won't create a second instance of
jiter_objfile_data for a given program space.  Therefore, it's not
necessary to check for `ps_data != NULL` in ~jiter_objfile_data: we know
a jit_program_space_data for that program space exists.  We also don't
need to check for `ps_data->objfile == this->objfile`, because we know
the objfile is the sole JITer in this program space.  Replace these two
conditions with assertions.

A pre-condition for calling the jit_read_descriptor function (which is
respected in the two call sites) is that the objfile `jiter` _is_ a
JITer - it already has a jiter_objfile_data attached to it.  When a
jiter_objfile_data exists, its `descriptor` field is necessarily set:
had the descriptor symbol not been found, jit_breakpoint_re_set_internal
would not have created the jiter_objfile_data.  Remove the check and
early return in jit_read_descriptor.  Access objfile's `jiter_data` field
directly instead of calling `get_jiter_objfile_data` (which creates the
jiter_objfile_data if it doesn't exist yet) and assert that the result
is not nullptr.

Finally, `jit_event_handler` is always passed a JITer objfile.  So, add
an assertion to ensure that.

gdb/ChangeLog:
2020-MM-DD  Simon Marchi  <simon.marchi@polymtl.ca>

	* jit.c (jiter_objfile_data::~jiter_objfile_data): Remove some
	checks.
	(jit_read_descriptor): Remove NULL check.
	(jit_event_handler): Add an assertion.
---
 gdb/jit.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 218871ed745..e5b6aa707ec 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -267,20 +267,17 @@ static program_space_key<jit_program_space_data> jit_program_space_key;
 
 jiter_objfile_data::~jiter_objfile_data ()
 {
-  /* Free the data allocated in the jit_program_space_data slot.  */
-  if (this->register_code != NULL)
-    {
-      struct jit_program_space_data *ps_data;
+  jit_program_space_data *ps_data
+    = jit_program_space_key.get (this->objfile->pspace);
 
-      ps_data = jit_program_space_key.get (this->objfile->pspace);
-      if (ps_data != NULL && ps_data->objfile == this->objfile)
-	{
-	  ps_data->objfile = NULL;
-	  if (ps_data->jit_breakpoint != NULL)
-	    delete_breakpoint (ps_data->jit_breakpoint);
-	  ps_data->cached_code_address = 0;
-	}
-    }
+  gdb_assert (ps_data != nullptr);
+  gdb_assert (ps_data->objfile == this->objfile);
+
+  ps_data->objfile = NULL;
+  if (ps_data->jit_breakpoint != NULL)
+    delete_breakpoint (ps_data->jit_breakpoint);
+
+  ps_data->cached_code_address = 0;
 }
 
 /* Fetch the jiter_objfile_data associated with OBJF.  If no data exists
@@ -336,10 +333,8 @@ jit_read_descriptor (gdbarch *gdbarch,
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
 
   gdb_assert (jiter != nullptr);
-  jiter_objfile_data *objf_data = get_jiter_objfile_data (jiter);
-
-  if (objf_data->descriptor == NULL)
-    return false;
+  jiter_objfile_data *objf_data = jiter->jiter_data.get ();
+  gdb_assert (objf_data != nullptr);
 
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
@@ -1322,6 +1317,10 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
   CORE_ADDR entry_addr;
   struct objfile *objf;
 
+  /* If we get a JIT breakpoint event for this objfile, it is necessarily a
+     JITer.  */
+  gdb_assert (jiter->jiter_data != nullptr);
+
   /* Read the descriptor from remote memory.  */
   if (!jit_read_descriptor (gdbarch, &descriptor, jiter))
     return;
-- 
2.17.1

[PATCH v3 5/9] gdb/jit: move cached_code_address and jit_breakpoint to jiter_objfile_data

[Tankut Baris Aktemur]

From: Simon Marchi <simon.marchi@polymtl.ca>

This is in preparation for allowing more than one JITer objfile per
program space.  Once we do that, each JITer objfile will have its own
JIT breakpoint (on the __jit_debug_register_code function it provides).
The cached_code_address field is just the runtime / relocated address of
that symbol.

Since they are going to become JITer-objfile-specific and not
program-space-specific, move these fields from jit_program_space_data to
jiter_objfile_data.

gdb/ChangeLog:
2020-MM-DD  Simon Marchi  <simon.marchi@polymtl.ca>

	* objfiles.h (struct jiter_objfile_data) <cached_code_address,
	jit_breakpoint>: Move here from ...
	* jit.c (jit_program_space_data): ... here.
	(jiter_objfile_data::~jiter_objfile_data): Update.
	(jit_breakpoint_deleted): Update.
	(jit_breakpoint_re_set_internal): Update.
---
 gdb/jit.c      | 41 +++++++++++++++++------------------------
 gdb/objfiles.h |  8 ++++++++
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index e5b6aa707ec..e51b6231946 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -248,17 +248,6 @@ struct jit_program_space_data
      symbols.  */
 
   struct objfile *objfile = nullptr;
-
-  /* If this program space 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.  */
-
-  CORE_ADDR cached_code_address = 0;
-
-  /* This is the JIT event breakpoint, or NULL if it has not been
-     set.  */
-
-  struct breakpoint *jit_breakpoint = nullptr;
 };
 
 static program_space_key<jit_program_space_data> jit_program_space_key;
@@ -273,11 +262,9 @@ jiter_objfile_data::~jiter_objfile_data ()
   gdb_assert (ps_data != nullptr);
   gdb_assert (ps_data->objfile == this->objfile);
 
-  ps_data->objfile = NULL;
-  if (ps_data->jit_breakpoint != NULL)
-    delete_breakpoint (ps_data->jit_breakpoint);
-
-  ps_data->cached_code_address = 0;
+  ps_data->objfile = nullptr;
+  if (this->jit_breakpoint != nullptr)
+    delete_breakpoint (this->jit_breakpoint);
 }
 
 /* Fetch the jiter_objfile_data associated with OBJF.  If no data exists
@@ -922,10 +909,16 @@ 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)
+      if (ps_data != nullptr && ps_data->objfile != nullptr)
 	{
-	  ps_data->cached_code_address = 0;
-	  ps_data->jit_breakpoint = NULL;
+	  objfile *objf = ps_data->objfile;
+	  jiter_objfile_data *jiter_data = objf->jiter_data.get ();
+
+	  if (jiter_data->jit_breakpoint == iter->owner)
+	    {
+	      jiter_data->cached_code_address = 0;
+	      jiter_data->jit_breakpoint = nullptr;
+	    }
 	}
     }
 }
@@ -974,16 +967,16 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 			"breakpoint_addr = %s\n",
 			paddress (gdbarch, addr));
 
-  if (ps_data->cached_code_address == addr)
+  if (objf_data->cached_code_address == addr)
     return true;
 
   /* Delete the old breakpoint.  */
-  if (ps_data->jit_breakpoint != NULL)
-    delete_breakpoint (ps_data->jit_breakpoint);
+  if (objf_data->jit_breakpoint != nullptr)
+    delete_breakpoint (objf_data->jit_breakpoint);
 
   /* Put a breakpoint in the registration symbol.  */
-  ps_data->cached_code_address = addr;
-  ps_data->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
+  objf_data->cached_code_address = addr;
+  objf_data->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
 
   return true;
 }
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 89cc3a20366..d508f3fab52 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -426,6 +426,14 @@ struct jiter_objfile_data
 
   /* Symbol for __jit_debug_descriptor.  */
   minimal_symbol *descriptor = nullptr;
+
+  /* This is the relocated address of the __jit_debug_register_code function
+     provided by this objfile.  This is used to detect relocations changes
+     requiring the breakpoint to be re-created.  */
+  CORE_ADDR cached_code_address = 0;
+
+  /* This is the JIT event breakpoint, or nullptr if it has been deleted.  */
+  breakpoint *jit_breakpoint = nullptr;
 };
 
 /* An objfile that is the product of JIT compilation and was registered
-- 
2.17.1

[PATCH v3 6/9] gdb/jit: enable tracking multiple JITer 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 removes that assumption, allowing an arbitrary number of
objfiles within a program space to be JITers.

- The "unique" program_space -> JITer objfile pointer in
  jit_program_space_data is removed.  In fact, jit_program_space_data
  becomes empty, so it is removed entirely.

- jit_breakpoint_deleted is modified, it now has to assume that any
  objfile in a program space is a potential JITer.  It now iterates on
  all objfiles, checking if they are indeed JITers, and if they are,
  whether the deleted breakpoint belongs to them.

- jit_breakpoint_re_set_internal also has to assume that any objfile in
  a program space is a potential JITer.  It creates (or updates) one
  jiter_objfile_data structure for each JITer it finds.

- Same for jit_inferior_init.  It now iterates all objfiles to read the
  initial JIT object list.

gdb/ChangeLog:
2020-MM-DD  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
	    Simon Marchi  <simon.marchi@polymtl.ca>

	* jit.c (struct jit_program_space_data): Remove.
	(jit_program_space_key): Remove.
	(jiter_objfile_data::~jiter_objfile_data): Remove program space
	stuff.
	(get_jit_program_space_data): Remove.
	(jit_breakpoint_deleted): Iterate on all of the program space's
	objfiles.
	(jit_inferior_init): Likewise.
	(jit_breakpoint_re_set_internal): Likewise.  Also change return
	type to void.
	(jit_breakpoint_re_set): Pass current_program_space to
	jit_breakpoint_re_set_internal.

gdb/testsuite/ChangeLog:
2020-07-13  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                                    | 171 +++++++------------
 gdb/testsuite/gdb.base/jit-reader-simple.exp |  43 ++++-
 2 files changed, 105 insertions(+), 109 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index e51b6231946..87872ac9018 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -239,30 +239,10 @@ 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.  */
-
-struct jit_program_space_data
-{
-  /* The objfile.  This is NULL if no objfile holds the JIT
-     symbols.  */
-
-  struct objfile *objfile = nullptr;
-};
-
-static program_space_key<jit_program_space_data> jit_program_space_key;
-
 /* Destructor for jiter_objfile_data.  */
 
 jiter_objfile_data::~jiter_objfile_data ()
 {
-  jit_program_space_data *ps_data
-    = jit_program_space_key.get (this->objfile->pspace);
-
-  gdb_assert (ps_data != nullptr);
-  gdb_assert (ps_data->objfile == this->objfile);
-
-  ps_data->objfile = nullptr;
   if (this->jit_breakpoint != nullptr)
     delete_breakpoint (this->jit_breakpoint);
 }
@@ -290,20 +270,6 @@ add_objfile_entry (struct objfile *objfile, CORE_ADDR entry)
   objfile->jited_data.reset (new jited_objfile_data (entry));
 }
 
-/* Return jit_program_space_data for current program space.  Allocate
-   if not already present.  */
-
-static struct jit_program_space_data *
-get_jit_program_space_data ()
-{
-  struct jit_program_space_data *ps_data;
-
-  ps_data = jit_program_space_key.get (current_program_space);
-  if (ps_data == NULL)
-    ps_data = jit_program_space_key.emplace (current_program_space);
-  return ps_data;
-}
-
 /* Helper function for reading the global JIT descriptor from remote
    memory.  Returns true if all went well, false otherwise.  */
 
@@ -906,15 +872,12 @@ jit_breakpoint_deleted (struct breakpoint *b)
 
   for (iter = b->loc; iter != NULL; iter = iter->next)
     {
-      struct jit_program_space_data *ps_data;
-
-      ps_data = jit_program_space_key.get (iter->pspace);
-      if (ps_data != nullptr && ps_data->objfile != nullptr)
+      for (objfile *objf : iter->pspace->objfiles ())
 	{
-	  objfile *objf = ps_data->objfile;
 	  jiter_objfile_data *jiter_data = objf->jiter_data.get ();
 
-	  if (jiter_data->jit_breakpoint == iter->owner)
+	  if (jiter_data != nullptr
+	      && jiter_data->jit_breakpoint == iter->owner)
 	    {
 	      jiter_data->cached_code_address = 0;
 	      jiter_data->jit_breakpoint = nullptr;
@@ -923,62 +886,55 @@ jit_breakpoint_deleted (struct breakpoint *b)
     }
 }
 
-/* (Re-)Initialize the jit breakpoint if necessary.
-   Return true if the jit breakpoint has been successfully initialized.  */
+/* (Re-)Initialize the jit breakpoints for JIT-producing objfiles in
+   PSPACE.  */
 
-static bool
-jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
-				struct jit_program_space_data *ps_data)
+static void
+jit_breakpoint_re_set_internal (struct gdbarch *gdbarch, program_space *pspace)
 {
-  struct bound_minimal_symbol reg_symbol;
-  struct bound_minimal_symbol desc_symbol;
   jiter_objfile_data *objf_data;
-  CORE_ADDR addr;
 
-  if (ps_data->objfile == NULL)
+  for (objfile *the_objfile : pspace->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);
+      bound_minimal_symbol 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);
+      bound_minimal_symbol desc_symbol
+	= lookup_minimal_symbol (jit_descriptor_name, NULL, the_objfile);
       if (desc_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
-	return false;
+	continue;
 
       objf_data = get_jiter_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_jiter_objfile_data (ps_data->objfile);
-
-  addr = MSYMBOL_VALUE_ADDRESS (ps_data->objfile, objf_data->register_code);
-
-  if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog,
-			"jit_breakpoint_re_set_internal, "
-			"breakpoint_addr = %s\n",
-			paddress (gdbarch, addr));
+      CORE_ADDR addr = MSYMBOL_VALUE_ADDRESS (the_objfile,
+					      objf_data->register_code);
 
-  if (objf_data->cached_code_address == addr)
-    return true;
+      if (jit_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "jit_breakpoint_re_set_internal, "
+			    "breakpoint_addr = %s\n",
+			    paddress (gdbarch, addr));
 
-  /* Delete the old breakpoint.  */
-  if (objf_data->jit_breakpoint != nullptr)
-    delete_breakpoint (objf_data->jit_breakpoint);
+      /* Check if we need to re-create the breakpoint.  */
+      if (objf_data->cached_code_address == addr)
+	continue;
 
-  /* Put a breakpoint in the registration symbol.  */
-  objf_data->cached_code_address = addr;
-  objf_data->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
+      /* Delete the old breakpoint.  */
+      if (objf_data->jit_breakpoint != nullptr)
+	delete_breakpoint (objf_data->jit_breakpoint);
 
-  return true;
+      /* Put a breakpoint in the registration symbol.  */
+      objf_data->cached_code_address = addr;
+      objf_data->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
+    }
 }
 
 /* The private data passed around in the frame unwind callback
@@ -1217,7 +1173,6 @@ jit_inferior_init (struct gdbarch *gdbarch)
 {
   struct jit_descriptor descriptor;
   struct jit_code_entry cur_entry;
-  struct jit_program_space_data *ps_data;
   CORE_ADDR cur_entry_addr;
 
   if (jit_debug)
@@ -1225,42 +1180,43 @@ 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))
-    return;
+  jit_breakpoint_re_set_internal (gdbarch, current_program_space);
 
-  /* There must be a JITer registered, otherwise we would exit early
-     above.  */
-  objfile *jiter = ps_data->objfile;
+  for (objfile *jiter : current_program_space->objfiles ())
+    {
+      if (jiter->jiter_data == nullptr)
+	continue;
 
-  /* 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);
+	}
     }
 }
 
@@ -1286,8 +1242,7 @@ jit_inferior_created_hook (void)
 void
 jit_breakpoint_re_set (void)
 {
-  jit_breakpoint_re_set_internal (target_gdbarch (),
-				  get_jit_program_space_data ());
+  jit_breakpoint_re_set_internal (target_gdbarch (), current_program_space);
 }
 
 /* This function cleans up any code entries left over when the
diff --git a/gdb/testsuite/gdb.base/jit-reader-simple.exp b/gdb/testsuite/gdb.base/jit-reader-simple.exp
index 48bd326b533..a8f33c6d7a8 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"
+	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

[PATCH v3 7/9] gdb/jit: remove jiter_objfile_data -> objfile back-link

[Tankut Baris Aktemur]

From: Simon Marchi <simon.marchi@polymtl.ca>

This is no longer needed, remove it.

gdb/ChangeLog:
2020-MM-DD  Simon Marchi  <simon.marchi@polymtl.ca>

	* objfiles.h (struct jiter_objfile_data) <jiter_objfile_data>:
	Remove.
	<objfile>: Remove.
	* jit.c (get_jiter_objfile_data): Update.
---
 gdb/jit.c      | 2 +-
 gdb/objfiles.h | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 87872ac9018..e15b67107af 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -254,7 +254,7 @@ static jiter_objfile_data *
 get_jiter_objfile_data (objfile *objf)
 {
   if (objf->jiter_data == nullptr)
-    objf->jiter_data.reset (new jiter_objfile_data (objf));
+    objf->jiter_data.reset (new jiter_objfile_data ());
 
   return objf->jiter_data.get ();
 }
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index d508f3fab52..e5828618dfe 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -412,15 +412,8 @@ class separate_debug_range
 
 struct jiter_objfile_data
 {
-  jiter_objfile_data (struct objfile *objfile)
-    : objfile (objfile)
-  {}
-
   ~jiter_objfile_data ();
 
-  /* Back-link to the objfile. */
-  struct objfile *objfile;
-
   /* Symbol for __jit_debug_register_code.  */
   minimal_symbol *register_code = nullptr;
 
-- 
2.17.1

[PATCH v3 8/9] gdb/jit: apply minor cleanup and modernization

[Tankut Baris Aktemur]

From: Simon Marchi <simon.marchi@polymtl.ca>

gdb/ChangeLog:
2020-07-14  Simon Marchi  <simon.marchi@polymtl.ca>
	    Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* jit.c (jit_read_descriptor): Define the descriptor address once,
	use twice.
	(jit_breakpoint_deleted): Move the declaration of the loop variable
	`iter` into the loop header.
	(jit_breakpoint_re_set_internal): Move the declaration of the local
	variable `objf_data` to the first point of definition.
	(jit_event_handler): Move the declaration of local variables
	`code_entry`, `entry_addr`, and `objf` to their first point of use.
	Rename `objf` to `jited`.
---
 gdb/jit.c | 53 +++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index e15b67107af..1850e5b143e 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -289,11 +289,12 @@ jit_read_descriptor (gdbarch *gdbarch,
   jiter_objfile_data *objf_data = jiter->jiter_data.get ();
   gdb_assert (objf_data != nullptr);
 
+  CORE_ADDR addr = MSYMBOL_VALUE_ADDRESS (jiter, objf_data->descriptor);
+
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
 			"jit_read_descriptor, descriptor_addr = %s\n",
-			paddress (gdbarch, MSYMBOL_VALUE_ADDRESS (jiter,
-								  objf_data->descriptor)));
+			paddress (gdbarch, addr));
 
   /* Figure out how big the descriptor is on the remote and how to read it.  */
   ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
@@ -302,9 +303,7 @@ jit_read_descriptor (gdbarch *gdbarch,
   desc_buf = (gdb_byte *) alloca (desc_size);
 
   /* Read the descriptor.  */
-  err = target_read_memory (MSYMBOL_VALUE_ADDRESS (jiter,
-						   objf_data->descriptor),
-			    desc_buf, desc_size);
+  err = target_read_memory (addr, desc_buf, desc_size);
   if (err)
     {
       printf_unfiltered (_("Unable to read JIT descriptor from "
@@ -865,12 +864,10 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
 static void
 jit_breakpoint_deleted (struct breakpoint *b)
 {
-  struct bp_location *iter;
-
   if (b->type != bp_jit_event)
     return;
 
-  for (iter = b->loc; iter != NULL; iter = iter->next)
+  for (bp_location *iter = b->loc; iter != nullptr; iter = iter->next)
     {
       for (objfile *objf : iter->pspace->objfiles ())
 	{
@@ -892,8 +889,6 @@ jit_breakpoint_deleted (struct breakpoint *b)
 static void
 jit_breakpoint_re_set_internal (struct gdbarch *gdbarch, program_space *pspace)
 {
-  jiter_objfile_data *objf_data;
-
   for (objfile *the_objfile : pspace->objfiles ())
     {
       /* Lookup the registration symbol.  If it is missing, then we
@@ -910,7 +905,8 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch, program_space *pspace)
 	  || BMSYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
 	continue;
 
-      objf_data = get_jiter_objfile_data (reg_symbol.objfile);
+      jiter_objfile_data *objf_data
+	= get_jiter_objfile_data (reg_symbol.objfile);
       objf_data->register_code = reg_symbol.minsym;
       objf_data->descriptor = desc_symbol.minsym;
 
@@ -1261,9 +1257,6 @@ void
 jit_event_handler (gdbarch *gdbarch, objfile *jiter)
 {
   struct jit_descriptor descriptor;
-  struct jit_code_entry code_entry;
-  CORE_ADDR entry_addr;
-  struct objfile *objf;
 
   /* If we get a JIT breakpoint event for this objfile, it is necessarily a
      JITer.  */
@@ -1272,27 +1265,35 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
   /* Read the descriptor from remote memory.  */
   if (!jit_read_descriptor (gdbarch, &descriptor, jiter))
     return;
-  entry_addr = descriptor.relevant_entry;
+  CORE_ADDR entry_addr = descriptor.relevant_entry;
 
   /* Do the corresponding action.  */
   switch (descriptor.action_flag)
     {
     case JIT_NOACTION:
       break;
+
     case JIT_REGISTER:
-      jit_read_code_entry (gdbarch, entry_addr, &code_entry);
-      jit_register_code (gdbarch, entry_addr, &code_entry);
-      break;
+      {
+	jit_code_entry code_entry;
+	jit_read_code_entry (gdbarch, entry_addr, &code_entry);
+	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 ();
+      {
+	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 ();
+
+	break;
+      }
 
-      break;
     default:
       error (_("Unknown action_flag value in JIT descriptor!"));
       break;
-- 
2.17.1

[PATCH v3 9/9] gdb/jit: skip jit symbol lookup if already detected the symbols don't exist

[Tankut Baris Aktemur]

From: Simon Marchi <simon.marchi@polymtl.ca>

To detect whether an objfile is a JITer, we lookup JIT interface
symbols in the objfile.  If an objfile does not have these symbols, we
conclude that it is not a JITer.  An objfile that does not have the
symbols will never have them.  Therefore, once we do a lookup and find
out that the objfile does not have JIT symbols, just set a flag so
that we can skip symbol lookup for that objfile the next time we reset
JIT breakpoints.

gdb/ChangeLog:
2020-07-14  Simon Marchi  <simon.marchi@polymtl.ca>
	    Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* objfiles.h (struct objfile) <skip_jit_symbol_lookup>: New field.
	* jit.c (jit_breakpoint_re_set_internal): Use the
	`skip_jit_symbol_lookup` field.
---
 gdb/jit.c      | 15 +++++++++++++--
 gdb/objfiles.h |  6 ++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 1850e5b143e..63d21d28bc0 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -891,19 +891,30 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch, program_space *pspace)
 {
   for (objfile *the_objfile : pspace->objfiles ())
     {
+      if (the_objfile->skip_jit_symbol_lookup)
+	continue;
+
       /* Lookup the registration symbol.  If it is missing, then we
 	 assume we are not attached to a JIT.  */
       bound_minimal_symbol reg_symbol
 	= lookup_minimal_symbol (jit_break_name, nullptr, the_objfile);
       if (reg_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (reg_symbol) == 0)
-	continue;
+	{
+	  /* No need to repeat the lookup the next time.  */
+	  the_objfile->skip_jit_symbol_lookup = true;
+	  continue;
+	}
 
       bound_minimal_symbol desc_symbol
 	= lookup_minimal_symbol (jit_descriptor_name, NULL, the_objfile);
       if (desc_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
-	continue;
+	{
+	  /* No need to repeat the lookup the next time.  */
+	  the_objfile->skip_jit_symbol_lookup = true;
+	  continue;
+	}
 
       jiter_objfile_data *objf_data
 	= get_jiter_objfile_data (reg_symbol.objfile);
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index e5828618dfe..371f6b6cb91 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -741,6 +741,12 @@ struct objfile
   /* JIT-related data for this objfile, if the objfile is JITed;
      that is, it was produced by a JITer.  */
   std::unique_ptr<jited_objfile_data> jited_data = nullptr;
+
+  /* A flag that is set to true if the JIT interface symbols are not
+     found in this objfile, so that we can skip the symbol lookup the
+     next time.  If an objfile does not have the symbols, it will
+     never have them.  */
+  bool skip_jit_symbol_lookup = false;
 };
 
 /* A deleter for objfile.  */
-- 
2.17.1

Re: [PATCH v3 0/9] Handling multiple JITers

[Simon Marchi]

On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> Hi,
> 
> This is v3 of the series originally submitted at
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-May/168959.html
> 
> The v2 is at
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-June/169539.html
> 
> Upon that revision, Simon Marchi had proposed
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-June/169758.html
> 
> The updates in this revision v3 are as follows:
> 
> 1. The patches mentioned by Simon in the link above have been cleaned up.
> 
> 2. The 'jiter_data' and 'jited_data' fields have been added directly
>    to the objfile struct as per Pedro's suggestion at
>    https://sourceware.org/pipermail/gdb-patches/2020-June/169796.html
> 
> 3. A final commit addresses Simon's comment of skipping JIT symbol
>    lookup in an objfile if a lookup was already done and the symbols
>    were not found.
> 
> Regards.
> Baris
> 
> Simon Marchi (7):
>   gdb/jit: link to jit_objfile_data directly from the objfile struct
>   gdb/jit: split jit_objfile_data in two
>   gdb/jit: apply some simplifications and assertions
>   gdb/jit: move cached_code_address and jit_breakpoint to
>     jiter_objfile_data
>   gdb/jit: remove jiter_objfile_data -> objfile back-link
>   gdb/jit: apply minor cleanup and modernization
>   gdb/jit: skip jit symbol lookup if already detected the symbols don't
>     exist
> 
> Tankut Baris Aktemur (2):
>   gdb/jit: pass the jiter objfile as an argument to jit_event_handler
>   gdb/jit: enable tracking multiple JITer objfiles
> 
>  gdb/breakpoint.c                             |   3 +-
>  gdb/jit.c                                    | 357 +++++++------------
>  gdb/jit.h                                    |   4 +-
>  gdb/objfiles.h                               |  50 +++
>  gdb/testsuite/gdb.base/jit-reader-simple.exp |  43 ++-
>  5 files changed, 234 insertions(+), 223 deletions(-)

The comments I sent on patches 1-3 are all I had to say, at least for now :)

Simon