Ping for this fix for a P1 issue.
On Sun, Feb 09, 2025 at 10:38:24PM +1100, Nathaniel Shead wrote:
> On Sun, Feb 09, 2025 at 01:16:00AM +1100, Nathaniel Shead wrote:
> > Tested on x86_64-pc-linux-gnu, OK for trunk if full bootstrap + regtest
> > passes?
> >
> > -- >8 --
> >
> > There are two issues with no-linkage decls (e.g. explicit type aliases)
> > in unnamed namespaces that this patch fixes.
> >
> > Firstly, we don't currently handle exporting no-linkage decls in unnamed
> > namespaces. This should be ill-formed in [module.export], since having
> > an exported declaration within a namespace-definition makes the
> > namespace definition exported (p2), but an unnamed namespace has
> > internal linkage thus violating p3.
> >
> > Secondly, by the standard it appears to be possible to emit unnamed
> > namespaces from named modules in certain scenarios. This patch makes
> > the adjustments needed to ensure we don't error in this case.
> >
>
> One thing to note with this is that it means that the following sample:
>
> export module M;
> namespace {
> struct Internal {};
> using Alias = Internal;
> }
>
> will still error:
>
> test.cpp:4:9: error: ‘using {anonymous}::Alias = struct
> {anonymous}::Internal’ exposes TU-local entity ‘struct {anonymous}::Internal’
> 4 | using Alias = Internal;
> | ^~~~~
> test.cpp:3:10: note: ‘struct {anonymous}::Internal’ declared with internal
> linkage
> 3 | struct Internal {};
> | ^~~~~~~~
>
> https://eel.is/c++draft/basic.link#17 is the relevant paragraph here,
> but I'm not 100% sure what it says about this example; I suppose that
> given Alias isn't really an entity maybe this should be OK? But either
> way we definitely don't want to emit this alias.
>
> Maybe the correct approach here is to mark an explicit type alias
> TU-local iff the type it refers to is itself TU-local? So something
> like this on top of this patch:
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 21251f98a35..7aa7f93df57 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -13331,6 +13331,8 @@ bool
> depset::hash::is_tu_local_entity (tree decl, bool explain/*=false*/)
> {
> gcc_checking_assert (DECL_P (decl));
> + location_t loc = DECL_SOURCE_LOCATION (decl);
> + tree type = TREE_TYPE (decl);
>
> /* Only types, functions, variables, and template (specialisations)
> can be TU-local. */
> @@ -13340,15 +13342,22 @@ depset::hash::is_tu_local_entity (tree decl, bool
> explain/*=false*/)
> && TREE_CODE (decl) != TEMPLATE_DECL)
> return false;
>
> - /* An explicit type alias is not an entity, and so is never TU-local.
> - Neither are the built-in declarations of 'int' and such. */
> + /* An explicit type alias is not an entity, but rather inherits
> + whether it's TU-local from the type that it aliases.
> + The built-in declarations of 'int' and such are never TU-local. */
> if (TREE_CODE (decl) == TYPE_DECL
> && !DECL_SELF_REFERENCE_P (decl)
> && !DECL_IMPLICIT_TYPEDEF_P (decl))
> - return false;
> -
> - location_t loc = DECL_SOURCE_LOCATION (decl);
> - tree type = TREE_TYPE (decl);
> + {
> + if (tree orig = DECL_ORIGINAL_TYPE (decl))
> + {
> + if (explain)
> + inform (loc, "%qD is an alias of TU-local type %qT", decl, orig);
> + return is_tu_local_entity (TYPE_NAME (orig), explain);
> + }
> + else
> + return false;
> + }
>
> /* Check specializations first for slightly better explanations. */
> int use_tpl = -1;
>
> Thoughts?
>
> > PR c++/118799
> >
> > gcc/cp/ChangeLog:
> >
> > * module.cc (depset::hash::is_tu_local_entity): Only types,
> > functions, variables, and template (specialisations) can be
> > TU-local.
> > (module_state::write_namespaces): Allow unnamed namespaces in
> > named modules.
> > (check_module_decl_linkage): Error for all exported declarations
> > in an unnamed namespace.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/modules/export-6.C: Adjust error message, add check for
> > no-linkage decls in namespace.
> > * g++.dg/modules/internal-4_b.C: Allow exposing a namespace with
> > internal linkage.
> > * g++.dg/modules/using-30_a.C: New test.
> > * g++.dg/modules/using-30_b.C: New test.
> > * g++.dg/modules/using-30_c.C: New test.
> >
> > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > ---
> > gcc/cp/module.cc | 35 ++++++++++++++++-----
> > gcc/testsuite/g++.dg/modules/export-6.C | 33 ++++++++++---------
> > gcc/testsuite/g++.dg/modules/internal-4_b.C | 4 +--
> > gcc/testsuite/g++.dg/modules/using-30_a.C | 9 ++++++
> > gcc/testsuite/g++.dg/modules/using-30_b.C | 6 ++++
> > gcc/testsuite/g++.dg/modules/using-30_c.C | 13 ++++++++
> > 6 files changed, 77 insertions(+), 23 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/using-30_a.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/using-30_b.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/using-30_c.C
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index 59716e1873e..21251f98a35 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -13332,6 +13332,14 @@ depset::hash::is_tu_local_entity (tree decl, bool
> > explain/*=false*/)
> > {
> > gcc_checking_assert (DECL_P (decl));
> >
> > + /* Only types, functions, variables, and template (specialisations)
> > + can be TU-local. */
> > + if (TREE_CODE (decl) != TYPE_DECL
> > + && TREE_CODE (decl) != FUNCTION_DECL
> > + && TREE_CODE (decl) != VAR_DECL
> > + && TREE_CODE (decl) != TEMPLATE_DECL)
> > + return false;
> > +
> > /* An explicit type alias is not an entity, and so is never TU-local.
> > Neither are the built-in declarations of 'int' and such. */
> > if (TREE_CODE (decl) == TYPE_DECL
> > @@ -16433,8 +16441,9 @@ module_state::write_namespaces (elf_out *to,
> > vec<depset *> spaces,
> > depset *b = spaces[ix];
> > tree ns = b->get_entity ();
> >
> > + /* This could be an anonymous namespace even for a named module,
> > + since we can still emit no-linkage decls. */
> > gcc_checking_assert (TREE_CODE (ns) == NAMESPACE_DECL);
> > - gcc_checking_assert (TREE_PUBLIC (ns) || header_module_p ());
> >
> > unsigned flags = 0;
> > if (TREE_PUBLIC (ns))
> > @@ -20394,13 +20403,25 @@ check_module_decl_linkage (tree decl)
> > /* An internal-linkage declaration cannot be generally be exported.
> > But it's OK to export any declaration from a header unit, including
> > internal linkage declarations. */
> > - if (!header_module_p ()
> > - && DECL_MODULE_EXPORT_P (decl)
> > - && decl_linkage (decl) == lk_internal)
> > + if (!header_module_p () && DECL_MODULE_EXPORT_P (decl))
> > {
> > - error_at (DECL_SOURCE_LOCATION (decl),
> > - "exporting declaration %qD with internal linkage", decl);
> > - DECL_MODULE_EXPORT_P (decl) = false;
> > + /* Let's additionally treat any exported declaration within an
> > + internal namespace as exporting a declaration with internal
> > + linkage, as this would implicitly also export the internal
> > + linkage namespace. */
> > + if (decl_internal_context_p (decl))
> > + {
> > + error_at (DECL_SOURCE_LOCATION (decl),
> > + "exporting declaration %qD declared in unnamed namespace",
> > + decl);
> > + DECL_MODULE_EXPORT_P (decl) = false;
> > + }
> > + else if (decl_linkage (decl) == lk_internal)
> > + {
> > + error_at (DECL_SOURCE_LOCATION (decl),
> > + "exporting declaration %qD with internal linkage", decl);
> > + DECL_MODULE_EXPORT_P (decl) = false;
> > + }
> > }
> > }
> >
> > diff --git a/gcc/testsuite/g++.dg/modules/export-6.C
> > b/gcc/testsuite/g++.dg/modules/export-6.C
> > index 460cdf08ea9..af54e5fbe87 100644
> > --- a/gcc/testsuite/g++.dg/modules/export-6.C
> > +++ b/gcc/testsuite/g++.dg/modules/export-6.C
> > @@ -16,27 +16,32 @@ export static auto [d] = S{}; // { dg-error "internal
> > linkage" "" { target c++2
> > #endif
> >
> > namespace {
> > - export int y = 456; // { dg-error "internal linkage" }
> > - export void h(); // { dg-error "internal linkage" }
> > - export void i() {} // { dg-error "internal linkage" }
> > - export template <typename T> void v(); // { dg-error "internal linkage" }
> > - export template <typename T> void w() {} // { dg-error "internal
> > linkage" }
> > - export auto [e] = S{}; // { dg-error "internal linkage" }
> > + export int y = 456; // { dg-error "exporting" }
> > + export void h(); // { dg-error "exporting" }
> > + export void i() {} // { dg-error "exporting" }
> > + export template <typename T> void v(); // { dg-error "exporting" }
> > + export template <typename T> void w() {} // { dg-error "exporting" }
> > + export auto [e] = S{}; // { dg-error "exporting" }
> >
> > - export namespace ns {} // { dg-error "internal linkage" }
> > - export namespace alias = global; // { dg-error "internal linkage" }
> > + export namespace ns {} // { dg-error "exporting" }
> > + export namespace alias = global; // { dg-error "exporting" }
> >
> > - export struct A {}; // { dg-error "internal linkage" }
> > - export template <typename T> struct B {}; // { dg-error "internal
> > linkage" }
> > + export struct A {}; // { dg-error "exporting" }
> > + export template <typename T> struct B {}; // { dg-error "exporting" }
> >
> > - export enum E {}; // { dg-error "internal linkage" }
> > - export enum class F {}; // { dg-error "internal linkage" }
> > + export enum E {}; // { dg-error "exporting" }
> > + export enum class F {}; // { dg-error "exporting" }
> >
> > - export template <typename T> using U = int; // { dg-error "internal
> > linkage" }
> > + export template <typename T> using U = int; // { dg-error "exporting" }
> >
> > #if __cplusplus >= 202002L
> > - export template <typename T> concept C = true; // { dg-error "internal
> > linkage" "" { target c++20 } }
> > + export template <typename T> concept C = true; // { dg-error
> > "exporting" "" { target c++20 } }
> > #endif
> > +
> > + // Also complain about exporting no-linkage decls in an unnamed namespace
> > + export typedef int T; // { dg-error "exporting" }
> > + export typedef struct {} *PC; // { dg-error "exporting" }
> > + export using V = int; // { dg-error "exporting" }
> > }
> >
> > export namespace {} // { dg-error "exporting unnamed namespace" }
> > diff --git a/gcc/testsuite/g++.dg/modules/internal-4_b.C
> > b/gcc/testsuite/g++.dg/modules/internal-4_b.C
> > index 81fc65a69bd..7e4fbfeb1fb 100644
> > --- a/gcc/testsuite/g++.dg/modules/internal-4_b.C
> > +++ b/gcc/testsuite/g++.dg/modules/internal-4_b.C
> > @@ -32,8 +32,8 @@ inline void expose_header_decl() { // { dg-error
> > "exposes TU-local entity" }
> > header_f();
> > }
> >
> > -// We additionally consider a namespace with internal linkage as TU-local
> > -namespace expose_ns = internal_ns; // { dg-error "exposes TU-local
> > entity" }
> > +// A namespace with internal linkage is not inherently an exposure.
> > +namespace expose_ns = internal_ns; // { dg-bogus "exposes TU-local
> > entity" }
> >
> > // But we don't consider a weakref as being TU-local, despite being
> > // marked static; this is to support uses of weakrefs in header files
> > diff --git a/gcc/testsuite/g++.dg/modules/using-30_a.C
> > b/gcc/testsuite/g++.dg/modules/using-30_a.C
> > new file mode 100644
> > index 00000000000..fc6a67f2bae
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/using-30_a.C
> > @@ -0,0 +1,9 @@
> > +// { dg-additional-options "-fmodules" }
> > +// { dg-module-cmi M }
> > +
> > +export module M;
> > +
> > +namespace {
> > + using A = int;
> > + typedef int B;
> > +}
> > diff --git a/gcc/testsuite/g++.dg/modules/using-30_b.C
> > b/gcc/testsuite/g++.dg/modules/using-30_b.C
> > new file mode 100644
> > index 00000000000..ba06c9de792
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/using-30_b.C
> > @@ -0,0 +1,6 @@
> > +// { dg-additional-options "-fmodules" }
> > +
> > +module M;
> > +
> > +A x;
> > +B y;
> > diff --git a/gcc/testsuite/g++.dg/modules/using-30_c.C
> > b/gcc/testsuite/g++.dg/modules/using-30_c.C
> > new file mode 100644
> > index 00000000000..34dc6f2c98d
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/using-30_c.C
> > @@ -0,0 +1,13 @@
> > +// { dg-additional-options "-fmodules" }
> > +// The different declarations in the anonymous namespace shouldn't clash
> > with
> > +// those in M.
> > +
> > +namespace {
> > + using A = double;
> > + typedef double B;
> > +}
> > +import M;
> > +int main() {
> > + A a = 1.0;
> > + B b = 2.0;
> > +}
> > --
> > 2.47.0
> >
