[PATCH] c++/modules: Remember that header units have CMIs

[PATCH] c++/modules: Remember that header units have CMIs

[Nathaniel Shead]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

This appears to be an oversight in the definition of module_has_cmi_p;
this comes up transitively in other functions used for e.g. determining
whether a name could potentially be accessed in a different translation
unit.

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.

gcc/cp/ChangeLog:

	* cp-tree.h (module_has_cmi_p): Also true for header units.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/linkage-3_a.H: New test.
	* g++.dg/modules/linkage-3_b.C: New test.
	* g++.dg/modules/linkage-3_c.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                           |  2 +-
 gcc/testsuite/g++.dg/modules/linkage-3_a.H | 19 +++++++++++++++++++
 gcc/testsuite/g++.dg/modules/linkage-3_b.C |  9 +++++++++
 gcc/testsuite/g++.dg/modules/linkage-3_c.C | 10 ++++++++++
 4 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_c.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index db098c32f2d..609904918ba 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7379,7 +7379,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/testsuite/g++.dg/modules/linkage-3_a.H b/gcc/testsuite/g++.dg/modules/linkage-3_a.H
new file mode 100644
index 00000000000..1e0ebd927e2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/linkage-3_a.H
@@ -0,0 +1,19 @@
+// { 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.
+
+// Strictly speaking this is not required to be supported:
+// [module.import] p5 says that when two different TUs import header-names
+// identifying the same header or source file, it is unspecified whether
+// they import the same header unit, and thus 's' could be a different entity
+// in each TU.  But with out current implementation this seems to reasonable to
+// allow (and it does currently work).
+
+struct { int y; } s;
+decltype(s) f();  // { dg-warning "used but not defined" "" { target c++17_down } }
+inline auto x = f();
diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_b.C b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
new file mode 100644
index 00000000000..935ef6150ec
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
@@ -0,0 +1,9 @@
+// { dg-additional-options "-fmodules-ts" }
+
+struct {} unrelated;
+
+import "linkage-3_a.H";
+
+decltype(s) f() {
+  return { 123 };
+}
diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_c.C b/gcc/testsuite/g++.dg/modules/linkage-3_c.C
new file mode 100644
index 00000000000..be527ff552d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/linkage-3_c.C
@@ -0,0 +1,10 @@
+// { dg-module-do run }
+// { dg-additional-options "-fmodules-ts" }
+
+import "linkage-3_a.H";
+
+int main() {
+  auto a = x.y;
+  if (a != 123)
+    __builtin_abort();
+}
-- 
2.43.2

Re: [PATCH] c++/modules: Remember that header units have CMIs

[Jason Merrill]

On 5/12/24 22:58, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

OK.

