[PATCH, v1] Add code for processing version 5 DWP files (for use with DWARF v5)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH, v1] Add code for processing version 5 DWP files (for use with DWARF v5)

Sourceware - gdb-patches mailing list
The DWARF v5 Spec describes a (slightly) new format for V5 .dwp files.
This patch updates GDB to allow it to read/process .dwp files in the
new DWARF v5 format, while continuing to be able to read/process .dwp
files in the older V1 & V2 formats.

The one thing I felt a little odd about in this patch:  I couldn't
re-use the enum dwarf_sect
definitions, because  in version 5 several of the sections have the
same name as in the previous versions, but have a different ordering,
with different numbers attached.  So I had to create a new enum,
dwarf_sect_v5 for this purpose.

Is this patch ok to commit?

-- Caroline
[hidden email]

gdb/ChangeLog

2020-07-20  Caroline Tice  <[hidden email]>

        * dwarf2/read.c (struct dwo_file): Update comment on 'sections' field.
        (struct dwp_sections): Update field comments.  Add loclists and
        rnglists fields.
        (struct virtual_v2_dwo_sections): Rename struct to
        'virtual_v2_or_v5_dwo_sections'; update comments at top of struct; add
        size & offset fields for loclists and rnglists.
        (struct dwp_hash_table): Add a 'v5' struct field to the union section.
        (create_dwp_hash_table):  Update the large comment above the function to
        discuss Version 5 DWP files as well.  Update all the version checks in
        the function to check for version 5 as well.  Add new section at the
        end to create dwp hash table for version 5.
        (create_dwp_v2_section): Rename function to
        'create_dwp_v2_or_v5_section'.  Update function comment appropriately.
        Add V5 to error message text.
        (create_dwo_unit_in_dwp_v2): Change calls to create_cwp_v2_section
        into calls to create_dwp_v2_or_v5_section.
        (create_dwo_unit_in_dwp_v5): New function.
        (lookup_dwo_unit_in_dwp): Update conditional statement to explicitly
        check for version2; add else clause to handle version 5.
        (dwarf2_locate_v2_dwp_sections): Update function comment to mention
        version 5.
        (dwarf2_locate_v5_dwp_sections): New function.
        (open_and_init_dwp_file): Add else-if clause for version 5 to call
        bfd_map_over_sections with dwarf2_locate_v5_dwp_sections.

include/ChangeLog

2020-07-20  Caroline Tice  <[hidden email]>

        * dwarf2.h (enum dwarf_sect_v5): A new enum section for the
        sections in a DWARF 5 DWP file (DWP version 5).

v1-0002-Add-code-for-processing-version-5-DWP-files-for-u.patch (38K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, v1] Add code for processing version 5 DWP files (for use with DWARF v5)

Simon Marchi-4
On 2020-07-21 12:06 a.m., Caroline Tice wrote:
> The DWARF v5 Spec describes a (slightly) new format for V5 .dwp files.
> This patch updates GDB to allow it to read/process .dwp files in the
> new DWARF v5 format, while continuing to be able to read/process .dwp
> files in the older V1 & V2 formats.

Can you please describe in the commit message what those differences are?

> The one thing I felt a little odd about in this patch:  I couldn't
> re-use the enum dwarf_sect
> definitions, because  in version 5 several of the sections have the
> same name as in the previous versions, but have a different ordering,
> with different numbers attached.  So I had to create a new enum,
> dwarf_sect_v5 for this purpose.

That part would need to be cross-posted to the binutils mailing list.  binutils
does use the DW_SECT_* enumerators, presumably to read dwp files too, so they
would likely use those new DWARF 5 enumerators eventually.

> Is this patch ok to commit?

It would be useful to precise somewhere, perhaps in the comment on `struct dwp_sections`.
that versions 1 and 2 are pre-standard versions, and that version 5 was introduced in
DWARF5.  And that versions 3 and 4 don't exist.

I don't have time to do an in-depth review right now, but one question that came to mind
is: is an advantage of having virtual_v2_or_v5_dwo_sections over having separate
virtual_v2_dwo_sections and virtual_v5_dwo_sections?  Now when using v2 or v5, there are
fields you don't use (because they are either v2-specific or v5-specific), so I imagine
it's just more error prone.  Does it avoid a lot of code duplication?

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, v2] Add code for processing version 5 DWP files (for use with DWARF v5)

