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

[David Blaikie <dblaikie at gmail dot com>]

Ping (no rush, but trying not to forget it myself (: )

On Thu, Jun 1, 2017 at 4:33 PM, David Blaikie <dblaikie@gmail.com> wrote:
> 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.