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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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