Re: [PATCH 3/7] New commands for loading and unloading a reader.

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Sanjoy" == Sanjoy Das <sanjoy@playingwithpointers.com> writes:

Sanjoy> Introduces two new GDB commands - `load-jit-reader' and
Sanjoy> `unload-jit-reader'.

I am not that fond of these command names.
I'm not sure what else I would propose... maybe a "jit" prefix command,
with "load" and "unload" subcommands?

Do we actually need the ability to unload?

Sanjoy> +#define GDB_READER_DIR (LIBDIR "/gdb/")

This should go through the relocation process done for other
directories.  See main.c.

Sanjoy> +/* One reader that has been loaded successfully, and can potentially be used to
Sanjoy> +   parse debug info. */
Sanjoy> +struct jit_reader
Sanjoy> +{
Sanjoy> +  struct gdb_reader_funcs *functions;
Sanjoy> +} *loaded_jit_reader = NULL;

Can this be static?

Sanjoy> +typedef struct gdb_reader_funcs * (reader_init_fn_type) (void);
Sanjoy> +const char *reader_init_fn_sym = "gdb_init_reader";

Likewise.

Sanjoy> +/* Try to load FILE_NAME as a JIT debug info reader. Set ERROR_STRING
Sanjoy> +   in case of an error (and return NULL), else return a correcly
Sanjoy> +   formed struct jit_reader. */
Sanjoy> +static struct jit_reader *
Sanjoy> +jit_reader_load (const char *file_name, char **error_string)

It is more normal in gdb to simply call error when an error occurs.
Then you don't need to worry about the return value, or passing in the
error_string pointer.

Sanjoy> +  so = gdb_dlopen (file_name);
Sanjoy> +
Sanjoy> +  if (!so)
Sanjoy> +    {
Sanjoy> +      *error_string = _("could not open reader file");
Sanjoy> +      return NULL;
Sanjoy> +    }

It might be nicer if gdb_dlopen called error, so that the different
ports could give better error messages, say via dlerror.

Sanjoy> +  if (so)
Sanjoy> +    gdb_dlclose(so);

With the error approach, you would install a cleanup that calls
gdb_dlclose, then discard_cleanups at the point of normal return.

Sanjoy> +  strncat (so_name, args, PATH_MAX - sizeof(GDB_READER_DIR) - 4);
Sanjoy> +  strcat (so_name, ".so");

It seems to me that you want a directory separator in here.

Sanjoy> +  free (loaded_jit_reader);

xfree.

Sanjoy> @@ -510,10 +610,10 @@ jit_event_handler (struct gdbarch *gdbarch)
Sanjoy>    struct jit_code_entry code_entry;
Sanjoy>    CORE_ADDR entry_addr;
Sanjoy>    struct objfile *objf;
Sanjoy> +  struct jit_inferior_data *inf_data = get_jit_inferior_data ();
 
Sanjoy>    /* Read the descriptor from remote memory.  */
Sanjoy> -  jit_read_descriptor (gdbarch, &descriptor,
Sanjoy> -		       get_jit_inferior_data ()->descriptor_addr);
Sanjoy> +  jit_read_descriptor (gdbarch, &descriptor, inf_data->descriptor_addr);
Sanjoy>    entry_addr = descriptor.relevant_entry;
 
Sanjoy>    /* Do the corresponding action.  */
Sanjoy> @@ -526,15 +626,16 @@ jit_event_handler (struct gdbarch *gdbarch)
Sanjoy>        jit_register_code (gdbarch, entry_addr, &code_entry);
Sanjoy>        break;
Sanjoy>      case JIT_UNREGISTER:
Sanjoy> -      objf = jit_find_objf_with_entry_addr (entry_addr);
Sanjoy> -      if (objf == NULL)
Sanjoy> -	printf_unfiltered (_("Unable to find JITed code "
Sanjoy> -			     "entry at address: %s\n"),
Sanjoy> -			   paddress (gdbarch, entry_addr));
Sanjoy> -      else
Sanjoy> -        jit_unregister_code (objf);
Sanjoy> -
Sanjoy> -      break;
Sanjoy> +      {
Sanjoy> +        objf = jit_find_objf_with_entry_addr (entry_addr);
Sanjoy> +        if (objf == NULL)
Sanjoy> +          printf_unfiltered (_("Unable to find JITed code "
Sanjoy> +                               "entry at address: %s\n"),
Sanjoy> +                             paddress (gdbarch, entry_addr));
Sanjoy> +        else
Sanjoy> +          jit_unregister_code (objf);
Sanjoy> +        break;
Sanjoy> +      }

These hunks seem to be just formatting changes.
Please drop.

Tom