Sourceware - gdb-patches mailing list
On Wed, Jul 22, 2020 at 6:04 AM Simon Marchi <[hidden email]> wrote:

> On 2020-07-21 12:06 a.m., Caroline Tice wrote:
> > The DWARF v5 Spec describes a (slightly) new format for V5 .dwp files.
> > This patch updates GDB to allow it to read/process .dwp files in the
> > new DWARF v5 format, while continuing to be able to read/process .dwp
> > files in the older V1 & V2 formats.
>
> Can you please describe in the commit message what those differences are?
>
>
Done.


> > The one thing I felt a little odd about in this patch:  I couldn't
> > re-use the enum dwarf_sect
> > definitions, because  in version 5 several of the sections have the
> > same name as in the previous versions, but have a different ordering,
> > with different numbers attached.  So I had to create a new enum,
> > dwarf_sect_v5 for this purpose.
>
> That part would need to be cross-posted to the binutils mailing list.
> binutils
> does use the DW_SECT_* enumerators, presumably to read dwp files too, so
> they
> would likely use those new DWARF 5 enumerators eventually.
>
>
I will create/submit a patch to the binutils mailing list.


> > Is this patch ok to commit?
>
> It would be useful to precise somewhere, perhaps in the comment on `struct
> dwp_sections`.
> that versions 1 and 2 are pre-standard versions, and that version 5 was
> introduced in
> DWARF5.  And that versions 3 and 4 don't exist.
>

Done.


>
> I don't have time to do an in-depth review right now, but one question
> that came to mind
> is: is an advantage of having virtual_v2_or_v5_dwo_sections over having
> separate
> virtual_v2_dwo_sections and virtual_v5_dwo_sections?


Not particularly; I was just trying to avoid code duplication.


> Now when using v2 or v5, there are
> fields you don't use (because they are either v2-specific or v5-specific),
> so I imagine
> it's just more error prone.  Does it avoid a lot of code duplication?
>

A small amount, but not a lot.


>
> Simon
>

Below is my updated patch (mostly just updated comments & commit message)

v2-0001-Add-code-for-processing-version-5-DWP-files-for-u.patch (41K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, v3] Add code for processing version 5 DWP files (for use with DWARF v5)

Sourceware - gdb-patches mailing list
The changes to include/dwarf2.h were accepted by binutils and have
gone in, so I've removed those changes from this patch.

Is this ok to commit?

-- Caroline
[hidden email]

On Tue, Jul 28, 2020 at 12:22 PM Caroline Tice <[hidden email]> wrote:

>
>
>
>
>
>
> On Wed, Jul 22, 2020 at 6:04 AM Simon Marchi <[hidden email]> wrote:
>>
>> On 2020-07-21 12:06 a.m., Caroline Tice wrote:
>> > The DWARF v5 Spec describes a (slightly) new format for V5 .dwp files.
>> > This patch updates GDB to allow it to read/process .dwp files in the
>> > new DWARF v5 format, while continuing to be able to read/process .dwp
>> > files in the older V1 & V2 formats.
>>
>> Can you please describe in the commit message what those differences are?
>>
>
> Done.
>
>>
>> > The one thing I felt a little odd about in this patch:  I couldn't
>> > re-use the enum dwarf_sect
>> > definitions, because  in version 5 several of the sections have the
>> > same name as in the previous versions, but have a different ordering,
>> > with different numbers attached.  So I had to create a new enum,
>> > dwarf_sect_v5 for this purpose.
>>
>> That part would need to be cross-posted to the binutils mailing list.  binutils
>> does use the DW_SECT_* enumerators, presumably to read dwp files too, so they
>> would likely use those new DWARF 5 enumerators eventually.
>>
>
> I will create/submit a patch to the binutils mailing list.
>
>>
>> > Is this patch ok to commit?
>>
>> It would be useful to precise somewhere, perhaps in the comment on `struct dwp_sections`.
>> that versions 1 and 2 are pre-standard versions, and that version 5 was introduced in
>> DWARF5.  And that versions 3 and 4 don't exist.
>
>
> Done.
>
>>
>>
>> I don't have time to do an in-depth review right now, but one question that came to mind
>> is: is an advantage of having virtual_v2_or_v5_dwo_sections over having separate
>> virtual_v2_dwo_sections and virtual_v5_dwo_sections?
>
>
> Not particularly; I was just trying to avoid code duplication.
>
>>
>> Now when using v2 or v5, there are
>> fields you don't use (because they are either v2-specific or v5-specific), so I imagine
>> it's just more error prone.  Does it avoid a lot of code duplication?
>
>
> A small amount, but not a lot.
>
>>
>>
>> Simon
>
>
> Below is my updated patch (mostly just updated comments & commit message)

