On Mon, May 20, 2024 at 06:00:09PM -0400, Jason Merrill wrote:
> On 5/17/24 02:14, 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.
>
> Since namespace-scope anonymous types are TU-local, we don't need to support
> that for proper modules, but it's not clear to me that we don't need to
> support it for header units.
>
> OTOH, https://eel.is/c++draft/module#import-5.3 allows c.C to import a
> different header unit than b.C, in which case the type is different and x
> violates the odr.
>
Right; I think at this stage I don't know how to support this for header
units (and even for module interface units it doesn't actually work;
more on this below), so I think saying that this is actually an ODR
violation is OK.
> > 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?
>
> If we need that condition, it should be its own predicate rather than
> expecting callers to do that combined check.
>
> But it's not clear to me how this is different from a type in the GMF of a
> named module, which is exactly the maybe_has_cmi case; there we could again
> see a different version of the type if another TU includes the header.
>
> Jason
>
This made me go back and double-check for named modules and it actually
does fail as well; the following sample ICEs, even:
export module M;
struct {} s;
int h(decltype(s));
int x = h(s); // ICE in write_unnamed_type_name, cp/mangle.cc:1806
So I think maybe the way to go here is to just not treat unnamed types
as something that could possibly be accessed from a different TU, like
the below. Then we don't need to do the special handling for header
units, since as you say, they're not materially different anyway.
Thoughts?
(And I've moved the original change to 'module_has_cmi_p' to a separate
patch given it's somewhat unrelated now.)
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk (and
maybe 14.2)?
-- >8 --
In r14-9530 we relaxed "depending on type with no-linkage" errors for
declarations that could actually be accessed from different TUs anyway.
However, this also enabled it for unnamed types, which never work.
In a normal module interface, an unnamed type is TU-local by
[basic.link] p15.2, and so cannot be exposed or the program is
ill-formed. We don't yet implement this checking but we should assume
that we will later; currently supporting this actually causes ICEs when
attempting to create the mangled name in some situations.
For a header unit, by [module.import] p5.3 it is unspecified whether two
TUs importing a header unit providing such a declaration are importing
the same header unit. In this case, we would require name mangling
changes to somehow allow the (anonymous) type exported by such a header
unit to correspond across different TUs in the presence of other
anonymous declarations, so for this patch just assume that this case
would be an ODR violation instead.
gcc/cp/ChangeLog:
* tree.cc (no_linkage_check): Anonymous types can't be accessed
in a different TU.
gcc/testsuite/ChangeLog:
* g++.dg/modules/linkage-1_a.C: Remove anonymous type test.
* g++.dg/modules/linkage-1_b.C: Likewise.
* g++.dg/modules/linkage-1_c.C: Likewise.
* g++.dg/modules/linkage-2.C: Add note about anonymous types.
Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
---
gcc/cp/tree.cc | 10 +---------
gcc/testsuite/g++.dg/modules/linkage-1_a.C | 4 ----
gcc/testsuite/g++.dg/modules/linkage-1_b.C | 1 -
gcc/testsuite/g++.dg/modules/linkage-1_c.C | 1 -
gcc/testsuite/g++.dg/modules/linkage-2.C | 9 +++++----
5 files changed, 6 insertions(+), 19 deletions(-)
diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index 9d37d255d8d..e2d0d3229c1 100644
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -3016,15 +3016,7 @@ no_linkage_check (tree t, bool relaxed_p)
/* Only treat unnamed types as having no linkage if they're at
namespace scope. This is core issue 966. */
if (TYPE_UNNAMED_P (t) && TYPE_NAMESPACE_SCOPE_P (t))
- {
- if (relaxed_p
- && TREE_PUBLIC (CP_TYPE_CONTEXT (t))
- && module_maybe_has_cmi_p ())
- /* This type could possibly be accessed outside this TU. */
- return NULL_TREE;
- else
- return t;
- }
+ return t;
for (r = CP_TYPE_CONTEXT (t); ; )
{
diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_a.C
b/gcc/testsuite/g++.dg/modules/linkage-1_a.C
index 750e31ff347..1d81312e94f 100644
--- a/gcc/testsuite/g++.dg/modules/linkage-1_a.C
+++ b/gcc/testsuite/g++.dg/modules/linkage-1_a.C
@@ -9,7 +9,3 @@ auto f() {
}
decltype(f()) g(); // { dg-warning "used but not defined" "" { target
c++17_down } }
export auto x = g();
-
-struct {} s;
-decltype(s) h(); // { dg-warning "used but not defined" "" { target
c++17_down } }
-export auto y = h();
diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_b.C
b/gcc/testsuite/g++.dg/modules/linkage-1_b.C
index f23962d76b7..5bc0d1b888d 100644
--- a/gcc/testsuite/g++.dg/modules/linkage-1_b.C
+++ b/gcc/testsuite/g++.dg/modules/linkage-1_b.C
@@ -3,4 +3,3 @@
module M;
decltype(f()) g() { return {}; }
-decltype(s) h() { return {}; }
diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_c.C
b/gcc/testsuite/g++.dg/modules/linkage-1_c.C
index f1406b99032..9ff1491b67e 100644
--- a/gcc/testsuite/g++.dg/modules/linkage-1_c.C
+++ b/gcc/testsuite/g++.dg/modules/linkage-1_c.C
@@ -5,5 +5,4 @@ import M;
int main() {
auto a = x;
- auto b = y;
}
diff --git a/gcc/testsuite/g++.dg/modules/linkage-2.C
b/gcc/testsuite/g++.dg/modules/linkage-2.C
index eb4d7b051af..f69bd7ff728 100644
--- a/gcc/testsuite/g++.dg/modules/linkage-2.C
+++ b/gcc/testsuite/g++.dg/modules/linkage-2.C
@@ -13,14 +13,15 @@ namespace {
return A{};
}
decltype(f()) g(); // { dg-error "used but never defined" }
-
- struct {} s;
- decltype(s) h(); // { dg-error "used but never defined" }
}
export void use() {
g();
- h();
}
+// Additionally, unnamed types have no linkage but are also TU-local,
+// and thus cannot be in a module interface unit.
+// We don't yet implement this checking however.
+struct {} s; // { dg-error "TU-local" "" { xfail *-*-* } }
+
// { dg-prune-output "not writing module" }
--
2.43.2