> -- >8 --
> 
> This appears to be an oversight in the definition of module_has_cmi_p;
> this comes up transitively in other functions used for e.g. determining
> whether a name could potentially be accessed in a different translation
> unit.
> 
> 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.
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (module_has_cmi_p): Also true for header units.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/linkage-3_a.H: New test.
> 	* g++.dg/modules/linkage-3_b.C: New test.
> 	* g++.dg/modules/linkage-3_c.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/cp-tree.h                           |  2 +-
>   gcc/testsuite/g++.dg/modules/linkage-3_a.H | 19 +++++++++++++++++++
>   gcc/testsuite/g++.dg/modules/linkage-3_b.C |  9 +++++++++
>   gcc/testsuite/g++.dg/modules/linkage-3_c.C | 10 ++++++++++
>   4 files changed, 39 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_a.H
>   create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_b.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_c.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index db098c32f2d..609904918ba 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7379,7 +7379,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/testsuite/g++.dg/modules/linkage-3_a.H b/gcc/testsuite/g++.dg/modules/linkage-3_a.H
> new file mode 100644
> index 00000000000..1e0ebd927e2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/linkage-3_a.H
> @@ -0,0 +1,19 @@
> +// { 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.
> +
> +// Strictly speaking this is not required to be supported:
> +// [module.import] p5 says that when two different TUs import header-names
> +// identifying the same header or source file, it is unspecified whether
> +// they import the same header unit, and thus 's' could be a different entity
> +// in each TU.  But with out current implementation this seems to reasonable to
> +// allow (and it does currently work).
> +
> +struct { int y; } s;
> +decltype(s) f();  // { dg-warning "used but not defined" "" { target c++17_down } }
> +inline auto x = f();
> diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_b.C b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
> new file mode 100644
> index 00000000000..935ef6150ec
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
> @@ -0,0 +1,9 @@
> +// { dg-additional-options "-fmodules-ts" }
> +
> +struct {} unrelated;
> +
> +import "linkage-3_a.H";
> +
> +decltype(s) f() {
> +  return { 123 };
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_c.C b/gcc/testsuite/g++.dg/modules/linkage-3_c.C
> new file mode 100644
> index 00000000000..be527ff552d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/linkage-3_c.C
> @@ -0,0 +1,10 @@
> +// { dg-module-do run }
> +// { dg-additional-options "-fmodules-ts" }
> +
> +import "linkage-3_a.H";
> +
> +int main() {
> +  auto a = x.y;
> +  if (a != 123)
> +    __builtin_abort();
> +}

[PATCH v2] c++/modules: Remember that header units have CMIs

[Nathaniel Shead]

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 <nathanieloshead@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

Re: [PATCH v2] c++/modules: Remember that header units have CMIs

[Nathaniel Shead]

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 <nathanieloshead@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();

Re: [PATCH v2] c++/modules: Remember that header units have CMIs

[Jason Merrill]

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.

> 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

[PATCH 1/2] c++/modules: Fix treatment of unnamed types

[Nathaniel Shead]

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 <nathanieloshead@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

[PATCH 2/2] c++/modules: Remember that header units have CMIs

[Nathaniel Shead]

And here's that patch.  As far as I can tell there should be no visible
change anymore, so there aren't any testcases.

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >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, allowing us to do additional work only when we know we need it.

gcc/cp/ChangeLog:

	* cp-tree.h (module_has_cmi_p): Also include header units.
	(module_maybe_has_cmi_p): Update comment.
	* module.cc (set_defining_module): Only need to track
	declarations for later exporting if the module may have a CMI.
	* name-lookup.cc (pushdecl): Likewise.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h      | 7 +++----
 gcc/cp/module.cc      | 2 +-
 gcc/cp/name-lookup.cc | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index ba9e848c177..9472759d3c8 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; }
@@ -7393,9 +7393,8 @@ inline bool named_module_purview_p ()
 inline bool named_module_attach_p ()
 { return named_module_p () && module_attach_p (); }
 
-/* We don't know if this TU will have a CMI while parsing the GMF,
-   so tentatively assume that it might, for the purpose of determining
-   whether no-linkage decls could be used by an importer.  */
+/* Like module_has_cmi_p, but tentatively assumes that this TU may have a
+   CMI if we haven't seen the module-declaration yet.  */
 inline bool module_maybe_has_cmi_p ()
 { return module_has_cmi_p () || (named_module_p () && !module_purview_p ()); }
 
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 520dd710549..8639ed6f1a2 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -19216,7 +19216,7 @@ set_defining_module (tree decl)
   gcc_checking_assert (!DECL_LANG_SPECIFIC (decl)
 		       || !DECL_MODULE_IMPORT_P (decl));
 
