From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 4D5E03893C63 for ; Wed, 9 Jun 2021 01:58:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4D5E03893C63 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 1591witS030836 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 8 Jun 2021 21:58:49 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 1591witS030836 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8A1D41E939; Tue, 8 Jun 2021 21:58:44 -0400 (EDT) Subject: Re: [PATCH 2/2] Remove dwarf2_cu::language To: Tom Tromey , gdb-patches@sourceware.org References: <20210608152630.4155280-1-tom@tromey.com> <20210608152630.4155280-3-tom@tromey.com> From: Simon Marchi Message-ID: Date: Tue, 8 Jun 2021 21:58:44 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210608152630.4155280-3-tom@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 9 Jun 2021 01:58:44 +0000 X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Jun 2021 01:58:51 -0000 On 2021-06-08 11:26 a.m., Tom Tromey wrote: > @@ -20350,51 +20354,51 @@ set_cu_language (unsigned int lang, struct dwarf2_cu *cu) If this function stays as-is in the previous patch, I think it should take the per_cu as a parameter, instead of the cu. And maybe be renamed set_per_cu_language (and maybe a candidate to be a method). > @@ -24415,17 +24420,17 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die, > attribute is not standardised yet. As a workaround for the > language detection we fall back to the DW_AT_producer > string. */ > - cu->language = language_opencl; > + cu->per_cu->lang = language_opencl; > } > else if (cu->producer != nullptr > && strstr (cu->producer, "GNU Go ") != NULL) > { > /* Similar hack for Go. */ > - cu->language = language_go; > + cu->per_cu->lang = language_go; > } > else > - cu->language = pretend_language; > - cu->language_defn = language_def (cu->language); > + cu->per_cu->lang = pretend_language; > + cu->language_defn = language_def (cu->per_cu->lang); It's not totally clear to me why it's better to go this route, eliminate dwarf2_cu::language and keep dwarf2_per_cu_data::lang. Because I find it a bit hard to follow that a dwarf2_per_cu_data is initialized by a dwarf2_cu being constructed. That suggests to me that the field would belong in dwarf2_cu. >From what I see, the only possible way for dwarf2_per_cu_data::lang to be set is if a dwarf2_cu exists (or has existed) for this per_cu. And the only place where dwarf2_per_cu_data::lang is read (prior to this patch) is in process_imported_unit_die: /* We're importing a C++ compilation unit with tag DW_TAG_compile_unit into another compilation unit, at root level. Regard this as a hint, and ignore it. */ if (die->parent && die->parent->parent == NULL && per_cu->unit_type == DW_UT_compile && per_cu->lang == language_cplus) return; So here, instead of referring to per_cu->lang, we could perhaps look up to see if a dwarf2_cu exists for `per_cu`, and lookup the language there? The downside would be if a dwarf2_cu has existed but was then freed, then we won't see the language. Whereas by storing the language in dwarf2_per_cu_data::lang, it will persist even if the dwarf2_cu gets freed. I'm not sure what my point is here, I think the patch looks good, but I just wanted to share these thoughts in case it lights a light bulb in your head. Simon