Re: [RFA] Add interface for registering JITed code

Tom Tromey
2009-07-24T20:42:25Z

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