From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x432.google.com (mail-pf1-x432.google.com [IPv6:2607:f8b0:4864:20::432]) by sourceware.org (Postfix) with ESMTPS id 6D731385842A for ; Tue, 9 Apr 2024 13:36:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6D731385842A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6D731385842A Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::432 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712669773; cv=none; b=FtNRYAQ15iYvVT1ndCqWTSjP7U56PK1997fClIZ51YZS9GgKsS/4C2Z191lbAYACjvuub4eaeqMocLsZY2XPdRj1pWczGRgCMB5GBDTq/FVBqptYjqxNu1pHHANndt8DVniUvjOJJImIn1Uy98kD8hvp+LDwBAMvbcl5ClDfwj0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712669773; c=relaxed/simple; bh=YC2ioWvlOji735WIRNZ1L5juC2kdzsA7XAvmqSjNAD4=; h=DKIM-Signature:Message-ID:Date:From:To:Subject:MIME-Version; b=Ix6IY8EVJ1l1c8aedyOKaB3GDn3+E6Bpb8qGZ1vL29CPq5+lBmh4bn8tJXds0ccYx09qIQWBtihPEOenqigyMBeUIfww/jKmtetv+xxuwUbsWmK3eBanGYJZm69DTB1AtS+W9ML1FInhC8axkYsZPjgKporxOSQqJ3dFjhV+KcE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x432.google.com with SMTP id d2e1a72fcca58-6ed112c64beso2300779b3a.1 for ; Tue, 09 Apr 2024 06:36:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1712669768; x=1713274568; darn=gcc.gnu.org; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=kSH6d/lQlKXj/mO1h4L9j9/mmwWX2hrGp841/qk0Cj8=; b=ll9XXZe8ESQC9kH9SftjSMiVqXJzUms8oILJl/I/PnfoPpxTVrGRDWkJ/uhJpb/LrS sfWuWjhtuTOpte04LqOOk8QVviWpbb+2p78F64Zochn4Mr/NnSf4g7/21/nLff1pri2E Sf8BQ4dhgpru2oW3H1YB5LDsAA6BHtkLOHXtyFN9Lj0+pV8BE9Q+/dKu7Irqzmq0d+7l EnZSkU+ryI+ylr8bb7DD/D2mSDOjPNjjH/z7+HYfQiUUKCyf2C5FYGsUR/kTJ2DNUQi/ hPUKC7oeoDX7Si7ROiaxoaXyRdtj4fuF10EOnJeosFqM/QuF9ud2QVxvNSy+K0hdjp/N iyRg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712669768; x=1713274568; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=kSH6d/lQlKXj/mO1h4L9j9/mmwWX2hrGp841/qk0Cj8=; b=aOJPRikezStCNzkJznwr60/NHFTIBzt6z0/taQrwbhjwT4NskanGORRTDHtTd+ulQP g0LdonPIJHAL2+xV74Ap0/ZdgBVvZgAGtCCF5lpxUoF+mRfH5xuvvQQh7vWwjkRdPKT9 7o6LdkV5Ede0KByrP2DCcEuxZbPZCCyNVPMrvdV1Be8EJtgeGv2bH3hk2JnHvzxEviR+ 9F9a4qEPPQf2u4LqlWXkePK9oW51hZIoEC2z9q8tsxqOE8PvlPvz9BCNwPzp4rh5q97/ 0bdfAcVd2MkEGb3H2yy+eSxRv3Ac8SuJoJfHDP6m+Ibc7/rLxwwRH6UL+bUyikT7BKRO hluA== X-Gm-Message-State: AOJu0YzINdeJf5Aev3yvmpGUJTGFk3Qve3RpcSgHhZbiBhbcSCtcSP7P TgrsCt01oVa/8Ils3/LNS7IcH+r6Z1ILhYPOgMnIjmQVZ2UXNhic X-Google-Smtp-Source: AGHT+IGhhBC6OFcYodeTIoNa1LWJZQMO/Bw3dT/81LazFnu8BY0oKDS9krVpdfhKlmXa9RoPuVxk9g== X-Received: by 2002:a05:6a21:4982:b0:1a3:e2c6:6ea6 with SMTP id ax2-20020a056a21498200b001a3e2c66ea6mr9704117pzc.23.1712669768139; Tue, 09 Apr 2024 06:36:08 -0700 (PDT) Received: from Thaum. (121-44-11-123.tpgi.com.au. [121.44.11.123]) by smtp.gmail.com with ESMTPSA id it24-20020a056a00459800b006ea81423c65sm8567266pfb.148.2024.04.09.06.36.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Apr 2024 06:36:07 -0700 (PDT) Message-ID: <66154447.050a0220.1e34e.720c@mx.google.com> X-Google-Original-Message-ID: Date: Tue, 9 Apr 2024 23:36:03 +1000 From: Nathaniel Shead To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, Nathan Sidwell Subject: Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] References: <660633b1.170a0220.717f3.817f@mx.google.com> <660ca996.170a0220.60d95.0377@mx.google.com> <6ea38b8a-4146-4c02-b28a-80f1de178e01@redhat.com> <660e8ec1.170a0220.2a0c4.3501@mx.google.com> <40220469-1dfe-4d3f-9a3d-686a074f80b9@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <40220469-1dfe-4d3f-9a3d-686a074f80b9@redhat.com> X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP 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 Mon, Apr 08, 2024 at 11:17:27PM -0400, Jason Merrill wrote: > On 4/4/24 07:27, Nathaniel Shead wrote: > > On Wed, Apr 03, 2024 at 11:18:01AM -0400, Jason Merrill wrote: > > > On 4/2/24 20:57, Nathaniel Shead wrote: > > > > On Tue, Apr 02, 2024 at 01:18:17PM -0400, Jason Merrill wrote: > > > > > On 3/28/24 23:21, Nathaniel Shead wrote: > > > > > > - && !(modules_p () && DECL_DECLARED_INLINE_P (fn))) > > > > > > + && !(modules_p () > > > > > > + && (DECL_DECLARED_INLINE_P (fn) > > > > > > + || DECL_TEMPLATE_INSTANTIATION (fn)))) > > > > > > > > > > How about using vague_linkage_p? > > > > > > > > > > > > > Right, of course. How about this? > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > > > > > -- >8 -- > > > > > > > > A template instantiation still needs to have its DECL_SAVED_TREE so that > > > > its definition is emitted into the CMI. This way it can be emitted in > > > > the object file of any importers that use it, in case it doesn't end up > > > > getting emitted in this TU. > > > > > > > > PR c++/104040 > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * semantics.cc (expand_or_defer_fn_1): Keep DECL_SAVED_TREE for > > > > all vague linkage functions. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * g++.dg/modules/pr104040_a.C: New test. > > > > * g++.dg/modules/pr104040_b.C: New test. > > > > > > > > Signed-off-by: Nathaniel Shead > > > > Reviewed-by: Jason Merrill > > > > --- > > > > gcc/cp/semantics.cc | 5 +++-- > > > > gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++ > > > > gcc/testsuite/g++.dg/modules/pr104040_b.C | 8 ++++++++ > > > > 3 files changed, 25 insertions(+), 2 deletions(-) > > > > create mode 100644 gcc/testsuite/g++.dg/modules/pr104040_a.C > > > > create mode 100644 gcc/testsuite/g++.dg/modules/pr104040_b.C > > > > > > > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > > > > index adb1ba48d29..03800a20b26 100644 > > > > --- a/gcc/cp/semantics.cc > > > > +++ b/gcc/cp/semantics.cc > > > > @@ -5033,9 +5033,10 @@ expand_or_defer_fn_1 (tree fn) > > > > /* We don't want to process FN again, so pretend we've written > > > > it out, even though we haven't. */ > > > > TREE_ASM_WRITTEN (fn) = 1; > > > > - /* If this is a constexpr function, keep DECL_SAVED_TREE. */ > > > > + /* If this is a constexpr function, or the body might need to be > > > > + exported from a module CMI, keep DECL_SAVED_TREE. */ > > > > if (!DECL_DECLARED_CONSTEXPR_P (fn) > > > > - && !(modules_p () && DECL_DECLARED_INLINE_P (fn))) > > > > + && !(modules_p () && vague_linkage_p (fn))) > > > > > > Also, how about module_maybe_has_cmi_p? OK with that change. > > > > Using 'module_maybe_has_cmi_p' doesn't seem to work. This is for two > > reasons, one of them fixable and one of them not (easily): > > > > - It seems that header modules don't count for 'module_maybe_has_cmi_p'; > > I didn't notice this initially, and maybe they should for the > > no-linkage decls too? > > I think so; they could similarly be referred to by an importer. > I'll investigate further and make a patch and test for this when I get a chance then. > > But even accounting for this, > > > > - For some reason only clearing it if the module might have a CMI causes > > crashes in importers for some testcases. I'm not 100% sure why yet, > > but I suspect it might be some duplicate-decls thing where the type > > inconsistently has DECL_SAVED_TREE applied, since this is also called > > on streamed-in declarations. > > Clearing if the module might have a CMI sounds backwards, I'd expect that to > be the case where we want to leave it alone. Is that the problem, or just a > typo? > Sorry typo, yes. I've tried the following incremental patch: diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 5a862a3ee5f..3341ade4e33 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -5036,7 +5036,8 @@ expand_or_defer_fn_1 (tree fn) /* If this is a constexpr function, or the body might need to be exported from a module CMI, keep DECL_SAVED_TREE. */ if (!DECL_DECLARED_CONSTEXPR_P (fn) - && !(modules_p () && vague_linkage_p (fn))) + && !((module_maybe_has_cmi_p () || header_module_p ()) + && vague_linkage_p (fn))) DECL_SAVED_TREE (fn) = NULL_TREE; return false; } and this causes ICEs with e.g. testsuite/g++.dg/modules/concept-6_b.C, where maybe_clone_body is called with a NULL cfun. I think one of the post-load processing loops might have cleared cfun before it got called? Not sure, haven't looked too hard; I can dig in further later if you would like. > > Out of interest, what was the reason that it was cleared at all in the > > first place? I wasn't able to find anything with git blame; is it just > > for performance reasons in avoiding excess lowering later? > > That change goes back to the LTO merge, I believe it was to reduce > unnecessary LTO streaming. > > But now that I think about it some more, I don't see why handling modules > specially here is necessary at all; the point of this code is that after we > build the destructor clones, the DECL_SAVED_TREE of the cloned function is > no longer useful. Why would modules care about the maybe-in-charge > function? > > Jason > The current modules implementation doesn't stream the clones: instead it always just streams the maybe-in-charge functions (including its tree) and recreates the clones on import. I believe Nathan said that there were issues with streaming the clones directly, see https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635882.html