Re: [PATCH] Fission support for multiple CUs per DWO file

[David Blaikie <dblaikie at gmail dot com>]

gdb/ChangeLog:

2017-05-12  David Blaikie  <dblaikie@gmail.com>

* dwarf2read.c (struct dwo_file): Use a htab of dwo_unit* (rather than
a singular dwo_unit*) to support multiple CUs in the same way that
multiple TUs are supported.
(static void create_cus_hash_table): Replace create_dwo_cu with a
function for parsing multiple CUs from a DWO file.
(open_and_init_dwo_file): Use create_cus_hash_table rather than create_dwo_cu.
(lookup_dwo_cutu): Lookup CU in the hash table in the dwo_file with
htab_find, rather than comparing the signature to a singleton CU in
the dwo_file.

gdb/testsuite/ChangeLog:

2017-05-12  David Blaikie  <dblaikie@gmail.com>

* gdb.dwarf2/fission-multi-cu.S: Test containing multiple CUs in a
DWO, built from fission-multi-cu{1,2}.c.
* gdb.dwarf2/fission-multi-cu.exp: Test similar to fission-base.exp,
except putting 'main' and 'func' in separate CUs while in the same DWO
file.
* gdb.dwarf2/fission-multi-cu1.c: First CU for the multi-CU-single-DWO test.
* gdb.dwarf2/fission-multi-cu2.c: Second CU for the multi-CU-single-DWO test.

On Mon, May 22, 2017 at 11:01 AM, Doug Evans <dje@google.com> wrote:
> David Blaikie writes:
>  > ...
>
> Hi. Review comments inline.

Hi - thanks for the review!

(responses inline, patch addressing the issues is attached)

Do let me know if there's other things that'd be good to address or if
this looks good as-is.

Thanks,
- Dave

> All nits.
>
> First one: Tab instead of spaces throughout (including ChangeLog entries).

Think I got all that addressed - though GMail seems to be getting in
my way of adding tabs into the ChangeLog entries inline in the email.
I'll be sure they're there when I commit it.

>
> The testcase is fine with me.
>
>  > gdb/ChangeLog:
>  >
>  > 2017-05-12  David Blaikie  <dblaikie@gmail.com>
>  >
>  >         * dwarf2read.c (struct dwo_file): Use a htab of dwo_unit*
>  > (rather than a singular dwo_unit*) to support multiple CUs in the same
>  > way that multiple TUs are supported.
>  >         (static void create_cus_hash_table): Replace create_dwo_cu
>  > with a function for parsing multiple CUs from a DWO file.
>  >         (open_and_init_dwo_file): Use create_cus_hash_table rather
>  > than create_dwo_cu.
>  >         (lookup_dwo_cutu): Lookup CU in the hash table in the dwo_file
>  > with htab_find, rather than comparing the signature to a singleton CU
>  > in the dwo_file.
>  >
>  > gdb/testsuite/ChangeLog:
>  >
>  > 2017-05-12  David Blaikie  <dblaikie@gmail.com>
>  >
>  >         * gdb.dwarf2/fission-multi-cu.S: Test containing multiple CUs
>  > in a DWO, built from fission-multi-cu{1,2}.c.
>  >         * gdb.dwarf2/fission-multi-cu.exp: Test similar to
>  > fission-base.exp, except putting 'main' and 'func' in separate CUs
>  > while in the same DWO file.
>  >         * gdb.dwarf2/fission-multi-cu1.c: First CU for the
>  > multi-CU-single-DWO test.
>  >         * gdb.dwarf2/fission-multi-cu2.c: Second CU for the
>  > multi-CU-single-DWO test.
>  > diff --git gdb/dwarf2read.c gdb/dwarf2read.c
>  > index b58d0fc16e..29eb5a14b2 100644
>  > --- gdb/dwarf2read.c
>  > +++ gdb/dwarf2read.c
>  > @@ -852,12 +852,9 @@ struct dwo_file
>  >       sections (for lack of a better name).  */
>  >    struct dwo_sections sections;
>  >
>  > -  /* The CU in the file.
>  > -     We only support one because having more than one requires hacking the
>  > -     dwo_name of each to match, which is highly unlikely to happen.
>  > -     Doing this means all TUs can share comp_dir: We also assume that
>  > -     DW_AT_comp_dir across all TUs in a DWO file will be identical.  */
>  > -  struct dwo_unit *cu;
>  > +  /* The CUs in the file.
>  > +     Each element is a struct dwo_unit. */
>
> Since this is currently non-standard, I think it will help some readers
> to elaborate on the Why of things here. IOW, add a comment explaining why
> we're now supporting multi-CUs in one DWO file.

Commented about the use case here & that it's supported as an extension.

