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
cmtice@google.com

gdb/ChangeLog

2020-08-06  Caroline Tice <cmtice@google.com>

        * 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
cmtice@google.com


On Wed, Aug 5, 2020 at 1:14 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Caroline" == Caroline Tice via Gdb-patches <gdb-patches@sourceware.org> 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