From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 86080 invoked by alias); 17 Apr 2018 19:17:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 85737 invoked by uid 89); 17 Apr 2018 19:17:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_NUMSUBJECT autolearn=ham version=3.3.2 spammy=avail, tus, H*UA:52.5.2, H*u:52.5.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 17 Apr 2018 19:17:01 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B0F8C31327E1; Tue, 17 Apr 2018 19:17:00 +0000 (UTC) Received: from theo.uglyboxes.com (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 727656266D; Tue, 17 Apr 2018 19:17:00 +0000 (UTC) Subject: Re: possible fix for PR symtab/23010 To: Tom Tromey , gdb-patches@sourceware.org References: <87po34kzxh.fsf@tromey.com> From: Keith Seitz Message-ID: Date: Tue, 17 Apr 2018 19:17:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <87po34kzxh.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-04/txt/msg00349.txt.bz2 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 > > 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