>
>  > +  htab_t cus;
>  >
>  >    /* Table of TUs in the file.
>  >       Each element is a struct dwo_unit.  */
>  > @@ -9702,72 +9699,75 @@ create_dwo_cu_reader (const struct die_reader_specs *reader,
>  >                      hex_string (dwo_unit->signature));
>  >  }
>  >
>  > -/* Create the dwo_unit for the lone CU in DWO_FILE.
>  > -   Note: This function processes DWO files only, not DWP files.  */
>
> Need to keep the function comment (just reword it).
> And please keep the note about only being used for DWO files, not DWP files.

Kept & reworded.

>
>  > -
>  > -static struct dwo_unit *
>  > -create_dwo_cu (struct dwo_file *dwo_file)
>  > +static void create_cus_hash_table (struct dwo_file &dwo_file,
>  > +                                   dwarf2_section_info &section,
>  > +                                   htab_t &cus_htab)
>  >  {
>  >    struct objfile *objfile = dwarf2_per_objfile->objfile;
>  > -  struct dwarf2_section_info *section = &dwo_file->sections.info;
>  > +  const struct dwarf2_section_info *abbrev_section = &dwo_file.sections.abbrev;
>  >    const gdb_byte *info_ptr, *end_ptr;
>  > -  struct create_dwo_cu_data create_dwo_cu_data;
>  > -  struct dwo_unit *dwo_unit;
>  >
>  > -  dwarf2_read_section (objfile, section);
>  > -  info_ptr = section->buffer;
>  > +  dwarf2_read_section (objfile, &section);
>  > +  info_ptr = section.buffer;
>  >
>  >    if (info_ptr == NULL)
>  > -    return NULL;
>  > +    return;
>  >
>  >    if (dwarf_read_debug)
>  >      {
>  >        fprintf_unfiltered (gdb_stdlog, "Reading %s for %s:\n",
>  > -                      get_section_name (section),
>  > -                      get_section_file_name (section));
>  > +                      get_section_name (&section),
>  > +                      get_section_file_name (&section));
>  >      }
>  >
>  > -  create_dwo_cu_data.dwo_file = dwo_file;
>  > -  dwo_unit = NULL;
>  > -
>  > -  end_ptr = info_ptr + section->size;
>  > +  end_ptr = info_ptr  + section.size;
>
> extra space

Removed.

>
>  >    while (info_ptr < end_ptr)
>  >      {
>  >        struct dwarf2_per_cu_data per_cu;
>  > +      struct create_dwo_cu_data create_dwo_cu_data;
>  > +      struct dwo_unit *dwo_unit;
>  > +      void **slot;
>  > +      sect_offset sect_off = (sect_offset) (info_ptr - section.buffer);
>  >
>  >        memset (&create_dwo_cu_data.dwo_unit, 0,
>  >            sizeof (create_dwo_cu_data.dwo_unit));
>  >        memset (&per_cu, 0, sizeof (per_cu));
>  >        per_cu.objfile = objfile;
>  >        per_cu.is_debug_types = 0;
>  > -      per_cu.sect_off = sect_offset (info_ptr - section->buffer);
>  > -      per_cu.section = section;
>  > +      per_cu.sect_off = sect_offset (info_ptr - section.buffer);
>  > +      per_cu.section = &section;
>  > +      create_dwo_cu_data.dwo_file = &dwo_file;
>  >
>  > -      init_cutu_and_read_dies_no_follow (&per_cu, dwo_file,
>  > +      init_cutu_and_read_dies_no_follow (&per_cu, &dwo_file,
>  >                                       create_dwo_cu_reader,
>  >                                       &create_dwo_cu_data);
>  > +      info_ptr += per_cu.length;
>  >
>  > -      if (create_dwo_cu_data.dwo_unit.dwo_file != NULL)
>  > -    {
>  > -      /* If we've already found one, complain.  We only support one
>  > -         because having more than one requires hacking the dwo_name of
>  > -         each to match, which is highly unlikely to happen.  */
>  > -      if (dwo_unit != NULL)
>  > -        {
>  > -          complaint (&symfile_complaints,
>  > -                     _("Multiple CUs in DWO file %s [in module %s]"),
>  > -                     dwo_file->dwo_name, objfile_name (objfile));
>  > -          break;
>  > -        }
>
> Add a comment explaining why this test is present:
>
>  > +      if (create_dwo_cu_data.dwo_unit.dwo_file == NULL)
>  > +        continue;

Added a comment & fixed a mistake I introduced by initializing
dwo_file before this call rather than letting the call initialize it.
Looks like the original code was using this for error detection & I
attempted to keep that the same but broke it by initializing the value
early.

>  >
>  > -      dwo_unit = OBSTACK_ZALLOC (&objfile->objfile_obstack, struct dwo_unit);
>  > -      *dwo_unit = create_dwo_cu_data.dwo_unit;
>  > -    }
>  > +      if (cus_htab == NULL)
>
> Remove the surrounding braces.
>
>  > +        {
>  > +          cus_htab = allocate_dwo_unit_table (objfile);
>  > +        }

Removed & reduced indentation.

Attachment: multiple_cus_per_dwo.diff
Description: Text document