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

[Simon Marchi <simark@simark.ca>]

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