v3-0001-Add-code-for-processing-version-5-DWP-files-for-u.patch (41K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, v3] Add code for processing version 5 DWP files (for use with DWARF v5)

Sourceware - gdb-patches mailing list
Ping?

-- Caroline
[hidden email]


On Wed, Jul 29, 2020 at 2:05 PM Caroline Tice <[hidden email]> wrote:

> The changes to include/dwarf2.h were accepted by binutils and have
> gone in, so I've removed those changes from this patch.
>
> Is this ok to commit?
>
> -- Caroline
> [hidden email]
>
> On Tue, Jul 28, 2020 at 12:22 PM Caroline Tice <[hidden email]> wrote:
> >
> >
> >
> >
> >
> >
> > On Wed, Jul 22, 2020 at 6:04 AM Simon Marchi <[hidden email]> wrote:
> >>
> >> On 2020-07-21 12:06 a.m., Caroline Tice wrote:
> >> > The DWARF v5 Spec describes a (slightly) new format for V5 .dwp files.
> >> > This patch updates GDB to allow it to read/process .dwp files in the
> >> > new DWARF v5 format, while continuing to be able to read/process .dwp
> >> > files in the older V1 & V2 formats.
> >>
> >> Can you please describe in the commit message what those differences
> are?
> >>
> >
> > Done.
> >
> >>
> >> > The one thing I felt a little odd about in this patch:  I couldn't
> >> > re-use the enum dwarf_sect
> >> > definitions, because  in version 5 several of the sections have the
> >> > same name as in the previous versions, but have a different ordering,
> >> > with different numbers attached.  So I had to create a new enum,
> >> > dwarf_sect_v5 for this purpose.
> >>
> >> That part would need to be cross-posted to the binutils mailing list.
> binutils
> >> does use the DW_SECT_* enumerators, presumably to read dwp files too,
> so they
> >> would likely use those new DWARF 5 enumerators eventually.
> >>
> >
> > I will create/submit a patch to the binutils mailing list.
> >
> >>
> >> > Is this patch ok to commit?
> >>
> >> It would be useful to precise somewhere, perhaps in the comment on
> `struct dwp_sections`.
> >> that versions 1 and 2 are pre-standard versions, and that version 5 was
> introduced in
> >> DWARF5.  And that versions 3 and 4 don't exist.
> >
> >
> > Done.
> >
> >>
> >>
> >> I don't have time to do an in-depth review right now, but one question
> that came to mind
> >> is: is an advantage of having virtual_v2_or_v5_dwo_sections over having
> separate
> >> virtual_v2_dwo_sections and virtual_v5_dwo_sections?
> >
> >
> > Not particularly; I was just trying to avoid code duplication.
> >
> >>
> >> Now when using v2 or v5, there are
> >> fields you don't use (because they are either v2-specific or
> v5-specific), so I imagine
> >> it's just more error prone.  Does it avoid a lot of code duplication?
> >
> >
> > A small amount, but not a lot.
> >
> >>
> >>
> >> Simon
> >
> >
> > Below is my updated patch (mostly just updated comments & commit message)
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, v3] Add code for processing version 5 DWP files (for use with DWARF v5)

Tom Tromey-2
In reply to this post by Sourceware - gdb-patches mailing list
>>>>> "Caroline" == Caroline Tice via Gdb-patches <[hidden email]> writes:

