[1/3] gdb/jit: use a map to store objfile and jit breakpoint info

Message ID 618117c0c731e4511927563ed376a6ad84ed138c.1590397723.git.tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • Handling multiple JITers
Related show

Commit Message

Simon Marchi via Gdb-patches May 25, 2020, 9:38 a.m.
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 jitter 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 JITer
would be missed.  This patch replaces the single objfile pointer with
a map so that multiple objfiles can be tracked per program space.

This is a preparatory patch, where we still use a single objfile, but
just access it via the map.  The subsequent patch utilizes multiple
objfiles.

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

	* jit.c: Include <map>.
	(struct jit_objfile_bp): New struct.
	(struct jit_program_space_data): Enable storing multiple objfiles.
	(jit_read_descriptor): Take an objfile*	instead of
	jit_program_space_data* as the last argument.
	(jit_breakpoint_deleted): Update to use the new jit_read_descriptor
	signature and the jit_program_space_data struct.
	(jit_breakpoint_re_set_internal): Ditto.
	(jit_inferior_init): Ditto.
	(jit_event_handler): Ditto.
	(free_objfile_data): Ditto.
---
 gdb/jit.c | 122 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 75 insertions(+), 47 deletions(-)

-- 
2.17.1

Comments

Simon Marchi June 14, 2020, 5:50 p.m. | #1
On 2020-05-25 5:38 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> diff --git a/gdb/jit.c b/gdb/jit.c

> index 1b5ef46469e..fdb1248ed5b 100644

> --- a/gdb/jit.c

> +++ b/gdb/jit.c

> @@ -42,6 +42,7 @@

>  #include "readline/tilde.h"

>  #include "completer.h"

>  #include <forward_list>

> +#include <map>

>  

>  static std::string jit_reader_dir;

>  

> @@ -241,17 +242,11 @@ 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.  */

> +/* Per-objfile structure recording JIT breakpoints.  */


Just to help disambiguate, maybe precise here that we are talking about
the JIT-providing objfiles (those that define the magic interface
symbols), not the objfiles that are the result of the JIT.

>  

> -struct jit_program_space_data

> +struct jit_objfile_bp

>  {

> -  /* The objfile.  This is NULL if no objfile holds the JIT

> -     symbols.  */

> -

> -  struct objfile *objfile = 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 +255,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 informations associated to objfiles.  */

> +

> +  std::map<objfile *, jit_objfile_bp> objfile_and_bps;


If we don't care about key ordering, I'd use an std::unordered_map.

But really, if we expect just to have a handful of items, it would probably
be more efficient to have a list or vector.

Also, given my comment on the following patch, I think we'll have to do
lookups by breakpoint address, so we would have to iterate on the maps items
anyway.  Unless we use the breakpoint address as the key.

>  };

>  

>  static program_space_key<jit_program_space_data> jit_program_space_key;

> @@ -332,9 +337,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 *objf)

>  {

>    int err;

>    struct type *ptr_type;

> @@ -344,17 +349,17 @@ 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)

> +  if (objf == nullptr)

>      return 0;


I would probably change jit_read_descriptor to require a non-NULL objfile.

Simon
Simon Marchi via Gdb-patches June 16, 2020, 9:47 a.m. | #2
On Sunday, June 14, 2020 7:50 PM, Simon Marchi wrote:
> On 2020-05-25 5:38 a.m., Tankut Baris Aktemur via Gdb-patches wrote:

> > diff --git a/gdb/jit.c b/gdb/jit.c

> > index 1b5ef46469e..fdb1248ed5b 100644

> > --- a/gdb/jit.c

> > +++ b/gdb/jit.c

> > @@ -42,6 +42,7 @@

> >  #include "readline/tilde.h"

> >  #include "completer.h"

> >  #include <forward_list>

> > +#include <map>

> >

> >  static std::string jit_reader_dir;

> >

> > @@ -241,17 +242,11 @@ 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.  */

> > +/* Per-objfile structure recording JIT breakpoints.  */

> 

> Just to help disambiguate, maybe precise here that we are talking about

> the JIT-providing objfiles (those that define the magic interface

> symbols), not the objfiles that are the result of the JIT.


Updated the comment in v2 as

  /* Structure recording JIT breakpoints per JITer objfile.  */

> >

> > -struct jit_program_space_data

> > +struct jit_objfile_bp

> >  {

> > -  /* The objfile.  This is NULL if no objfile holds the JIT

> > -     symbols.  */

> > -

> > -  struct objfile *objfile = 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 +255,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 informations associated to objfiles.  */

> > +

> > +  std::map<objfile *, jit_objfile_bp> objfile_and_bps;

> 

> If we don't care about key ordering, I'd use an std::unordered_map.

> 

> But really, if we expect just to have a handful of items, it would probably

> be more efficient to have a list or vector.

> 

> Also, given my comment on the following patch, I think we'll have to do

> lookups by breakpoint address, so we would have to iterate on the maps items

> anyway.  Unless we use the breakpoint address as the key.


In several places we need to simply iterate over all.  In couple places we need to
lookup by the objfile, and in one place by the breakpoint.  In practice, I would
expect the number of JITer objfiles to be fewer than a handful.  Given all these,
I updated the patch to use a list.

> >  };

> >

> >  static program_space_key<jit_program_space_data> jit_program_space_key;

> > @@ -332,9 +337,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 *objf)

> >  {

> >    int err;

> >    struct type *ptr_type;

> > @@ -344,17 +349,17 @@ 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)

> > +  if (objf == nullptr)

> >      return 0;

> 

> I would probably change jit_read_descriptor to require a non-NULL objfile.

> 

> Simon


In v2, this spot now includes a gdb_assert.  There are two places that call this function.
In both, I believe it's guaranteed that the argument is non-null.

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

