Re: possible fix for PR symtab/23010

[Keith Seitz <keiths at redhat dot com>]

On 04/12/2018 12:06 PM, Tom Tromey wrote:
> 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've looked into this quite a bit, too. In my case, I was looking specifically at the assertion caused by passing "-readnow" with libwebkit.so.debug on Fedora (Jan's reproducer).

I tried for darn near a week to come up with an isolated reproducer to no avail. :-(

Based on what I was seeing, I came up with a different approach, but I don't care for it at all. I find the proposed solution a whole lot less risky. [My approach was to have language_minimal CUs "inherit" the importing CU's language in inherit_abstract_dies. I can dream up a few instances where this might be problematic. I don't know if they're really legitimate use cases, but nothing in the standard specifically discredits my imaginings.]

>     There are two problems with this patch:
>     
>     1. It is difficult to reason about.  There are many cases where I've
>        patched the code to call init_cutu_and_read_dies with the flag set
>        to "please do read partial units" -- but I find it difficult to be
>        sure that this is always correct.
>     
>     2. It is still missing a standalone test case.  This seemed hard.

It is. :-)

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

Aside: didn't we decide that "update all callers" was sufficient? [I'm only curious -- not asking for changes to a silly ChangeLog.]

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:

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index e3f544a6a4..406aa0d52e 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -2870,7 +2870,7 @@ dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu)
>        : (per_cu->v.psymtab == NULL || !per_cu->v.psymtab->readin))
>      {
>        queue_comp_unit (per_cu, language_minimal);
> -      load_cu (per_cu);
> +      load_cu (per_cu, true);
>  
>        /* If we just loaded a CU from a DWO, and we're working with an index
>  	 that may badly handle TUs, load all the TUs in that DWO as well.

and

> @@ -4144,7 +4144,7 @@ dw2_expand_all_symtabs (struct objfile *objfile)
>      {
>        dwarf2_per_cu_data *per_cu = dwarf2_per_objfile->get_cutu (i);
>  
> -      dw2_instantiate_symtab (per_cu);
> +      dw2_instantiate_symtab (per_cu, true);
>      }
>  }
>  

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.

Keith