Re: possible fix for PR symtab/23010

[Joel Brobecker <brobecker at adacore dot com>]

Hello Global Maintainers,

I was wondering if anyone had any thoughts regarding Tom patch.
https://sourceware.org/ml/gdb-patches/2018-04/msg00234.html

Below are my comments on it, and also my interrogations on whether
we might want this patch in 8.1.1 or not.

Additional thoughts:
  - This is a regression
  - This is an internal error, so it can be fairly problematic
  - It only happens with -readnow, it seems, which I assume
    is not widely used considering the performance and memory
    cost of this feature.

I might tip in favor of putting it in, considering the fact that
I don't think there is much of a workaround, but I would not make
that call just on my own, because the patch is far from obvious.


> > >     2018-04-12  Tom Tromey  <tom@tromey.com>
> > >     
> > >             PR symtab/23010:
> > >             * dwarf2read.c (load_cu, dw2_do_instantiate_symtab)
> > >             (dw2_instantiate_symtab): Add skip_partial parameter.
> > >             (dw2_find_last_source_symtab, dw2_map_expand_apply)
> > >             (dw2_lookup_symbol, dw2_expand_symtabs_for_function)
> > >             (dw2_expand_all_symtabs, dw2_expand_symtabs_with_fullname)
> > >             (dw2_expand_symtabs_matching_one)
> > >             (dw2_find_pc_sect_compunit_symtab)
> > >             (dw2_debug_names_lookup_symbol)
> > >             (dw2_debug_names_expand_symtabs_for_function): Update.
> > >             (init_cutu_and_read_dies): Add skip_partial parameter.
> > >             (process_psymtab_comp_unit, build_type_psymtabs_1)
> > >             (process_skeletonless_type_unit, load_partial_comp_unit)
> > >             (psymtab_to_symtab_1): Update.
> > >             (load_full_comp_unit): Add skip_partial parameter.
> > >             (process_imported_unit_die, dwarf2_read_addr_index)
> > >             (follow_die_offset, dwarf2_fetch_die_loc_sect_off)
> > >             (dwarf2_fetch_constant_bytes, dwarf2_fetch_die_type_sect_off)
> > >             (read_signatured_type): Update.
> [...]
> > This patch looks reasonable to me, but I would ask you to consider
> > mentioning why partial_units are skipped where they are (even if to
> > just reference the problem or bug?). That's these two hunks, I think:
> 
> +1.
> 
> > I've been manually testing this patch with everything that I can think
> > of on libwebkit.so, and I cannot trigger anything ill-behaved at all.
> > 
> > I recommend a maintainer approve this, even if it is more a band-aid
> > than a "proper" fix. It fixes a fairly big (and maybe even common)
> > problem with minimal impact/risk.
> 
> I reviewed the patch as best as I could, but as Tom says, it's hard
> to reason. But at the same time, it was conservative, as the new
> param is false by default except in a couple of cases.
> 
> I'd love for another maintainer to take a look, especially if we are
> going to consider this patch for inclusion in 8.1.1. But I can't
> think of someone who was actively involved in this area.
> 
> Considering the fact that this has had two reviews, and also that
> it comes from you, whom I trust quite a bit for changes in this area,
> let's give others a week to provide comments. Failing that, let's
> push it, to see how well it fares.
> 
> We may decide to skip this bug for 8.1.1, though. Although, thinking
> aloud, if there was any regression caused by it, it would be with units
> which haven't been expanded yet, right? A workaround would be to trigger
> the unit's expansion, which seems easy enough. So, small risk vs
> no-crash reward.... Hmmm...
> 
> -- 
> Joel

-- 
Joel