Re: [PATCH 5/7] Use the loaded reader.

[Abhijit Halder <abhijit.k.halder@gmail.com>]

On Sat, Aug 27, 2011 at 7:17 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Sat, Aug 27, 2011 at 6:42 PM, Sanjoy Das
> <sanjoy@playingwithpointers.com> wrote:
>> Invoke the loaded JIT debug info reader to parse the registered symbol
>> files.
>>
>> gdb/ChangeLog:
>>        * gdb/jit.c: Include block.h, dictionary.h and frame-unwind.h.
>>        (add_objfile_entry, jit_target_read_impl, jit_object_open_impl)
>>        (jit_symtab_open_impl, compare_block, jit_block_open_impl)
>>        (jit_symtab_line_mapping_add_impl, jit_symtab_close_impl)
>>        (finalize_symtab, jit_object_close_impl)
>>        (jit_reader_try_read_symtab, jit_bfd_try_read_symtab)
>>        (free_objfile_data): New functions.
>>        (_initialize_jit): Register jit_objfile_data with a proper cleanup
>>        function.
>> ---
>>  gdb/ChangeLog |   12 ++
>>  gdb/jit.c     |  479 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 468 insertions(+), 23 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 78076ef..7cdde7b 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,5 +1,17 @@
>>  2011-08-27  Sanjoy Das  <sdas@igalia.com>
>>
>> +       * gdb/jit.c: Include block.h, dictionary.h and frame-unwind.h.
>> +       (add_objfile_entry, jit_target_read_impl, jit_object_open_impl)
>> +       (jit_symtab_open_impl, compare_block, jit_block_open_impl)
>> +       (jit_symtab_line_mapping_add_impl, jit_symtab_close_impl)
>> +       (finalize_symtab, jit_object_close_impl)
>> +       (jit_reader_try_read_symtab, jit_bfd_try_read_symtab)
>> +       (free_objfile_data): New functions.
>> +       (_initialize_jit): Register jit_objfile_data with a proper cleanup
>> +       function.
>> +
>> +2011-08-27  Sanjoy Das  <sdas@igalia.com>
>> +
>>        * gdb/jit.c: Include gdb-dlfcn.h.
>>        (loaded_jit_reader, reader_init_fn_sym): New static variables.
>>        (jit_reader_load, jit_reader_load_command)
>> diff --git a/gdb/jit.c b/gdb/jit.c
>> index 4fc5819..3be8c4d 100644
>> --- a/gdb/jit.c
>> +++ b/gdb/jit.c
>> @@ -21,8 +21,11 @@
>>
>>  #include "jit.h"
>>  #include "jit-reader.h"
>> +#include "block.h"
>>  #include "breakpoint.h"
>>  #include "command.h"
>> +#include "dictionary.h"
>> +#include "frame-unwind.h"
>>  #include "gdbcmd.h"
>>  #include "gdbcore.h"
>>  #include "inferior.h"
>> @@ -233,6 +236,18 @@ struct jit_inferior_data
>>   CORE_ADDR descriptor_addr;  /* &__jit_debug_descriptor  */
>>  };
>>
>> +/* Remember a mapping from entry_addr to objfile.  */
>> +
>> +static void
>> +add_objfile_entry (struct objfile *objfile, CORE_ADDR entry)
>> +{
>> +  CORE_ADDR *entry_addr_ptr;
>> +
>> +  entry_addr_ptr = xmalloc (sizeof (CORE_ADDR));
>> +  *entry_addr_ptr = entry;
>> +  set_objfile_data (objfile, jit_objfile_data, entry_addr_ptr);
>> +}
>> +
>>  /* Return jit_inferior_data for current inferior.  Allocate if not already
>>    present.  */
>>
>> @@ -329,30 +344,417 @@ jit_read_code_entry (struct gdbarch *gdbarch,
>>       extract_unsigned_integer (&entry_buf[3 * ptr_size], 8, byte_order);
>>  }
>>
>> -/* 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
>> -   a symbol file added by the user.  */
>> +/* Proxy object for building a block. */
>> +
>> +struct gdb_block
>> +{
>> +  /* gdb_blocks are linked into a tree structure. Next points to the
>> +     next node at the same depth as this block and parent to the
>> +     parent gdb_block. */
> Should be two spaces after period. I am not sure but I think inside
> structure there should not be any empty new-line between the comment
> and the definition of member variable. Please correct me if I am
> wrong.
>
>> +
>> +  struct gdb_block *next, *parent;
> Please put a comment here.
>> +  struct block *real_block;
>> +
>> +  /* The first and last code address corresponding to this block */
> Missing trailing period and there should be two spaces after the period.
>
>> +
>> +  CORE_ADDR begin, end;
>> +
>> +  /* The name of this block (if any).  If this is non-NULL, the
>> +     FUNCTION symbol symbol is set to this value. */
>> +
>> +  const char *name;
>> +};
>> +
>> +/* Proxy object for building a symtab. */
>> +
>> +struct gdb_symtab
>> +{
>> +  /* The list of blocks in this symtab. These will eventually be
>> +     converted to real blocks. */
>> +
>> +  struct gdb_block *blocks;
>> +
>> +  /* The number of blocks inserted. */
>> +
>> +  int nblocks;
>> +
>> +  /* A mapping between line numbers to PC. */
>> +
>> +  struct linetable *linetable;
>> +
>> +  /* The source file for this symtab. */
>> +
>> +  const char *file_name;
>> +  struct gdb_symtab *next;
>> +};
>> +
>> +/* Proxy object for building an object. */
>> +
>> +struct gdb_object
>> +{
>> +  struct gdb_symtab *symtabs;
>> +};
>> +
>> +/* The type of the `private' data passed around by the callback
>> +   functions. */
>> +
>> +struct jit_dbg_reader_data
>> +{
>> +  CORE_ADDR entry_address;
>> +};
> Is there any reason not making it a typedef (also the above one)? I
> mean to say something like this:
> typedef CORE_ADDR jit_dbg_reader_data;
> typedef struct gdb_symtab *gdb_object;
> The advantage we may achieve is in saving two level of indirection.
>
>> +
>> +/* The reader calls into this function to read data off the targets
>> +   address space. */
>> +
>> +static enum gdb_status
>> +jit_target_read_impl (GDB_CORE_ADDR target_mem, void *gdb_buf, int len)
>> +{
>> +  int result = target_read_memory ((CORE_ADDR) target_mem, gdb_buf, len);
>> +  if (result == 0)
>> +    return GDB_SUCCESS;
>> +  else
>> +    return GDB_FAIL;
>> +}
>> +
>> +/* The reader calls into this function to create a new gdb_object
>> +   which it can then pass around to the other callbacks. Right now,
>> +   all that is required is allocating the memory. */
>> +
>> +static struct gdb_object *
>> +jit_object_open_impl (struct gdb_symbol_callbacks *cb)
I am not clear on why we are passing struct gdb_symbol_callbacks * as
an argument even when we are not using it inside function.

