Re: [PATCH 2/2] Add symbol-handling callbacks to the jit-reader interface.

[Tom Tromey <tromey at redhat dot com>]

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

Sanjoy> Implements the callbacks in `struct gdbjit_symtab_callbacks'. This
Sanjoy> will allow the JIT debug info reader to be able to parse the debug
Sanjoy> info and actually push the symbols into GDB.

Sanjoy> +#include <dlfcn.h>

I assume this isn't portable.

You could either lift the needed code from GCC's plugin interface, or I
suppose use libtool's libltdl.

Sanjoy> +/* The environment variable we poke at to get the file name of the shared object
Sanjoy> +   to load. */
Sanjoy> +
Sanjoy> +static const char *jit_dbg_reader_so_name_env = "JIT_DEBUG_READER";

I don't think environment variables are a good way to go.

Either it should be automatic in a trusted-library scenario, or it
should require a command from the user.

Sanjoy> +static const char *jit_init_reader_sym = "gdbjit_init_reader";

Instead of a separate symbol for each function, how about just a single
function that we call that returns a struct full of function pointers?

Sanjoy> +struct jit_dbg_reader
Sanjoy> +{
Sanjoy> +  jit_init_reader_fn *init;
Sanjoy> +  jit_read_debug_info_fn *read;
Sanjoy> +  jit_unwind_frame_fn *unwind;
Sanjoy> +  jit_get_frame_id_fn *get_frame_id;
Sanjoy> +  jit_destroy_reader_fn *destroy;
Sanjoy> +
Sanjoy> +  void *handle, *private_data;
Sanjoy> +};

A struct and all its fields should have comments.

Sanjoy> +  handle = dlopen (file_name, RTLD_NOW);
Sanjoy> +  if (!handle)
Sanjoy> +    goto cleanup;

I think there is a GNU requirement that plugins should export a symbol
like "plugin_is_GPL_compatible".  See GCC.

Sanjoy> +static struct gdbjit_symtab *
Sanjoy> +jit_symtab_open_impl (struct gdbjit_symtab_callbacks *cb, const char *file_name)
Sanjoy> +{
Sanjoy> +  struct gdbjit_symtab *ret = XZALLOC (struct gdbjit_symtab);

Does anything ever free this?  I couldn't find it.

Sanjoy> +/* Returns true if the block corrensponding to old should be placed before the

Typo, "corresponding".

Sanjoy> +   block corrensponding to new in the final blockvector. */

Likewise.

Sanjoy> +static struct gdbjit_block *
Sanjoy> +jit_new_block_impl (struct gdbjit_symtab_callbacks *cb,
Sanjoy> +                    struct gdbjit_symtab *symtab,
Sanjoy> +                    struct gdbjit_block *parent,
Sanjoy> +                    CORE_ADDR begin, CORE_ADDR end, const char *name)
Sanjoy> +{
Sanjoy> +  struct gdbjit_block *block = XZALLOC (struct gdbjit_block);
Sanjoy> +
Sanjoy> +  (void) cb;

Don't do that.

Sanjoy> +  if (stab->linetable) {

Brace placement.

Sanjoy> +  if (target_read_memory (target_mem, gdb_mem, sz))

This has a QUIT in it, so it can presumably throw an exception if the
user C-c's at exactly the wrong moment.

So I think this needs some kind of TRY_CATCH wrapper.  There's another
case of this as well.


I found it quite hard to read this patch for some reason; maybe I'm just
having a bad day.  Anyway I feel certain I have missed various important
things.

Tom