Re: [RFA] Add interface for registering JITed code

[Tom Tromey <tromey@redhat.com>]

>>>>> "Reid" == Reid Kleckner <rnk@mit.edu> writes:

Reid> So that means we need LLVM to generate dwarf debug info, and we
Reid> need to register it with GDB.

Nice.

Your overall approach seems good to me.

[...]

Reid> If LLVM frees machine code, then it sets the action enum in the
Reid> descriptor to JIT_UNREGISTER, points the descriptor at the relevant
Reid> code entry, and calls __jit_debug_register_code again.

Given that this will be a supported way for GDB to connect to JITs, I
think that the official interface (symbol names, types, enum values,
etc) should be documented in the GDB manual somewhere.

I wonder what happens if the ELF data in the inferior is corrupted.
Perhaps there should be a way to disable the feature.  What do you
think?

This patch also deserves an entry in NEWS.

I have a few comments on the details of the patch itself.

Reid> +struct breakpoint *
Reid> +create_jit_event_breakpoint (CORE_ADDR address)
Reid> +{
Reid> +  struct breakpoint *b;
Reid> +
Reid> +  b = create_internal_breakpoint (address, bp_jit_event);
Reid> +  update_global_location_list_nothrow (1);
Reid> +  return b;
Reid> +}

I am not sure but I suspect this should be called from
breakpoint_re_set -- that is, follow the longjmp model a bit more.

Does the current code work ok if LLVM is a .so linked into the
application?  I suspect, but do not know for sure, that in this
situation gdb will not see the JIT symbols when the inferior-created
hook is run.q

There is also the somewhat more pathological case of a program with
multiple JITs linked in.  I'm on the fence about whether this is really
needed.

Reid> +/* This is a linked list mapping code entry addresses to objfiles.  */
Reid> +static struct jit_obj_link *jit_objfiles = NULL;

You can store arbitrary module-specific data in an objfile using the
objfile_data API.  This would be better because it is an already
supported way of dealing with the lifetime of the associated data.

This would mean getting rid of the linked list here and just looping
over objfiles when needed.

Reid> +      error (_("Unable to read JIT descriptor from remote memory!\n"));

Don't use \n at the end of error text.  (There is code in gdb that does
this but I think the rule is not to.)

Reid> +  descriptor->version = extract_unsigned_integer (&desc_buf[0], 4);

You'll need to update to head ... a few of these functions have an extra
argument now.

I am not sure if using target_gdbarch is ok -- maybe passing the arch to
jit_read_descriptor as an argument would be better.  Maybe Ulrich could
weigh in here.

Reid> +  descriptor->first_entry = extract_typed_address
Reid> +      (&desc_buf[8 + ptr_size], ptr_type);

A formatting nit ... I think it is more typical to split before the '='
than before the '('.

Reid> +static void
Reid> +jit_register_code (CORE_ADDR entry_addr, struct jit_code_entry *code_entry)
[...]
Reid> +      free (buffer);

xfree, but...

Reid> +  objfile = symbol_file_add_from_local_memory (templ, buffer, size);
Reid> +  if (objfile == NULL)
Reid> +    free (buffer);

symbol_file_add_from_local_memory can call error, so you need a cleanup
here rather than explicit calls to free.

Reid> +  if (reg_addr == (CORE_ADDR)NULL)
Reid> +    return;

Just write `0' instead of `(CORE_ADDR)NULL'.  There are 2 instances of
this.

Reid> +  if (err) return;

Newline before the return.  There are a few of these.

Reid> +  /* Check that the version number agrees with that we support.  */
Reid> +  if (descriptor.version != 1)
Reid> +    {
Reid> +      error (_("Unsupported JIT protocol version in descriptor!\n"));
Reid> +      return;

error calls longjmp, so the return is redundant.  There are a couple of
these in the patch.

Reid> +/* When the JIT breakpoint fires, the inferior wants us to take one of these
Reid> +   actions.  */
Reid> +
Reid> +typedef enum
Reid> +{
Reid> +  JIT_NOACTION = 0,
Reid> +  JIT_REGISTER,
Reid> +  JIT_UNREGISTER
Reid> +} jit_actions_t;

Please update the comment here to explain that these values are exported
and used by the inferior, so they cannot be changed.

Reid> +      /* Hack to work around the fact that BFD does not take ownership of the
Reid> +         memory for files allocated in memory.  */

Is it possible to fix this directly in BFD?  Since...

Reid> +        bim = (struct bfd_in_memory *) objfile->obfd->iostream;

... this is definitely fishy :-)

Reid> +struct objfile *
Reid> +symbol_file_add_from_local_memory (struct bfd *templ, bfd_byte *buffer,
Reid> +                                   bfd_size_type size)
Reid> +{
[...]
Reid> +  filename = xstrdup ("<in-memory>");

Because this function can call error, you need a cleanup here.

Tom