This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fission support for multiple CUs per DWO file
- From: David Blaikie <dblaikie at gmail dot com>
- To: Doug Evans <dje at google dot com>
- Cc: gdb-patches <gdb-patches at sourceware dot org>, Pedro Alves <palves at redhat dot com>
- Date: Tue, 27 Jun 2017 11:52:13 -0700
- Subject: Re: [PATCH] Fission support for multiple CUs per DWO file
- Authentication-results: sourceware.org; auth=none
- References: <94eb2c14fcd6985e8c055020a838@google.com> <CAENS6Ev+Wsfc7Y3fWxwgCns5y3eaT8TLvjyy=0_0R3wgD8Z4Ag@mail.gmail.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 §ion,
>> > + 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, §ion);
>> > + 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 (§ion),
>> > + get_section_file_name (§ion));
>> > }
>> >
>> > - 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 = §ion;
>> > + 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.