From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 04C8B3858C29 for ; Wed, 31 Jan 2024 16:30:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 04C8B3858C29 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 04C8B3858C29 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706718620; cv=none; b=VDk4LLPnYauR6b7l24kou81kYPrzrp2QwS6zFwMQrGhJ9go8z+b7vSR3I8C14gryq/GFxfNIZXaajVbCRe715wpwMDtj/FYIc31Eivog86jCcxnuKl2u+XJtoeUQ/cGbaMvQnVZw4dDUmsBSDIBdWtnSIQCdmt8F6X+dC/H5OW0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706718620; c=relaxed/simple; bh=akYt/n3NjafQtjNpUGst15U/ycbvsGfjTfBOsbJRN4E=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=IWOG0GepHNk9eVdO8474PIM7hHBw+PdBO1J7DHesJYUUCkAc1t0JFUOozgMIyFyw19b4d593ayXUFVEA3ApYtuf/mXOpwxf1f1cdZNvyyiWkwU4qqPUwpt4E8OnBqvOZA2GxUK4c9dlai1aUya2gb/Kz+3j16hPOrAbrHsvwmQM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1706718617; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=T1DNyM12G8mSzFF1ETUUtztwAAQjxJ6+jv/jG9L4TPg=; b=ORSxx1+L956lDyhHzINAOV+AqaMRUdvE4UOGVKHjUlu1Jngc3Odkk+P8OxGBNatCSx5XU1 6sQplDkKOTk7Qwy8Wes2+hDh2/oD4JxmKBl23nAQa0kQAVF1wO1gCcjimyRQKIyuZAPpSp xHgxg0/8r51DxL4ZaMyc4upYMwupvWk= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-196-_HaYGtPNOGmsiMH7paGoTA-1; Wed, 31 Jan 2024 11:30:14 -0500 X-MC-Unique: _HaYGtPNOGmsiMH7paGoTA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7B746185A784; Wed, 31 Jan 2024 16:30:14 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.70]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3F3D5492BC6; Wed, 31 Jan 2024 16:30:14 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 40VGUBtf291465 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 31 Jan 2024 17:30:12 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 40VGUBXY291464; Wed, 31 Jan 2024 17:30:11 +0100 Date: Wed, 31 Jan 2024 17:30:10 +0100 From: Jakub Jelinek To: "H.J. Lu" Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH v2] Handle private COMDAT function symbol reference in readonly data section Message-ID: Reply-To: Jakub Jelinek References: <20240131022136.572689-1-hjl.tools@gmail.com> MIME-Version: 1.0 In-Reply-To: <20240131022136.572689-1-hjl.tools@gmail.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Jan 30, 2024 at 06:21:36PM -0800, H.J. Lu wrote: > Changes in v2: > > 1. Check decl non-null before dereferencing it. > 2. Update PR rtl-optimization/113617 from Thanks for updating the testcase. > --- a/gcc/varasm.cc > +++ b/gcc/varasm.cc > @@ -7459,16 +7459,46 @@ default_elf_select_rtx_section (machine_mode mode, rtx x, > { > int reloc = compute_reloc_for_rtx (x); > > + tree decl = nullptr; > + > + /* If it is a private COMDAT function symbol reference, call > + function_rodata_section for the read-only or relocated read-only > + data section associated with function DECL so that the COMDAT > + section will be used for the private COMDAT function symbol. */ > + if (HAVE_COMDAT_GROUP) > + { > + if (GET_CODE (x) == CONST > + && GET_CODE (XEXP (x, 0)) == PLUS > + && CONST_INT_P (XEXP (XEXP (x, 0), 1))) > + x = XEXP (XEXP (x, 0), 0); > + > + if (GET_CODE (x) == SYMBOL_REF) > + { > + decl = SYMBOL_REF_DECL (x); > + if (decl > + && (TREE_CODE (decl) != FUNCTION_DECL > + || !DECL_COMDAT_GROUP (decl) > + || TREE_PUBLIC (decl))) > + decl = nullptr; > + } > + } > + > /* ??? Handle small data here somehow. */ > > if (reloc & targetm.asm_out.reloc_rw_mask ()) > { > + if (decl) > + return targetm.asm_out.function_rodata_section (decl, true); As I wrote before, I still very much dislike this. We want to refer to the _ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE private symbol defined in .text._ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE section in _ZN1AIxE3fooExx comdat group from some readonly data memory, and read that from _ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_ function defined in .text._ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_ section in the same comdat group. The patch puts that into .data.rel.ro.local._ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE section in the same comdat group, but that just looks weird and for targets which use section anchors also inefficient. If we have a shared constant pool (otherwise the constants would be emitted into a per-function constant pool of that _ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_ function and would live in something based on that function name. But in case it is shared, it is normally just .data.rel.ro.local or .data.rel.ro section, shared by whatever refers to it. These comdat private symbols are kind of exception, they can still be shared, but have to be shared only within the containing comdat group because it isn't valid to refer to them from other comdat groups. So, it is ok if say two different functions in the same comdat group actually share those MEM constants. Thus, I think for the DECL_COMDAT_GROUP (decl) && HAVE_COMDAT_GROUP case it would be best to make it clear in the section name that it is a .data.rel.ro.local or .data.rel.ro section shared by everything in the comdat group. So, shouldn't it be just .section .data.rel.ro.local,"awG",@progbits,_ZN1AIxE3fooExx,comdat and emit that directly in this function rather than using targetm.asm_out.function_rodata_section? Looking at targetm.asm_out.function_rodata_section, it is default_function_rodata_section on most targets, then on darwin, cygwin, AIX and mcore default_no_function_rodata_section which just returns the shared readonly_data_section (I hope those targets don't DECL_COMDAT_GROUP (decl) && HAVE_COMDAT_GROUP, otherwise it will simply not work) and then loongarch does some ugly magic (which is related to jumptables and so nothing we need to care about here hopefully). Another question is if we need to do anything about the DECL_COMDAT_GROUP (decl) && DECL_SECTION_NAME (decl) && startswith (DECL_SECTION_NAME (decl), ".gnu.linkonce.t.") case (older linkers) (i.e. when using years old GNU linkers). Jakub