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

Tom Tromey
2011-08-12T20:58:39Z

>>>>> "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