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 <nathanielosh...@gmail.com>
> ---
> 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();