Caroline> The changes to include/dwarf2.h were accepted by binutils and have
Caroline> gone in, so I've removed those changes from this patch.

Thank you for the patch.

I read it and I have a few minor nits, plus one actual bug.

Caroline> -  if (version == 2)
Caroline> +  if ((version == 2) || (version == 5))

Please remove the extra parentheses on this line.

Caroline> +  else // version == 5

gdb still only uses /* */-style comments.

Caroline> +      int i;

This should be local to the for loop:

Caroline> +      for (i = 0; i < nr_columns; ++i)

... for (int i = ...)

Caroline> +      htab->section_pool.v5.sizes =
Caroline> + htab->section_pool.v5.offsets + (sizeof (uint32_t)

The '=' should go at the start of the second line here.

Caroline> +static struct dwo_unit *
Caroline> +create_dwo_unit_in_dwp_v5 (struct dwarf2_per_objfile *dwarf2_per_objfile,
Caroline> +                           struct dwp_file *dwp_file,
Caroline> +                           uint32_t unit_index,
Caroline> +                           const char *comp_dir,
Caroline> +                           ULONGEST signature, int is_debug_types)
Caroline> +{

...

Caroline> +  const struct dwp_hash_table *dwp_htab =
Caroline> +    is_debug_types ? dwp_file->tus : dwp_file->cus;

'=' on the wrong line.

Caroline> +  int i;

This should also be in the 'for' loop below.

Caroline> +  memset (&sections, 0, sizeof (sections));

Can this just be

    struct virtual_v2_or_v5_dwo_sections sections {};

?

gdb is moving more to a C++-y style where variables are declared near
their first use.

Caroline> +      dwo_file->dwo_name = obstack_strdup (&objfile->objfile_obstack,
Caroline> +                                           virtual_dwo_name);

Here we use objfile::intern now instead.  This matters because it puts
the string into the per-BFD object rather than the objfile obstack, so
that sharing works correctly.  I suspect this approach could cause
crashes in some multi-inferior scenarios.

Caroline> +  dwo_unit = OBSTACK_ZALLOC (&objfile->objfile_obstack, struct dwo_unit);

These were also moved to the per-BFD obstack during the big sharing
series.  The other site doing this now looks like:

  dwo_unit = OBSTACK_ZALLOC (&per_objfile->per_bfd->obstack, struct dwo_unit);

Caroline> +  dwo_unit->section =
Caroline> +    XOBNEW (&objfile->objfile_obstack, struct dwarf2_section_info);

Likewise.

Caroline> +  else // version == 5

Comment style.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, v4] Add code for processing version 5 DWP files (for use with DWARF v5)

Sourceware - gdb-patches mailing list
Thank you for your review!  I think I have addressed all of your
comments, and am attaching the updated patch.  Please let me know if
this is ok to commit now.

-- Caroline
[hidden email]

gdb/ChangeLog

2020-08-06  Caroline Tice <[hidden email]>

        * dwarf2/read.c (struct dwo_file): Update comment on 'sections' field.
        (struct dwp_sections): Update field comments.  Add loclists and
        rnglists fields.
        (struct virtual_v2_dwo_sections): Rename struct to
        'virtual_v2_or_v5_dwo_sections'; update comments at top of struct; add
        size & offset fields for loclists and rnglists.
        (struct dwp_hash_table): Add a 'v5' struct field to the union section.
        (create_debug_type_hash_table): Add 'DW_UT_split_type' to the check for
        skipping dummy type units.
        (create_dwp_hash_table): Update the large comment above the function to
        discuss Version 5 DWP files as well, with references.  Update all the
        version checks in the function to check for version 5 as well.  Add new
        section at the end to create dwp hash table for version 5.
        (create_dwp_v2_section): Rename function to
        'create_dwp_v2_or_v5_section'.  Update function comment appropriately.
        Add V5 to error message text.
        (create_dwo_unit_in_dwp_v2): Change calls to create_dwp_v2_section
        into calls to create_dwp_v2_or_v5_section.
        (create_dwo_unit_in_dwp_v5): New function.
        (lookup_dwo_unit_in_dwp): Update conditional statement to explicitly
        check for version2; add else clause to handle version 5.
        (open_and_init_dwo_file): Add code to check dwarf version & only call
        create_debug_types_hash_table (with sections.types) if version is not 5;
        else call create_debug_type_hash_table, with sections.info.
        (dwarf2_locate_v2_dwp_sections): Update function comment to mention
        version 5.
        (dwarf2_locate_v5_dwp_sections): New function.
        (open_and_init_dwp_file): Add else-if clause for version 5 to call
        bfd_map_over_sections with dwarf2_locate_v5_dwp_sections.



-- Caroline
[hidden email]


On Wed, Aug 5, 2020 at 1:14 PM Tom Tromey <[hidden email]> wrote:

>
> >>>>> "Caroline" == Caroline Tice via Gdb-patches <[hidden email]> writes:
>
> Caroline> The changes to include/dwarf2.h were accepted by binutils and have
> Caroline> gone in, so I've removed those changes from this patch.
>
> Thank you for the patch.
>
> I read it and I have a few minor nits, plus one actual bug.
>
> Caroline> -  if (version == 2)
> Caroline> +  if ((version == 2) || (version == 5))
>
> Please remove the extra parentheses on this line.
>
> Caroline> +  else // version == 5
>
> gdb still only uses /* */-style comments.
>
> Caroline> +      int i;
>
> This should be local to the for loop:
>
> Caroline> +      for (i = 0; i < nr_columns; ++i)
>
> ... for (int i = ...)
>
> Caroline> +      htab->section_pool.v5.sizes =
> Caroline> +     htab->section_pool.v5.offsets + (sizeof (uint32_t)
>
> The '=' should go at the start of the second line here.
>
> Caroline> +static struct dwo_unit *
> Caroline> +create_dwo_unit_in_dwp_v5 (struct dwarf2_per_objfile *dwarf2_per_objfile,
> Caroline> +                           struct dwp_file *dwp_file,
> Caroline> +                           uint32_t unit_index,
> Caroline> +                           const char *comp_dir,
> Caroline> +                           ULONGEST signature, int is_debug_types)
> Caroline> +{
>
> ...
>
> Caroline> +  const struct dwp_hash_table *dwp_htab =
> Caroline> +    is_debug_types ? dwp_file->tus : dwp_file->cus;
>
> '=' on the wrong line.
>
> Caroline> +  int i;
>
> This should also be in the 'for' loop below.
>
> Caroline> +  memset (&sections, 0, sizeof (sections));
>
> Can this just be
>
>     struct virtual_v2_or_v5_dwo_sections sections {};
>
> ?
>
> gdb is moving more to a C++-y style where variables are declared near
> their first use.
>
> Caroline> +      dwo_file->dwo_name = obstack_strdup (&objfile->objfile_obstack,
> Caroline> +                                           virtual_dwo_name);
>
> Here we use objfile::intern now instead.  This matters because it puts
> the string into the per-BFD object rather than the objfile obstack, so
> that sharing works correctly.  I suspect this approach could cause
> crashes in some multi-inferior scenarios.
>
> Caroline> +  dwo_unit = OBSTACK_ZALLOC (&objfile->objfile_obstack, struct dwo_unit);
>
> These were also moved to the per-BFD obstack during the big sharing
> series.  The other site doing this now looks like:
>
>   dwo_unit = OBSTACK_ZALLOC (&per_objfile->per_bfd->obstack, struct dwo_unit);
>
> Caroline> +  dwo_unit->section =
> Caroline> +    XOBNEW (&objfile->objfile_obstack, struct dwarf2_section_info);
>
> Likewise.
>
> Caroline> +       else // version == 5
>
> Comment style.
>
> Tom

v4-0001-Add-code-for-processing-version-5-DWP-files-for-u.patch (40K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, v4] Add code for processing version 5 DWP files (for use with DWARF v5)

Tom Tromey-2
>>>>> "Caroline" == Caroline Tice via Gdb-patches <[hidden email]> writes:

Caroline> Thank you for your review!  I think I have addressed all of your
Caroline> comments, and am attaching the updated patch.  Please let me know if
Caroline> this is ok to commit now.

Yes, this is ok.  Thank you.

Tom