[RFC] JIT Debug info reader
Yao Qi
yao@codesourcery.com
Fri Jul 1 13:51:00 GMT 2011
On 06/30/2011 11:52 PM, Sanjoy Das wrote:
> Hi!
>
> I've been working on easing JIT interaction with GDB. The current
> situation of generating ELF files in memory is usually too much work for
> existing JIT compilers; especially since they don't otherwise contain
> the ELF / DWARF manipulation routines (as opposed to a more traditional
> compiler).
>
> One way to solve this (which is what the attached patches try to do) is
> to have the JIT vendor themselves decide on a "format" for the debug
> info. Then the JIT compiler can register the debug info objects by
> calling `__jit_debug_register_code', and also provide a shared object
> that can be loaded and used to parse the emitted debug info.
I agree with you on this point.
>
> With the current patch, the JIT compiler writers can create an SO which
> implements the interface defined in `jit-reader.h', and call GDB with
> `JIT_DEBUG_READER' set to the SO to load as the debug info reader.
>
> The current version only allows the JIT debug info reader to parse out
> some minimal amount of information, which will then allow GDB to display
> a meaningful stack trace. The idea is that in case the user wants to use
> GDB for full fledged debugging of generated code, it is probably better
> to bite the bullet and generate DWARF.
>
> I've done some preliminary testing by writing a debug info emitter and
> reader for a Javascript VM; and things seem to work. I have attached the
> patches for general comments and review.
>
I'd like to know how do you test this piece of code. Usually, code in
gdb is tested by dejagnu test cases. I don't know how much work to do
if we want to test your three patches under current test framework
dejagnu. We may need a debug info generator and reader from somewhere
during gdb tests.
Thanks for your patch. Usually, when some big patches are submitted for
review, they are split into pieces (as you did), and send each of patch
in a separate mail, to describe what this patch does, so that reviewer
is clear and easy to review. You can find many big changes are reviewed
in this way, for example,
[0/2] more OO, Ada exception catchpoints: intro
[1/2] more OO, Ada exception catchpoints: move catch exception/assert
commands to ada-lang.c.
[2/2] more OO, Ada exception catchpoints: user specified conditions
Of course, ChangeLog is needed for each patch.
Since I didn't see the debug info emitter and reader, I don't have much
comments on your patches, except some general ones.
>
> 0001-Adds-a-jit-reader-interface.patch
>
> diff --git a/gdb/jit-reader.h b/gdb/jit-reader.h
> new file mode 100644
> index 0000000..0380234
> --- /dev/null
> +++ b/gdb/jit-reader.h
> @@ -0,0 +1,168 @@
> +/* Defines the interface for JIT debug-info readers. This file needs to be
> + self-sufficient since it will be included in the JIT debug-info readers
> + directly.
> +
> + This is a minimum-functionality version - the JIT reader can only read in
> + enough debug information to allow GDB to display a meaningful stack trace. */
Please add copyright header here.
> +
> +#ifndef GDB_JIT_READER_H
> +#define GDB_JIT_READER_H
> +
> +/* Versioning: for the SO to be correctly recognized and loaded, put the macro
> + GDB_JIT_DECLARE_API_VERSION in a source file. */
> +
> +#define GDB_JIT_INTERFACE_VERSION 1
> +#define GDB_JIT_DECLARE_API_VERSION \
> + extern int __gdbjit_so_api_version (void) \
> + { \
> + return GDB_JIT_INTERFACE_VERSION; \
> + }
> +
> +#define GDB_JIT_SUCCESS 1
> +#define GDB_JIT_FAIL 0
> +#define GDBJIT_MAX_REGISTER_SIZE 64 /* Mirrors the internal GDB definition. */
> +
> +
> +/* Denotes a register value. */
> +struct gdbjit_reg_value {
> + /* Set to non-zero if this register has a valid value. Otherwise 0. */
> + int defined;
> + /* The actual value of the register (this is considered valid only if defined
> + is non-zero). */
> + unsigned char value[GDBJIT_MAX_REGISTER_SIZE];
> +};
> +
> +/* Unique frame identifier. This should remain constant throughout the lifetime
> + of the frame concerned. */
> +struct gdbjit_frame_id {
> + void *code_address;
> + void *stack_address;
> +};
> +
Why don't you use "CORE_ADDR" to replace "void *"?
> 0002-Symbol-handling-code.patch
>
> +
> +struct jit_dbg_reader {
Please put "{" in the next line.
> + jit_init_reader_fn *init;
> + jit_read_debug_info_fn *read;
> + jit_unwind_frame_fn *unwind;
> + jit_get_frame_id_fn *get_frame_id;
> + jit_destroy_reader_fn *destroy;
> +
> + void *handle, *private_data;
> +};
>
> +struct gdbjit_block
> +{
> + struct gdbjit_block *next, *parent;
> + struct block *real_block;
> +
> + void *begin, *end;
> + const char *name;
> +};
> +
Could you give some comments on "begin", "end" and "name"?
> +
> +/* Returns true if the block corrensponding to old should be placed before the
> + block corrensponding to new in the final blockvector. */
"old" and "new" in your comment should be captalized.
> +static int
> +compare_block (struct gdbjit_block *old, struct gdbjit_block *new)
> +{
> + if (old == NULL)
> + return 1;
> + if (old->begin < new->begin)
> + return 1;
> + else if (old->begin == new->begin)
> + {
> + if (old->end > new->end)
> + return 1;
> + else
> + return 0;
> + }
> + else
> + return 0;
> +}
> +
> +static struct gdbjit_block *
> +jit_new_block_impl (struct gdbjit_symtab_callbacks *cb,
> + struct gdbjit_symtab *symtab,
> + struct gdbjit_block *parent,
> + void *begin, void *end, const char *name)
> +{
> + struct gdbjit_block *block = XZALLOC (struct gdbjit_block);
> +
> + (void) cb;
> +
> + block->next = symtab->blocks;
> + block->begin = begin;
> + block->end = end;
> + block->name = name ? xstrdup (name) : NULL;
> + block->parent = parent;
> +
> + /* Ensure that the blocks are inserted in the correct (reverse of the order
> + expected by blockvector). */
> + if (compare_block (symtab->blocks, block))
> + {
> + symtab->blocks = block;
> + }
> + else
> + {
> + struct gdbjit_block *i = symtab->blocks;
a blank line is needed between variable declaraion and code body.
> + for (;; i = i->next)
> + {
> + /* Guranteed to terminate, since compare_block (NULL, _) returns 1 */
> + if (compare_block (i->next, block))
> + {
> + block->next = i->next;
> + i->next = block;
> + break;
> + }
> + }
> + }
> + symtab->nblocks++;
> +
> + return block;
> +}
> +
> +
> +static void
> +jit_symtab_close_impl (struct gdbjit_symtab_callbacks *cb,
> + struct gdbjit_symtab *stab)
> +{
> + struct jit_dbg_reader_data *dat = cb->private;
> + struct symtab *symtab;
> + struct gdbjit_block *blck_iter, *blck_iter_tmp;
> + struct block *block_iter;
> + struct objfile *objfile;
> + int actual_nblocks = FIRST_LOCAL_BLOCK + stab->nblocks, i;
> + CORE_ADDR begin, end;
> +
> + if (*dat->objf == NULL)
> + {
> + objfile = allocate_objfile (NULL, 0);
> + *dat->objf = objfile;
> + objfile->gdbarch = target_gdbarch;
> + objfile->msymbols = obstack_alloc (&objfile->objfile_obstack,
> + sizeof (struct minimal_symbol));
Looks indent is not right.
> + objfile->msymbols[0].ginfo.name = NULL;
> + objfile->msymbols[0].ginfo.value.address = 0;
> + objfile->name = xstrdup ("JIT");
> + }
> + else
> + objfile = *dat->objf;
> +
> + symtab = allocate_symtab (stab->file_name, objfile);
> + symtab->dirname = NULL; /* JIT compilers compile in memory */
> +
> + if (stab->linetable) {
> + int size = (stab->linetable->nitems - 1) * sizeof (struct linetable_entry) +
> + sizeof (struct linetable);
> + LINETABLE (symtab) = obstack_alloc (&objfile->objfile_obstack, size);
> + memcpy (LINETABLE (symtab), stab->linetable, size);
> + } else {
> + LINETABLE (symtab) = NULL;
> + }
> +
> + symtab->blockvector = obstack_alloc (&objfile->objfile_obstack,
> + (sizeof (struct blockvector)
> + + (actual_nblocks - 1) *
> + sizeof (struct block *)));
> +
> + /* (begin, end) will contain the PC range this entire blockvector spans. */
> + symtab->primary = 1;
> + BLOCKVECTOR_MAP (symtab->blockvector) = NULL;
> + begin = (CORE_ADDR) stab->blocks->begin;
> + end = (CORE_ADDR) stab->blocks->end;
> + BLOCKVECTOR_NBLOCKS (symtab->blockvector) = actual_nblocks;
> +
> + for (i = (actual_nblocks - 1), blck_iter = stab->blocks;
> + i >= FIRST_LOCAL_BLOCK; i--)
> + {
> + struct block *bl = allocate_block (&objfile->objfile_obstack);
> + struct symbol *block_name = obstack_alloc (&objfile->objfile_obstack,
> + sizeof (struct symbol));
> +
> + BLOCK_DICT (bl) = dict_create_linear (&objfile->objfile_obstack, NULL);
> + BLOCK_START (bl) = (CORE_ADDR) blck_iter->begin;
> + BLOCK_END (bl) = (CORE_ADDR) blck_iter->end;
> + BLOCK_FUNCTION (bl) = block_name;
> +
> + memset (block_name, 0, sizeof (struct symbol));
> + SYMBOL_DOMAIN (block_name) = VAR_DOMAIN;
> + SYMBOL_CLASS (block_name) = LOC_BLOCK;
> + block_name->symtab = symtab;
> + SYMBOL_BLOCK_VALUE (block_name) = bl;
> +
> + block_name->ginfo.name = obstack_alloc (&objfile->objfile_obstack,
> + 1 + strlen (blck_iter->name));
> + strcpy (block_name->ginfo.name, blck_iter->name);
> + BLOCK_FUNCTION (bl) = block_name;
> +
> + BLOCKVECTOR_BLOCK (symtab->blockvector, i) = bl;
> + if (begin < (long) blck_iter->begin)
> + begin = (long) blck_iter->begin;
> + if (end > (long) blck_iter->end)
> + end = (long) blck_iter->end;
> + }
> +
> + blck_iter = blck_iter->next;
> +
> + /* Now add the special blocks. */
> + block_iter = NULL;
> + for (i = 0; i < FIRST_LOCAL_BLOCK; i++)
> + {
> + struct block *bl = allocate_block (&objfile->objfile_obstack);
> + BLOCK_DICT (bl) = dict_create_linear (&objfile->objfile_obstack, NULL);
> + BLOCK_SUPERBLOCK (bl) = block_iter;
> + block_iter = bl;
> +
> + BLOCK_START (bl) = (CORE_ADDR) begin;
> + BLOCK_END (bl) = (CORE_ADDR) end;
> +
> + BLOCKVECTOR_BLOCK (symtab->blockvector, i) = bl;
> + }
> +
> + /* Fill up the superblock fields. */
> + for (blck_iter = stab->blocks; blck_iter; blck_iter = blck_iter->next)
> + {
> + if (blck_iter->parent != NULL)
> + BLOCK_SUPERBLOCK (blck_iter->real_block) =
> + blck_iter->parent->real_block;
> + }
> +
> + /* Free memory. */
> + blck_iter = stab->blocks;
> + for (blck_iter = stab->blocks, blck_iter_tmp = blck_iter->next;
> + blck_iter; blck_iter = blck_iter_tmp)
> + {
> + xfree (blck_iter);
> + }
> +}
> +
> +
> /* This function registers code associated with a JIT code entry. It uses the
> pointer and size pair in the entry to read the symbol file from the remote
> and then calls symbol_file_add_from_local_memory to add it as though it were
> @@ -246,10 +638,10 @@ jit_register_code (struct gdbarch *gdbarch,
> struct section_addr_info *sai;
> struct bfd_section *sec;
> struct objfile *objfile;
> - struct cleanup *old_cleanups, *my_cleanups;
> - int i;
> + int i, success;
> const struct bfd_arch_info *b;
> CORE_ADDR *entry_addr_ptr;
> + struct jit_inferior_data *inf_data = get_jit_inferior_data ();
>
> if (jit_debug)
> fprintf_unfiltered (gdb_stdlog,
> @@ -258,53 +650,62 @@ jit_register_code (struct gdbarch *gdbarch,
> paddress (gdbarch, code_entry->symfile_addr),
> pulongest (code_entry->symfile_size));
>
> - nbfd = bfd_open_from_target_memory (code_entry->symfile_addr,
> - code_entry->symfile_size, gnutarget);
> - old_cleanups = make_cleanup_bfd_close (nbfd);
> -
> - /* Check the format. NOTE: This initializes important data that GDB uses!
> - We would segfault later without this line. */
> - if (!bfd_check_format (nbfd, bfd_object))
> + if (jit_try_read_symtab (inf_data, code_entry->symfile_addr,
> + code_entry->symfile_size))
> {
> - printf_unfiltered (_("\
> + objfile = inf_data->jit_objfile;
> + }
> + else
> + {
You move original code in "else" branch, so please move local variable
declaration here as well.
>
> 0003-JIT-unwinder.patch
>
> Adds a new unwinder which relays all the unwinding action to the JIT reader loaded in memory.
> ---
> gdb/i386-tdep.c | 4 ++
> gdb/jit.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> gdb/jit.h | 2 +
> 3 files changed, 159 insertions(+), 0 deletions(-)
>
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 366d0fa..66626ab 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -30,6 +30,7 @@
> #include "frame-base.h"
> #include "frame-unwind.h"
> #include "inferior.h"
> +#include "jit.h"
> #include "gdbcmd.h"
> #include "gdbcore.h"
> #include "gdbtypes.h"
> @@ -7330,6 +7331,9 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> CFI info will be used if it is available. */
> dwarf2_append_unwinders (gdbarch);
>
> + /* JIT reader pseudo-unwinder. */
> + jit_prepend_unwinder (gdbarch);
> +
JIT code unwinder is not a feature specific to any port. It should be a
GDB-wide unwinder, like dwarf unwinder. It is better to register it
else where.
> frame_base_set_default (gdbarch, &i386_frame_base);
>
> /* Pseudo registers may be changed by amd64_init_abi. */
> diff --git a/gdb/jit.c b/gdb/jit.c
> index d259e04..1a00a91 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -25,6 +25,7 @@
> #include "breakpoint.h"
> #include "command.h"
> #include "dictionary.h"
> +#include "frame-unwind.h"
> #include "gdbcmd.h"
> #include "gdbcore.h"
> #include "inferior.h"
> @@ -769,6 +770,152 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
> return 0;
> }
>
> +struct jit_unwind_private {
> + struct gdbjit_reg_value *registers;
> + struct frame_info *this_frame;
> +};
> +
> +static void
> +jit_unwind_reg_set_impl (struct gdbjit_unwind_callbacks *cb, int regnum,
> + struct gdbjit_reg_value value)
> +{
> + struct jit_unwind_private *priv = cb->private;
> + int gdb_reg = gdbarch_dwarf2_reg_to_regnum (target_gdbarch, regnum);
> + priv->registers[gdb_reg] = value;
> +}
> +
> +static struct gdbjit_reg_value
> +jit_unwind_reg_get_impl (struct gdbjit_unwind_callbacks *cb, int regnum)
> +{
> + struct jit_unwind_private *priv = cb->private;
> + struct gdbjit_reg_value val;
> + int gdb_reg = gdbarch_dwarf2_reg_to_regnum (target_gdbarch, regnum);
> + val.defined = frame_register_read (priv->this_frame, gdb_reg, val.value);
> + return val;
> +}
> +
> +static int
> +jit_frame_sniffer (const struct frame_unwind *self,
> + struct frame_info *this_frame, void **cache)
> +{
> + /* First look up the PC, and check if the code address has been registered or
> + not. */
> + CORE_ADDR pc = get_frame_pc (this_frame);
> + struct jit_inferior_data *inf_data = get_jit_inferior_data ();
> + struct jit_unwind_private *priv_data;
> + struct jit_dbg_reader *iter;
> + struct gdbjit_unwind_callbacks callbacks =
> + {
> + jit_unwind_reg_get_impl,
> + jit_unwind_reg_set_impl,
> + jit_target_read_impl
> + };
> +
> + if (inf_data->reader == NULL)
> + return 0;
> +
> + /* All the unwinding happens here, and the unwound registers are written to
> + this block of memory, which we then sneakily read back in
> + jit_frame_prev_register. */
> + if (!*cache)
> + {
> + *cache = XZALLOC (struct jit_unwind_private);
> + priv_data = *cache;
> + priv_data->registers = XCALLOC (gdbarch_num_regs (target_gdbarch),
> + struct gdbjit_reg_value);
> + priv_data->this_frame = this_frame;
> + }
> + else
> + {
> + priv_data = *cache;
> + priv_data->this_frame = this_frame;
> + }
> +
> + callbacks.private = priv_data;
> +
> + /* Try to coax the provided unwinder to unwind the stack, and hope it
> + succeeds. */
> + if (inf_data->reader->unwind (inf_data->reader->private_data, &callbacks)
> + == GDB_JIT_SUCCESS)
> + return 1;
> + xfree (priv_data->registers);
> + xfree (priv_data);
> + *cache = NULL;
> +
> + return 0;
> +}
> +
> +static enum unwind_stop_reason
> +jit_frame_unwind_stop_reason (struct frame_info *this_frame, void **cache)
> +{
> + /* TODO: This needs to be fixed. */
> + return UNWIND_NO_REASON;
> +}
> +
> +static void
> +jit_frame_this_id (struct frame_info *this_frame, void **cache,
> + struct frame_id *this_id)
> +{
> + struct jit_inferior_data *inf_data = get_jit_inferior_data ();
> + struct jit_unwind_private private =
> + {
> + NULL,
> + this_frame
> + };
> + struct gdbjit_frame_id frame_id;
> + struct gdbjit_unwind_callbacks callbacks =
> + {
> + jit_unwind_reg_get_impl,
> + NULL,
> + jit_target_read_impl,
> +
> + &private
> + };
> +
> + frame_id = inf_data->reader->get_frame_id (inf_data->reader->private_data,
> + &callbacks);
> + this_id->stack_addr = (CORE_ADDR) frame_id.stack_address;
> + this_id->code_addr = (CORE_ADDR) frame_id.stack_address;
I guess it should be "frame_id.code_address".
> + this_id->stack_addr_p = this_id->code_addr_p = 1;
AFAIK, we don't have to set stack_addr_p and code_addr_p explicitly.
GDB can do it for us ( but I don't know where it is performed). In each
port's frame_this_id function, I don't see stack_addr_p and code_addr_p
is set there, so I think we don't need it here as well.
> +}
> +
--
Yao (é½å°§)
More information about the Gdb-patches
mailing list