From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x635.google.com (mail-pl1-x635.google.com [IPv6:2607:f8b0:4864:20::635]) by sourceware.org (Postfix) with ESMTPS id 63FD1384AB75 for ; Fri, 17 May 2024 12:21:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 63FD1384AB75 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 63FD1384AB75 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::635 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715948511; cv=none; b=Mw7tU3kHKV2taBpemQjOQSqBk049ZWMmKscFeDrTD5ycJBVr4HJtJQ5a9nvkVkHCwDaWFfbyaKHM6y6/iD9YEMmohBFGMO0CAfgmPBq5vbIwAESiXjh0IK61/NgEEBWRYy3cJ4PJ7Yvyr6L5w+Gk4V3Zc4+BkuJuABdbthG2KnM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715948511; c=relaxed/simple; bh=efvpy7lOdQtlXe18p3TycYfGBAIrdSCF3wB9+fReOM4=; h=DKIM-Signature:Message-ID:Date:From:To:Subject:MIME-Version; b=Y+rupzIZH/ajZmpFxYymIx1bG+BGZrXdq7+0RGQowTZul85dgTU625HGe8WIsK+U72IwIobyKuho4YYS+lF5Tfh1vccpdpOYxnFV/+mM966HgkW6DBSBGT2aVgPKyO9nbp0YuY7YvSSysjpVhWTka6MVXs0In8AHAs+1OD3fBFw= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x635.google.com with SMTP id d9443c01a7336-1e651a9f3ffso5188165ad.1 for ; Fri, 17 May 2024 05:21:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1715948508; x=1716553308; 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=eWxgCmlZFSdfbaXMRg7AH859g1QNeNpOzr+78QJreTg=; b=YEp9j4suwvxm9T6cR1zZgTLYXUYCOPrsYKsAZyg6bXVBSw5u7VVKqbjMkse8C4nBn9 jL/5MlGAqHUfmOAdp0U0wkadoGf3/QFfO5sI/lPqC6j//e3jEjpQZ7h2OThs+n9cr8NP a6eJSDTZFDG70+6IZkZd9opbsn4Tw3vXCYrXjr1tU4cV8W1732VCC2ghnpDZm6cfZl3U hi1ZHBoRS1kvd4zdLwAfqOE66RAloqjdFVF2rmiYqO2p33U9ARFuRyFRb/w/4xgep1By zLXVsgn4DpO8C8ITW/6dQO/5JK5ABSpe/seng/nR+M76JpAvAihI7v9VWP5hsnpcH9Sy T9gA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715948508; x=1716553308; 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=eWxgCmlZFSdfbaXMRg7AH859g1QNeNpOzr+78QJreTg=; b=GvV43sjsZPqAZ7PMo5j3XkVXXWpsT5A/j+8qHa1XS4f/W/jT5Ttm26BzXYN3RJyK8U mznpf5UWTgNB6IJW63Zg74g7Vv6f6LsclapDY1hufplsRad/tTIS0VSalt50NtWk+uy+ XnKxQUv7iHyANQYPOuX1c6IGYgwFNdtF4Tjtboy1/dmfQQ5tZDf+tm7I38c3/mQEFYaR SyBm108S5IsGgZXLiQaqUgz64N/4+NZmglYJqOCs9UiVPOn8STZPoADWnwt6vLqxZZax mgXcVRHh3yzOcUyl6YcRJKWOk1KATzA1p/07D0Ugo3QYPF9LXVw3QBOV10fTYjxR1vBl Pw5g== X-Gm-Message-State: AOJu0Yx85WdTBBJKplr0g7O54jBr8Yw+7phYV1aUoJNoUtBOSLXWsGfQ QHohd60cQoU717NON99RTLG2XLjuc/Rlmrnv2X6Vb6QVZalMXUiW X-Google-Smtp-Source: AGHT+IHNMDi1K1/vuyrbB67bL0ZsAfwuPTLlZhT6OyaDO/vpRpv8RxOmQFoAkqTERFDDAieL6+ea3A== X-Received: by 2002:a17:903:947:b0:1e3:1526:77d5 with SMTP id d9443c01a7336-1ef43d28152mr276650005ad.23.1715948507961; Fri, 17 May 2024 05:21:47 -0700 (PDT) Received: from Thaum. (14-200-72-253.tpgi.com.au. [14.200.72.253]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-1ef0badcd5asm156347515ad.96.2024.05.17.05.21.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 May 2024 05:21:47 -0700 (PDT) Message-ID: <66474bdb.170a0220.3a923.8236@mx.google.com> X-Google-Original-Message-ID: Date: Fri, 17 May 2024 22:21:42 +1000 From: Nathaniel Shead To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, Nathan Sidwell Subject: Re: [PATCH v2] c++/modules: Remember that header units have CMIs References: <664181e2.650a0220.dbfd8.e3d5@mx.google.com> <10d5ff7c-3d30-4317-97d9-91ea2370bbfa@redhat.com> <6646f5cb.170a0220.fb17d.75a5@mx.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6646f5cb.170a0220.fb17d.75a5@mx.google.com> X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,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 Fri, May 17, 2024 at 04:14:31PM +1000, Nathaniel Shead wrote: > On Tue, May 14, 2024 at 06:21:48PM -0400, Jason Merrill wrote: > > On 5/12/24 22:58, Nathaniel Shead wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > OK. > > > > I realised as I was looking over this again that I might have spoken too > soon with the header unit example being supported. Doing the following: > > // a.H > struct { int y; } s; > decltype(s) f(decltype(s)); // { dg-error "used but never defined" } > inline auto x = f({ 123 }); > > // b.C > struct {} unrelated; > import "a.H"; > decltype(s) f(decltype(s) x) { > return { 456 + x.y }; > } > > // c.C > import "linkage-3_a.H"; > int main() { auto a = x.y; } > > Actually does fail to link, because in 'c.C' we call 'f(.anon_0)' but > the definition 'b.C' is f(.anon_1). > > I don't think this is fixable, so I don't think this direction is > workable. > > That said, I think that it might still be worth making header modules > satisfy 'module_has_cmi_p', since that is true to the name, and will be > useful in other places we currently use 'module_p ()': in which case we > could instead make all the callers in 'no_linkage_check' do > 'module_maybe_has_cmi_p () && !header_module_p ()'; something like the > following, perhaps? > > But I'm not too fussed about this overall if you think this will just > make things more complicated. Otherwise bootstrapped and regtested (so > far just modules.exp) on x86_64-pc-linux-gnu, OK for trunk if full > regtest passes? > > -- >8 -- > > This appears to be an oversight in the definition of module_has_cmi_p. > This change will allow us to use the function directly in more places > that need to additional work only if generating a module CMI in the > future. > > However, we do need to change callers of 'module_maybe_has_cmi_p'; in > particular header units, though having a CMI, do not provide a TU to > emit names into, and thus each importer will emit their own definitions > which may not match for no-linkage types. > > gcc/cp/ChangeLog: > > * cp-tree.h (module_has_cmi_p): Also true for header units. > * decl.cc (grokfndecl): Disallow no-linkage names in header > units. > * tree.cc (no_linkage_check): Likewise. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/linkage-3.H: New test. > > Signed-off-by: Nathaniel Shead > --- > gcc/cp/cp-tree.h | 2 +- > gcc/cp/decl.cc | 2 +- > gcc/cp/tree.cc | 13 +++++++----- > gcc/testsuite/g++.dg/modules/linkage-3.H | 25 ++++++++++++++++++++++++ > 4 files changed, 35 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3.H > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index ba9e848c177..ac55b5579a1 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7381,7 +7381,7 @@ inline bool module_interface_p () > inline bool module_partition_p () > { return module_kind & MK_PARTITION; } > inline bool module_has_cmi_p () > -{ return module_kind & (MK_INTERFACE | MK_PARTITION); } > +{ return module_kind & (MK_INTERFACE | MK_PARTITION | MK_HEADER); } > > inline bool module_purview_p () > { return module_kind & MK_PURVIEW; } > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 6fcab615d55..f89a7df30b7 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -10802,7 +10802,7 @@ grokfndecl (tree ctype, > used by an importer. We don't just use module_has_cmi_p here > because for entities in the GMF we don't yet know whether this > module will have a CMI, so we'll conservatively assume it might. */ > - publicp = module_maybe_has_cmi_p (); > + publicp = module_maybe_has_cmi_p () && !header_module_p (); > > if (publicp && cxx_dialect == cxx98) > { > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc > index 9d37d255d8d..00c50e3130d 100644 > --- a/gcc/cp/tree.cc > +++ b/gcc/cp/tree.cc > @@ -2974,9 +2974,9 @@ verify_stmt_tree (tree t) > > /* Check if the type T depends on a type with no linkage and if so, > return it. If RELAXED_P then do not consider a class type declared > - within a vague-linkage function or in a module CMI to have no linkage, > - since it can still be accessed within a different TU. Remember: > - no-linkage is not the same as internal-linkage. */ > + within a vague-linkage function or in a non-header module CMI to > + have no linkage, since it can still be accessed within a different TU. > + Remember: no-linkage is not the same as internal-linkage. */ > > tree > no_linkage_check (tree t, bool relaxed_p) > @@ -3019,7 +3019,8 @@ no_linkage_check (tree t, bool relaxed_p) > { > if (relaxed_p > && TREE_PUBLIC (CP_TYPE_CONTEXT (t)) > - && module_maybe_has_cmi_p ()) > + && module_maybe_has_cmi_p () > + && !header_module_p ()) > /* This type could possibly be accessed outside this TU. */ > return NULL_TREE; > else > @@ -3037,7 +3038,9 @@ no_linkage_check (tree t, bool relaxed_p) > { > if (relaxed_p > && (vague_linkage_p (r) > - || (TREE_PUBLIC (r) && module_maybe_has_cmi_p ()))) > + || (TREE_PUBLIC (r) > + && module_maybe_has_cmi_p () > + && !header_module_p ()))) > r = CP_DECL_CONTEXT (r); > else > return t; > diff --git a/gcc/testsuite/g++.dg/modules/linkage-3.H b/gcc/testsuite/g++.dg/modules/linkage-3.H > new file mode 100644 > index 00000000000..a34ff084eaf > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/linkage-3.H > @@ -0,0 +1,25 @@ > +// { dg-additional-options "-fmodule-header" } > +// { dg-module-cmi !{} } > + > +// Like linkage-1, but for header units. > + > +// External linkage definitions must be declared as 'inline' to satisfy > +// [module.import] p6, so we don't need to care about voldemort types in > +// function definitions. However, we still care about anonymous types like > +// this: because a header unit is not a TU, it's up to each importer to export > +// the name declared here, and depending on what other anonymous types they > +// declare they could give each declaration different mangled names. > +// So we should still complain about this because in general it's not safe > +// to assume that the declaration can be provided in another TU; this is OK > +// to do by [module.import] p5. > + > +inline auto f() { > + struct A {}; > + return A{}; > +} > +decltype(f()) g(); // OK, vague linkage function > +auto x = g(); > + > +struct { int y; } s; > +decltype(s) h(); // { dg-error "used but never defined" } > +inline auto y = h(); > -- > 2.43.2 > Oops, I attached the wrong version of the test: it should be this one: diff --git a/gcc/testsuite/g++.dg/modules/linkage-3.H b/gcc/testsuite/g++.dg/modules/linkage-3.H new file mode 100644 index 00000000000..3e1b4bad057 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/linkage-3.H @@ -0,0 +1,26 @@ +// { dg-additional-options "-fmodule-header -Wno-error=c++20-extensions" } +// { dg-module-cmi !{} } + +// Like linkage-1, but for header units. + +// External linkage definitions must be declared as 'inline' to satisfy +// [module.import] p6, so we don't need to care about voldemort types in +// function definitions. However, we still care about anonymous types like +// this: because a header unit is not a TU, it's up to each importer to export +// the name declared here, and depending on what other anonymous types they +// declare they could give each declaration different mangled names. +// So we should still complain about this because in general it's not safe +// to assume that the declaration can be provided in another TU; this is OK +// to do by [module.import] p5. + +// OK, vague linkage function +inline auto f() { + struct A {}; + return A{}; +} +decltype(f()) g(); // { dg-warning "used but not defined" "" { target c++17_down } } +auto x = g(); + +struct { int y; } s; +decltype(s) h(); // { dg-error "used but never defined" } +inline auto y = h();