This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Add interface for registering JITed code


Here's an updated patch.  I still need to write the manual
documentation and switch to the iovec, though.  I wrote this message
and the patch yesterday before Ulrich's message arrived.

On Thu, Jul 23, 2009 at 10:42 AM, Tom Tromey<tromey@redhat.com> wrote:
>>>>>> "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.

Glad to hear I'm on the right track.

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

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

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

Done.

> 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

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

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

How would this work?  Would they both have separate functions
__jit_debug_register at different addresses?  One way to solve both of
these problems might be to create a single libgdbjitagent library
similar to oprofile's agent library.  That way we have some control
over both the inferior side and the GDB side.

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

Ah, that's much better.  Unfortunately because the data for other
objfiles is uninitialized (it's returned from calloc), there's no way
for me to check which objfiles contain JITed code.  Do I actually need
to have this inferior_exit hook?  I just noticed that when I kill and
restart the inferior it doesn't seem to free these objfiles, so I
added the hook.

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

Done.

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

I'll use target_gdbarch until anyone else says otherwise.

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

Done.

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

Switched to xfree.

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

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

> Reid> + ?if (reg_addr == (CORE_ADDR)NULL)
> Reid> + ? ?return;
>
> Just write `0' instead of `(CORE_ADDR)NULL'. ?There are 2 instances of
> this.

Done.

> Reid> + ?if (err) return;
>
> Newline before the return. ?There are a few of these.

Done.

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

Done.  I made some functions just return void instead of error codes
and also call error.

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

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

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

I talked to Doug Evans about it, and he said he'd look into getting a
patch into BFD.  The basic problem is that BFD doesn't take ownership
of any memory buffers that you give it.  If you look in bfd_close, it
does a check for flags & BFD_IN_MEMORY and calls iovec->bclose for
BFD's that aren't in memory.  It doesn't do anything with iostream
(bim) or bim->buffer.  Other BFD clients may rely on this or also have
workarounds, so that will need to be sorted out before we can just go
ahead and free it on the BFD side.

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

Done.

Reid

Attachment: jit-patch.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]