From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33909 invoked by alias); 7 May 2018 17:13:15 -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 33896 invoked by uid 89); 7 May 2018 17:13:15 -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, tip, approve, H*f:sk:2018043 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, 07 May 2018 17:13:13 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id BE1CB1176DE; Mon, 7 May 2018 13:13:11 -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 OZe2x2wB9TFi; Mon, 7 May 2018 13:13:11 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 8ACFE11766C; Mon, 7 May 2018 13:13:11 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id E7C538304F; Mon, 7 May 2018 10:13:09 -0700 (PDT) Date: Mon, 07 May 2018 17:13: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: <20180507171309.f4yjprji4g3deubw@adacore.com> References: <87po34kzxh.fsf@tromey.com> <20180430224454.wrnu4u45o5gukrxs@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180430224454.wrnu4u45o5gukrxs@adacore.com> User-Agent: NeoMutt/20170113 (1.7.2) X-SW-Source: 2018-05/txt/msg00174.txt.bz2 Hello Global Maintainers, I was wondering if anyone had any thoughts regarding Tom patch. https://sourceware.org/ml/gdb-patches/2018-04/msg00234.html Below are my comments on it, and also my interrogations on whether we might want this patch in 8.1.1 or not. Additional thoughts: - This is a regression - This is an internal error, so it can be fairly problematic - It only happens with -readnow, it seems, which I assume is not widely used considering the performance and memory cost of this feature. I might tip in favor of putting it in, considering the fact that I don't think there is much of a workaround, but I would not make that call just on my own, because the patch is far from obvious. > > > 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 -- Joel