Re: [RFA] Add interface for registering JITed code

[Reid Kleckner <rnk at mit dot edu>]

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

I'll figure it out on Monday, but for now, here's some code changes.

> 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

I have absolutely no idea.  :)

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

Yes, on the LLVM side I actually put a big fat global lock around the
whole thing.  I don't know if it's possible for there to be multiple
symbols when there are dynamic libraries or who knows what, but only
one thread should be touching those globals at a time.  If you had
multiple JITs that were unaware of each other in the binary trying to
JIT, then it would break down, but that seems like an uncommon use
case.  If it comes to that, someone could write a library that they
both link in which handles the synchronization.

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

I put it in breakpoint_re_set and follow_exec after the solib stuff,
so that should probably work, but I have no way of testing it.

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

I think if someone has that use case, then they can write a client
side library that wraps the __jit symbols and make the two JITs call
that.

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

Oh right, I wrote that code, and then was worried that the
objfile_data wouldn't be initialized to NULL.  It's returned from
calloc, which according to Jeff is initialized to 0's, so the code I
wrote works.

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

That's true, but they should probably stay closely coupled.  I started
off just copying over the whole struct with one target_read_mem call,
and having them magically line up.  However, that doesn't work if
you're debugging a 32-bit process with a 64-bit debugger.

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

Actually, it is, because entry_addr is a CORE_ADDR.  CORE_ADDR fits
into a void* right?  I'm just doing it to avoid mallocing a single
CORE_ADDR.

> 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 *)".

Done.

> 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...).

This code has been reverted, and I use bfd_openr_iovec now.

Reid

Attachment: jit-patch.txt
Description: Text document