>> +{
>> +  return XZALLOC (struct gdb_object);
>> +}
>> +
>> +/* Readers call into this function to open a new gdb_symtab, which,
>> +   again, is passed around to other callbacks. */
>> +
>> +static struct gdb_symtab *
>> +jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
>> +                      struct gdb_object *object,
>> +                      const char *file_name)
I am not able to see the use of struct gdb_symbol_callbacks *cb inside
the function.

>> +{
>> +  struct gdb_symtab *ret;
>> +
>> +  ret = XZALLOC (struct gdb_symtab);
>> +  ret->file_name = file_name ? xstrdup (file_name) : xstrdup ("");
>> +  ret->next = object->symtabs;
>> +  object->symtabs = ret;
>> +  return ret;
>> +}
>> +
>> +/* Returns true if the block corresponding to old should be placed
>> +   before the block corresponding to new in the final blockvector. */
>> +
>> +static int
>> +compare_block (struct gdb_block *old, struct gdb_block *new)
I think this should be
+static int
+compare_block (const struct gdb_block *const old, const struct
gdb_block *const 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;
>> +}
>> +
>> +/* Called by readers to open a new gdb_block. This function also
>> +   inserts the new gdb_block in the correct place in the corresponding
>> +   gdb_symtab. */
>> +
>> +static struct gdb_block *
>> +jit_block_open_impl (struct gdb_symbol_callbacks *cb,
>> +                     struct gdb_symtab *symtab, struct gdb_block *parent,
>> +                     GDB_CORE_ADDR begin, GDB_CORE_ADDR end, const char *name)
>> +{
>> +  struct gdb_block *block = XZALLOC (struct gdb_block);
>> +
>> +  block->next = symtab->blocks;
>> +  block->begin = (CORE_ADDR) begin;
>> +  block->end = (CORE_ADDR) 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 gdb_block *i = symtab->blocks;
>> +
>> +      for (;; i = i->next)
>> +        {
>> +          /* Guaranteed 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;
>> +}
>> +
>> +/* Readers call this to add a line mapping (from PC to line number) to
>> +   a gdb_symtab. */
>>
>>  static void
>> -jit_register_code (struct gdbarch *gdbarch,
>> -                  CORE_ADDR entry_addr, struct jit_code_entry *code_entry)
>> +jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
>> +                                  struct gdb_symtab *stab, int nlines,
>> +                                  struct gdb_line_mapping *map)
>> +{
>> +  int i;
>> +
>> +  if (nlines < 1)
>> +    return;
>> +
>> +  stab->linetable = xmalloc (sizeof (struct linetable)
>> +                             + (nlines - 1) * sizeof (struct linetable_entry));
>> +  stab->linetable->nitems = nlines;
>> +  for (i = 0; i < nlines; i++)
>> +    {
>> +      stab->linetable->item [i].pc = (CORE_ADDR) map[i].pc;
>> +      stab->linetable->item [i].line = map[i].line;
>> +    }
>> +}
>> +
>> +/* Called by readers to close a gdb_symtab. Does not need to do
>> +   anything as of now. */
>> +
>> +static void
>> +jit_symtab_close_impl (struct gdb_symbol_callbacks *cb,
>> +                       struct gdb_symtab *stab)
>> +{
>> +}
Do we need at all this definition?

>> +
>> +/* Transform STAB to a proper symtab, and add it it OBJFILE. */
>> +
>> +static void
>> +finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
>>  {
>> +  struct symtab *symtab;
>> +  struct gdb_block *gdb_block_iter, *gdb_block_iter_tmp;
>> +  struct block *block_iter;
>> +  int actual_nblocks, i, blockvector_size;
>> +  CORE_ADDR begin, end;
>> +
>> +  actual_nblocks = FIRST_LOCAL_BLOCK + stab->nblocks;
>> +
>> +  symtab = allocate_symtab (stab->file_name, objfile);
>> +  symtab->dirname = NULL; /* JIT compilers compile in memory */
I am not sure whether this way of putting comment is allowed under GNU style.

>> +
>> +  /* Copy over the linetable entry if one was provided. */
>> +  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;
>> +    }
>> +
>> +  blockvector_size = (sizeof (struct blockvector)
>> +                      + (actual_nblocks - 1) * sizeof (struct block *));
>> +  symtab->blockvector = obstack_alloc (&objfile->objfile_obstack,
>> +                                       blockvector_size);
>> +
>> +  /* (begin, end) will contain the PC range this entire blockvector spans. */
>> +  symtab->primary = 1;
>> +  BLOCKVECTOR_MAP (symtab->blockvector) = NULL;
>> +  begin = stab->blocks->begin;
>> +  end = stab->blocks->end;
>> +  BLOCKVECTOR_NBLOCKS (symtab->blockvector) = actual_nblocks;
>> +
>> +  /* First run over all the gdb_block objects, creating a real block
>> +     object for each. Simultaneously, keep setting the real_block
>> +     fields. */
>> +  for (i = (actual_nblocks - 1), gdb_block_iter = stab->blocks;
>> +       i >= FIRST_LOCAL_BLOCK; i--, gdb_block_iter = gdb_block_iter->next)
>> +    {
>> +      struct block *new_block = allocate_block (&objfile->objfile_obstack);
>> +      struct symbol *block_name = obstack_alloc (&objfile->objfile_obstack,
>> +                                                 sizeof (struct symbol));
>> +
>> +      BLOCK_DICT (new_block) = dict_create_linear (&objfile->objfile_obstack,
>> +                                                   NULL);
>> +      /* The address range. */
>> +      BLOCK_START (new_block) = (CORE_ADDR) gdb_block_iter->begin;
>> +      BLOCK_END (new_block) = (CORE_ADDR) gdb_block_iter->end;
>> +
>> +      /* The name. */
>> +      memset (block_name, 0, sizeof (struct symbol));
>> +      SYMBOL_DOMAIN (block_name) = VAR_DOMAIN;
>> +      SYMBOL_CLASS (block_name) = LOC_BLOCK;
>> +      SYMBOL_SYMTAB (block_name) = symtab;
>> +      SYMBOL_BLOCK_VALUE (block_name) = new_block;
>> +
>> +      block_name->ginfo.name = obsavestring (gdb_block_iter->name,
>> +                                             strlen (gdb_block_iter->name),
>> +                                             &objfile->objfile_obstack);
>> +
>> +      BLOCK_FUNCTION (new_block) = block_name;
>> +
>> +      BLOCKVECTOR_BLOCK (symtab->blockvector, i) = new_block;
>> +      if (begin > BLOCK_START (new_block))
>> +        begin = BLOCK_START (new_block);
>> +      if (end < BLOCK_END (new_block))
>> +        end = BLOCK_END (new_block);
>> +
>> +      gdb_block_iter->real_block = new_block;
>> +    }
>> +
>> +  /* Now add the special blocks. */
>> +  block_iter = NULL;
>> +  for (i = 0; i < FIRST_LOCAL_BLOCK; i++)
>> +    {
>> +      struct block *new_block = allocate_block (&objfile->objfile_obstack);
>> +      BLOCK_DICT (new_block) = dict_create_linear (&objfile->objfile_obstack,
>> +                                                   NULL);
>> +      BLOCK_SUPERBLOCK (new_block) = block_iter;
>> +      block_iter = new_block;
>> +
>> +      BLOCK_START (new_block) = (CORE_ADDR) begin;
>> +      BLOCK_END (new_block) = (CORE_ADDR) end;
>> +
>> +      BLOCKVECTOR_BLOCK (symtab->blockvector, i) = new_block;
>> +    }
>> +
>> +  /* Fill up the superblock fields for the real blocks, using the
>> +     real_block fields populated earlier. */
>> +  for (gdb_block_iter = stab->blocks; gdb_block_iter;
>> +       gdb_block_iter = gdb_block_iter->next)
>> +    {
>> +      if (gdb_block_iter->parent != NULL)
>> +        BLOCK_SUPERBLOCK (gdb_block_iter->real_block) =
>> +          gdb_block_iter->parent->real_block;
>> +    }
>> +
>> +  /* Free memory. */
>> +  gdb_block_iter = stab->blocks;
>> +
>> +  for (gdb_block_iter = stab->blocks, gdb_block_iter_tmp = gdb_block_iter->next;
>> +       gdb_block_iter; gdb_block_iter = gdb_block_iter_tmp)
>> +    {
>> +      xfree ((void *) gdb_block_iter->name);
>> +      xfree (gdb_block_iter);
>> +    }
>> +  xfree (stab->linetable);
>> +  xfree ((char *) stab->file_name);
>> +  xfree (stab);
>> +}
>> +
>> +/* Called when closing a gdb_objfile. Converts OBJ to a proper objfile. */
>> +
>> +static void
>> +jit_object_close_impl (struct gdb_symbol_callbacks *cb,
>> +                       struct gdb_object *obj)
>> +{
>> +  struct gdb_symtab *i, *j;
>> +  struct objfile *objfile;
>> +  struct jit_dbg_reader_data *priv_data;
>> +
>> +  priv_data = cb->priv_data;
>> +
>> +  objfile = allocate_objfile (NULL, 0);
>> +  objfile->gdbarch = target_gdbarch;
>> +  objfile->msymbols = obstack_alloc (&objfile->objfile_obstack,
>> +                                     sizeof (struct minimal_symbol));
>> +  objfile->msymbols[0].ginfo.name = NULL;
>> +  objfile->msymbols[0].ginfo.value.address = 0;
>> +
>> +  xfree (objfile->name);
>> +  objfile->name = xstrdup ("<< JIT compiled code >>");
>> +
>> +  j = NULL;
>> +  for (i = obj->symtabs; i; i = j)
>> +    {
>> +      j = i->next;
>> +      finalize_symtab (i, objfile);
>> +    }
>> +  add_objfile_entry (objfile, priv_data->entry_address);
>> +  xfree (obj);
>> +}
>> +
>> +/* Try to read CODE_ENTRY using the loaded jit reader (if any). */
>> +
>> +static int
>> +jit_reader_try_read_symtab (struct jit_code_entry *code_entry)
>> +{
>> +  void *gdb_mem;
>> +  int status = 0;
>> +  struct jit_dbg_reader *i;
>> +  struct jit_dbg_reader_data priv_data;
>> +  struct gdb_reader_funcs *funcs;
>> +  struct gdb_symbol_callbacks callbacks =
>> +    {
>> +      jit_object_open_impl,
>> +      jit_symtab_open_impl,
>> +      jit_block_open_impl,
>> +      jit_symtab_close_impl,
>> +      jit_object_close_impl,
>> +
>> +      jit_symtab_line_mapping_add_impl,
>> +      jit_target_read_impl,
>> +
>> +      &priv_data
>> +    };
>> +
>> +  priv_data.entry_address = code_entry->symfile_addr;
>> +
>> +  if (!loaded_jit_reader)
>> +    return 0;
>> +
>> +  gdb_mem = xmalloc (code_entry->symfile_size);
>> +  if (target_read_memory (code_entry->symfile_addr, gdb_mem,
>> +                          code_entry->symfile_size))
>> +    {
>> +      status = 0;
>> +      goto cleanup;
>> +    }
>> +
>> +  funcs = loaded_jit_reader->functions;
>> +  if (funcs->read (funcs, &callbacks, gdb_mem, code_entry->symfile_size)
>> +      == GDB_SUCCESS)
>> +    {
>> +      status = 1;
>> +      goto cleanup;
>> +    }
>> +
>> + cleanup:
>> +  xfree (gdb_mem);
>> +  return status;
>> +}
>> +
>> +/* Try to read CODE_ENTRY using BFD. */
>> +
>> +static void
>> +jit_bfd_try_read_symtab (struct jit_code_entry *code_entry,
>> +                         struct gdbarch *gdbarch)
>> +{
>> +  struct cleanup *old_cleanups;
>> +  struct objfile *objfile;
>>   bfd *nbfd;
>>   struct section_addr_info *sai;
>>   struct bfd_section *sec;
>> -  struct objfile *objfile;
>> -  struct cleanup *old_cleanups, *my_cleanups;
>> -  int i;
>>   const struct bfd_arch_info *b;
>> -  CORE_ADDR *entry_addr_ptr;
>> -
>> -  if (jit_debug)
>> -    fprintf_unfiltered (gdb_stdlog,
>> -                       "jit_register_code, symfile_addr = %s, "
>> -                       "symfile_size = %s\n",
>> -                       paddress (gdbarch, code_entry->symfile_addr),
>> -                       pulongest (code_entry->symfile_size));
>> +  int i;
>>
>>   nbfd = bfd_open_from_target_memory (code_entry->symfile_addr,
>>                                       code_entry->symfile_size, gnutarget);
>> @@ -395,12 +797,34 @@ JITed symbol file is not an object file, ignoring it.\n"));
>>   /* This call takes ownership of sai.  */
>>   objfile = symbol_file_add_from_bfd (nbfd, 0, sai, OBJF_SHARED, NULL);
>>
>> -  /* Remember a mapping from entry_addr to objfile.  */
>> -  entry_addr_ptr = xmalloc (sizeof (CORE_ADDR));
>> -  *entry_addr_ptr = entry_addr;
>> -  set_objfile_data (objfile, jit_objfile_data, entry_addr_ptr);
>> -
>>   discard_cleanups (old_cleanups);
>> +  add_objfile_entry (objfile, code_entry->symfile_addr);
>> +}
>> +
>> +/* 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
>> +   a symbol file added by the user.  */
>> +
>> +static void
>> +jit_register_code (struct gdbarch *gdbarch,
>> +                   CORE_ADDR entry_addr, struct jit_code_entry *code_entry)
>> +{
>> +  int i, success;
>> +  const struct bfd_arch_info *b;
>> +  struct jit_inferior_data *inf_data = get_jit_inferior_data ();
>> +
>> +  if (jit_debug)
>> +    fprintf_unfiltered (gdb_stdlog,
>> +                        "jit_register_code, symfile_addr = %s, "
>> +                        "symfile_size = %s\n",
>> +                        paddress (gdbarch, code_entry->symfile_addr),
>> +                        pulongest (code_entry->symfile_size));
>> +
>> +  success = jit_reader_try_read_symtab (code_entry);
>> +
>> +  if (!success)
>> +    jit_bfd_try_read_symtab (code_entry, gdbarch);
>>  }
>>
>>  /* This function unregisters JITed code and frees the corresponding
>> @@ -635,6 +1059,14 @@ jit_event_handler (struct gdbarch *gdbarch)
>>     }
>>  }
>>
>> +/* Called to free the data allocated to the jit_inferior_data slot. */
>> +
>> +static void
>> +free_objfile_data (struct objfile *objfile, void *data)
>> +{
>> +  xfree (data);
>> +}
>> +
>>  /* Provide a prototype to silence -Wmissing-prototypes.  */
>>
>>  extern void _initialize_jit (void);
>> @@ -655,7 +1087,8 @@ _initialize_jit (void)
>>   observer_attach_inferior_created (jit_inferior_created_observer);
>>   observer_attach_inferior_exit (jit_inferior_exit_hook);
>>   observer_attach_executable_changed (jit_executable_changed_observer);
>> -  jit_objfile_data = register_objfile_data ();
>> +  jit_objfile_data =
>> +    register_objfile_data_with_cleanup (NULL,free_objfile_data);
>>   jit_inferior_data =
>>     register_inferior_data_with_cleanup (jit_inferior_data_cleanup);
>>   add_com ("jit-reader-load", no_class, jit_reader_load_command, _("\
>> --
>> 1.7.5.4
>>
>>
>