diff --git a/gdb/jit.c b/gdb/jit.c
index 1b5ef46469e..fdb1248ed5b 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -42,6 +42,7 @@ 
 #include "readline/tilde.h"
 #include "completer.h"
 #include <forward_list>
+#include <map>
 
 static std::string jit_reader_dir;
 
@@ -241,17 +242,11 @@  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.  */
+/* Per-objfile structure recording JIT breakpoints.  */
 
-struct jit_program_space_data
+struct jit_objfile_bp
 {
-  /* The objfile.  This is NULL if no objfile holds the JIT
-     symbols.  */
-
-  struct objfile *objfile = 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 +255,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 informations associated to objfiles.  */
+
+  std::map<objfile *, jit_objfile_bp> objfile_and_bps;
 };
 
 static program_space_key<jit_program_space_data> jit_program_space_key;
@@ -332,9 +337,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 *objf)
 {
   int err;
   struct type *ptr_type;
@@ -344,17 +349,17 @@  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)
+  if (objf == nullptr)
     return 0;
-  objf_data = get_jit_objfile_data (ps_data->objfile);
+  objf_data = get_jit_objfile_data (objf);
   if (objf_data->descriptor == NULL)
     return 0;
 
+  CORE_ADDR addr = MSYMBOL_VALUE_ADDRESS (objf, objf_data->descriptor);
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
 			"jit_read_descriptor, descriptor_addr = %s\n",
-			paddress (gdbarch, MSYMBOL_VALUE_ADDRESS (ps_data->objfile,
-								  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;
@@ -363,9 +368,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,
-						   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 "
@@ -941,11 +944,13 @@  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 &objf_and_bp : ps_data->objfile_and_bps)
+	  if (objf_and_bp.second.jit_breakpoint == iter->owner)
+	    {
+	      objf_and_bp.second.cached_code_address = 0;
+	      objf_and_bp.second.jit_breakpoint = nullptr;
+	    }
     }
 }
 
@@ -961,7 +966,11 @@  jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
   struct jit_objfile_data *objf_data;
   CORE_ADDR addr;
 
-  if (ps_data->objfile == NULL)
+  objfile *the_objfile = nullptr;
+  if (!ps_data->objfile_and_bps.empty ())
+    the_objfile = ps_data->objfile_and_bps.begin ()->first;
+
+  if (the_objfile == nullptr)
     {
       /* Lookup the registration symbol.  If it is missing, then we
 	 assume we are not attached to a JIT.  */
@@ -980,12 +989,12 @@  jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
       objf_data->register_code = reg_symbol.minsym;
       objf_data->descriptor = desc_symbol.minsym;
 
-      ps_data->objfile = reg_symbol.objfile;
+      the_objfile = reg_symbol.objfile;
     }
   else
-    objf_data = get_jit_objfile_data (ps_data->objfile);
+    objf_data = get_jit_objfile_data (the_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,
@@ -993,16 +1002,20 @@  jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 			"breakpoint_addr = %s\n",
 			paddress (gdbarch, addr));
 
-  if (ps_data->cached_code_address == addr)
+  /* Note that this will insert a fresh value if THE_OBJFILE does not
+     exist as a key.  */
+  jit_objfile_bp &info = ps_data->objfile_and_bps[the_objfile];
+
+  if (info.cached_code_address == addr)
     return 0;
 
   /* Delete the old breakpoint.  */
-  if (ps_data->jit_breakpoint != NULL)
-    delete_breakpoint (ps_data->jit_breakpoint);
+  if (info.jit_breakpoint != nullptr)
+    delete_breakpoint (info.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);
+  info.cached_code_address = addr;
+  info.jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
 
   return 0;
 }
@@ -1255,9 +1268,14 @@  jit_inferior_init (struct gdbarch *gdbarch)
   if (jit_breakpoint_re_set_internal (gdbarch, ps_data) != 0)
     return;
 
+  /* Fetch the saved objfile.  */
+  objfile *objf = nullptr;
+  if (!ps_data->objfile_and_bps.empty ())
+    objf = (ps_data->objfile_and_bps.begin ())->first;
+
   /* 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, objf))
     return;
 
   /* Check that the version number agrees with that we support.  */
@@ -1269,16 +1287,17 @@  jit_inferior_init (struct gdbarch *gdbarch)
       return;
     }
 
-  /* If we've attached to a running program, we need to check the descriptor
-     to register any functions that were already generated.  */
+  /* 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.  */
+      /* 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;
 
@@ -1335,11 +1354,16 @@  jit_event_handler (struct gdbarch *gdbarch)
   struct jit_descriptor descriptor;
   struct jit_code_entry code_entry;
   CORE_ADDR entry_addr;
-  struct objfile *objf;
+
+  jit_program_space_data *ps_data = get_jit_program_space_data ();
+
+  /* Fetch the saved objfile.  */
+  objfile *objf = nullptr;
+  if (!ps_data->objfile_and_bps.empty ())
+    objf = (ps_data->objfile_and_bps.begin ())->first;
 
   /* Read the descriptor from remote memory.  */
-  if (!jit_read_descriptor (gdbarch, &descriptor,
-			    get_jit_program_space_data ()))
+  if (!jit_read_descriptor (gdbarch, &descriptor, objf))
     return;
   entry_addr = descriptor.relevant_entry;
 
@@ -1380,12 +1404,16 @@  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;
+	  auto search = ps_data->objfile_and_bps.find (objfile);
+	  if (search != ps_data->objfile_and_bps.end ())
+	    {
+	      if (search->second.jit_breakpoint != nullptr)
+		delete_breakpoint (search->second.jit_breakpoint);
+
+	      ps_data->objfile_and_bps.erase (search);
+	    }
 	}
     }