From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 43033 invoked by alias); 30 Apr 2018 22:44:59 -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 43003 invoked by uid 89); 30 Apr 2018 22:44:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=no version=3.3.2 spammy=reward, inclusion, Rust, love X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Apr 2018 22:44:58 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id A7D5811713A; Mon, 30 Apr 2018 18:44:56 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 9EfjS8AmANiW; Mon, 30 Apr 2018 18:44:56 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 76D83117132; Mon, 30 Apr 2018 18:44:56 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id BAED183055; Mon, 30 Apr 2018 15:44:54 -0700 (PDT) Date: Mon, 30 Apr 2018 22:44:00 -0000 From: Joel Brobecker To: Keith Seitz Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: possible fix for PR symtab/23010 Message-ID: <20180430224454.wrnu4u45o5gukrxs@adacore.com> References: <87po34kzxh.fsf@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) X-SW-Source: 2018-04/txt/msg00670.txt.bz2 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 > > > > 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