-  if (module_p ())
+  if (module_maybe_has_cmi_p ())
     {
       /* We need to track all declarations within a module, not just those
 	 in the module purview, because we don't necessarily know yet if
diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index 78f08acffaa..f1f8c19feb1 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -4103,7 +4103,7 @@ pushdecl (tree decl, bool hiding)
 
 	  if (level->kind == sk_namespace
 	      && TREE_PUBLIC (level->this_entity)
-	      && module_p ())
+	      && module_maybe_has_cmi_p ())
 	    maybe_record_mergeable_decl (slot, name, decl);
 	}
     }
-- 
2.43.2

Re: [PATCH 1/2] c++/modules: Fix treatment of unnamed types

[Jason Merrill]

On 5/23/24 09:27, Nathaniel Shead wrote:
> 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?

Sounds good.

> (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.
> 
> 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();

The above changes seem undesirable; we should still give that error.

> +// 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 *-*-* } }

The comment should clarify that the problem is the non-TU-local variable 
's' exposing the TU-local type.  If s is also TU-local (as the one in 
the anonymous namespace above) it's fine as long as s is not itself exposed.

Why doesn't the exposure code catch this?  I guess the code in 
make_dependency to mark internal types needs to handle anonymous types 
as well.

Jason

Re: [PATCH 2/2] c++/modules: Remember that header units have CMIs

[Jason Merrill]

On 5/23/24 09:29, Nathaniel Shead wrote:
> And here's that patch.  As far as I can tell there should be no visible
> change anymore, so there aren't any testcases.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

OK.

> -- >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, allowing us to do additional work only when we know we need it.
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (module_has_cmi_p): Also include header units.
> 	(module_maybe_has_cmi_p): Update comment.
> 	* module.cc (set_defining_module): Only need to track
> 	declarations for later exporting if the module may have a CMI.
> 	* name-lookup.cc (pushdecl): Likewise.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/cp-tree.h      | 7 +++----
>   gcc/cp/module.cc      | 2 +-
>   gcc/cp/name-lookup.cc | 2 +-
>   3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index ba9e848c177..9472759d3c8 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; }
> @@ -7393,9 +7393,8 @@ inline bool named_module_purview_p ()
>   inline bool named_module_attach_p ()
>   { return named_module_p () && module_attach_p (); }
>   
> -/* We don't know if this TU will have a CMI while parsing the GMF,
> -   so tentatively assume that it might, for the purpose of determining
> -   whether no-linkage decls could be used by an importer.  */
> +/* Like module_has_cmi_p, but tentatively assumes that this TU may have a
> +   CMI if we haven't seen the module-declaration yet.  */
>   inline bool module_maybe_has_cmi_p ()
>   { return module_has_cmi_p () || (named_module_p () && !module_purview_p ()); }
>   
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 520dd710549..8639ed6f1a2 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -19216,7 +19216,7 @@ set_defining_module (tree decl)
>     gcc_checking_assert (!DECL_LANG_SPECIFIC (decl)
>   		       || !DECL_MODULE_IMPORT_P (decl));
>   
> -  if (module_p ())
> +  if (module_maybe_has_cmi_p ())
>       {
>         /* We need to track all declarations within a module, not just those
>   	 in the module purview, because we don't necessarily know yet if
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index 78f08acffaa..f1f8c19feb1 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -4103,7 +4103,7 @@ pushdecl (tree decl, bool hiding)
>   
>   	  if (level->kind == sk_namespace
>   	      && TREE_PUBLIC (level->this_entity)
> -	      && module_p ())
> +	      && module_maybe_has_cmi_p ())
>   	    maybe_record_mergeable_decl (slot, name, decl);
>   	}
>       }

Re: [PATCH 1/2] c++/modules: Fix treatment of unnamed types

[Nathaniel Shead]

On Thu, May 23, 2024 at 03:36:48PM -0400, Jason Merrill wrote:
> On 5/23/24 09:27, Nathaniel Shead wrote:
> > 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?
> 
> Sounds good.
> 
> > (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.
> > 
> > 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();
> 
> The above changes seem undesirable; we should still give that error.
> 

Oh whoops, I don't know why I got rid of those test cases; added back in
and they still correctly error.

> > +// 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 *-*-* } }
> 
> The comment should clarify that the problem is the non-TU-local variable 's'
> exposing the TU-local type.  If s is also TU-local (as the one in the
> anonymous namespace above) it's fine as long as s is not itself exposed.
> 

Done.

> Why doesn't the exposure code catch this?  I guess the code in
> make_dependency to mark internal types needs to handle anonymous types as
> well.
> 
> Jason
> 

Yes, the current code is pretty limited in how it determines what an
exposure is: currently it just finds 'internal linkage', determined
exclusively by either being in an anonymous namespace or being marked
'static', which is not always correct.

I'd been looking into fixing up 'decl_linkage' to understand the C++11
meaning of 'internal linkage' and using that instead, and additionally
for 'export'-declarations, but I've run into a couple of issues with the
dynamic way that TREE_PUBLIC is set/unset on things and when
'decl_linkage' is actually called for other purposes, so it'll take me
some time to work that out.  (And that's just for internal linkage; it
still doesn't handle other TU-local types like this).

Regardless, here's the patch with a new version of the testcase; tested
on x86_64-pc-linux-gnu, OK for trunk/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 <nathanieloshead@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   |  6 ++++++
 5 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index 0485a618c6c..fe3f034d000 100644
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -2988,15 +2988,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..d913d6a30fc 100644
--- a/gcc/testsuite/g++.dg/modules/linkage-2.C
+++ b/gcc/testsuite/g++.dg/modules/linkage-2.C
@@ -23,4 +23,10 @@ export void use() {
   h();
 }
 
+// Additionally, unnamed types have no linkage but are also TU-local, and thus
+// cannot be exposed in a module interface unit.  The non-TU-local entity 's'
+// here is an exposure of this type, so this should be an error; we don't yet
+// implement this checking however.
+struct {} s;  // { dg-error "TU-local" "" { xfail *-*-* } }
+
 // { dg-prune-output "not writing module" }
-- 
2.43.2

Re: [PATCH 1/2] c++/modules: Fix treatment of unnamed types

[Jason Merrill]

On 5/23/24 21:27, Nathaniel Shead wrote:
> On Thu, May 23, 2024 at 03:36:48PM -0400, Jason Merrill wrote:
>> On 5/23/24 09:27, Nathaniel Shead wrote:
>>> 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?
>>
>> Sounds good.
>>
>>> (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.
>>>
>>> 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();
>>
>> The above changes seem undesirable; we should still give that error.
>>
> 
> Oh whoops, I don't know why I got rid of those test cases; added back in
> and they still correctly error.
> 
>>> +// 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 *-*-* } }
>>
>> The comment should clarify that the problem is the non-TU-local variable 's'
>> exposing the TU-local type.  If s is also TU-local (as the one in the
>> anonymous namespace above) it's fine as long as s is not itself exposed.
>>
> 
> Done.
> 
>> Why doesn't the exposure code catch this?  I guess the code in
>> make_dependency to mark internal types needs to handle anonymous types as
>> well.
>>
>> Jason
>>
> 
> Yes, the current code is pretty limited in how it determines what an
> exposure is: currently it just finds 'internal linkage', determined
> exclusively by either being in an anonymous namespace or being marked
> 'static', which is not always correct.
> 
> I'd been looking into fixing up 'decl_linkage' to understand the C++11
> meaning of 'internal linkage' and using that instead, and additionally
> for 'export'-declarations, but I've run into a couple of issues with the
> dynamic way that TREE_PUBLIC is set/unset on things and when
> 'decl_linkage' is actually called for other purposes, so it'll take me
> some time to work that out.  (And that's just for internal linkage; it
> still doesn't handle other TU-local types like this).
> 
> Regardless, here's the patch with a new version of the testcase; tested
> on x86_64-pc-linux-gnu, OK for trunk/14.2?

OK.

> -- >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 <nathanieloshead@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   |  6 ++++++
>   5 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> index 0485a618c6c..fe3f034d000 100644
> --- a/gcc/cp/tree.cc
> +++ b/gcc/cp/tree.cc
> @@ -2988,15 +2988,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..d913d6a30fc 100644
> --- a/gcc/testsuite/g++.dg/modules/linkage-2.C
> +++ b/gcc/testsuite/g++.dg/modules/linkage-2.C
> @@ -23,4 +23,10 @@ export void use() {
>     h();
>   }
>   
> +// Additionally, unnamed types have no linkage but are also TU-local, and thus
> +// cannot be exposed in a module interface unit.  The non-TU-local entity 's'
> +// here is an exposure of this type, so this should be an error; we don't yet
> +// implement this checking however.
> +struct {} s;  // { dg-error "TU-local" "" { xfail *-*-* } }
> +
>   // { dg-prune-output "not writing module" }