Re: possible fix for PR symtab/23010

Keith Seitz
2018-04-17T19:17:04Z

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