Re: possible fix for PR symtab/23010

[Joel Brobecker <brobecker@adacore.com>]

Hello,


> > I was talking with Keith & Pedro about PR symtab/23010 on irc this week,
> > because it was the bug at the base of the Rust -readnow problem that Jan
> > reported.
> > 
> > I came up with this patch yesterday.  It fixes the problem, but I didn't
> > write a test and also I'm not sure if it is the best way to go.
> > 
> > So, I thought I'd send it for commentary.

I am not sure either, but I can't think of anything else.

> >     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