Re: [RFA] Add interface for registering JITed code

[Tom Tromey <tromey@redhat.com>]

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

Reid> Here's an updated patch.

Very quick!

Reid> Where should this go?  It doesn't really fit under any of the
Reid> top-level topics, so far as I can tell.  I only have about a page to
Reid> write about it.

Yeah, I don't really have a suggestion for this.

Today I've also been wondering whether this works with core files.
Have you tried that?  (I can't imagine why it wouldn't work.  And I
don't think it is really a requirement; I'm just curious.)q

Also I was wondering about a multi-threaded JIT.  I suppose it is
sufficient to mention in the documentation that the JIT is responsible
for making sure only a single thread can call __jit_debug_register_code
at a time.

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

Reid> I don't know, since we statically link to LLVM.  I'm not an expert in
Reid> how dynamic loaders work, so I don't know how to fix this.

Ok.  One way would be to stick some code in breakpoint_re_set and
update_breakpoints_after_exec.  (I am not sure if this is the best way or
not.)  Anyway the idea is that after a .so is noticed, re-do the searching,
in case the new .so is a JIT.  So, the work would be done per-objfile,
and not once per inferior.

Maybe this can be done via observers as well; I don't know.

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

Reid> How would this work?  Would they both have separate functions
Reid> __jit_debug_register at different addresses?

Yeah.  This would work if you had two JITs in a process, say loaded
dynamically, and the various __jit symbols had hidden visibility.

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

Reid> Ah, that's much better.  Unfortunately because the data for other
Reid> objfiles is uninitialized (it's returned from calloc), there's no way
Reid> for me to check which objfiles contain JITed code.

I don't think I understand.  I thought you meant that you couldn't
detect whether or not the JIT datum was set on an objfile, but I see
that jit_inferior_exit_hook already does this the way that I would
expect.

Reid> Do I actually need to have this inferior_exit hook?  I just
Reid> noticed that when I kill and restart the inferior it doesn't seem
Reid> to free these objfiles, so I added the hook.

Yeah, I think you need it.

I was thinking maybe this should just be a flag on the objfile, plus a
change to delete such objfiles in reread_symbols.  But I am not sure
which is better, really; and your way is less invasive.

Reid> I took a shot at putting cleanups in, but I'm not sure I did it
Reid> correctly.  In particular, I'm not sure about the discard_cleanups
Reid> logic.

It looks ok to me.

Just FYI, there's a section in the internals manual about cleanups that
is reasonably clear.

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

Reid> Done.  The same is true about the rest of the structs, the ordering of
Reid> the fields cannot be changed without updating the protocol version.

My reading of the code is that it unpacks target memory field-by-field
into the host-side structure.  So, it is ok for this to change.  What
cannot change is the definition of these structs on the target.

Am I misreading this?

These structs, in their target form, should probably all go in the
manual as well.

Reid> +  /* Remember a mapping from entry_addr to objfile.  */
Reid> +  set_objfile_data (objfile, jit_objfile_data, (void*) entry_addr);

I don't think you need the cast here.  There are a few of these.

Reid> +      printf_unfiltered ("Unable to find JITed code entry at address: %p\n",
Reid> +                         (void*) entry_addr);

A style nit, in GNU style the cast is written "(void *)".

Reid> +  nbfd = bfd_open_from_memory (templ, buffer, size, filename);
Reid> +  if (nbfd == NULL)
Reid> +    error (_("Unable to create BFD from local memory: %s"),
Reid> +           bfd_errmsg (bfd_get_error ()));
Reid> +
Reid> +  /* We set loadbase to 0 and assume that the symbol file we're loading has the
Reid> +     absolute addresses set in the ELF.  */
Reid> +  loadbase = 0;
Reid> +  objfile = symbol_file_add_from_memory_common (nbfd, loadbase, 0);

I suspect this needs a cleanup to free the BFD in case
symbol_file_add_from_memory_common errors.  But I couldn't tell
immediately if that is possible (in gdb I tend to assume that anything
can throw...).

Tom