From 99c2ed7407154164c25b4f12e82af77c67cde3a3 Mon Sep 17 00:00:00 2001 From: Nathaniel Shead Date: Thu, 30 Oct 2025 23:13:21 +1100 Subject: [PATCH] c++/modules: Allow ignoring some TU-local exposure errors in GMF [PR121574] A frequent issue with migrating to C++20 modules has been dealing with third-party libraries with internal functions or data. This causes GCC to currently refuse to build the module if any references to these internal-linkage declarations escape into the module CMI. This can seem needlessly hostile, however, especially since we have the capabilities to support this (to a degree) from header units, albeit with the inherent ODR issues associated with their use. In aid of this, this patch demotes the error to a pedwarn in various scenarios, by treating some declarations as not being TU-local even if they otherwise would have been. Effort has been made to not alter semantics of valid programs, and to continue to diagnose cases that the standard says we must. In particular, any code in the module purview is still a hard error, due to the inherent issues with exposing TU-local entities, and the lack of any migration requirements. Because this patch is just to assist migration, we only deal with the simplest (yet most common) cases: namespace scope functions and variables. Types are hard to handle neatly as we risk getting thousands of unhelpful warnings as we continue to walk the type body and find new TU-local entities to complain about. Templates are also tricky because it's hard to tell if an instantiation that occurred in the module purview only refers to global module entities or if it's inadvertantly exposing a purview entity as well. Neither of these are likely to occur frequently in third-party code; if need be, this can be relaxed later as well. Similarly, even in the GMF a constexpr variable with a TU-local value will not be usable in constant expressions in the importer, and since we cannot easily warn about this from the importer we continue to make this an error in the module interface. PR c++/121574 gcc/c-family/ChangeLog: * c.opt: New warning '-Wexpose-global-module-tu-local'. * c.opt.urls: Regenerate. gcc/ChangeLog: * doc/invoke.texi: Document '-Wexpose-global-module-tu-local'. gcc/cp/ChangeLog: * module.cc (depset::disc_bits): Replace 'DB_REFS_TU_LOCAL_BIT' and 'DB_EXPOSURE_BIT' with new four flags 'DB_{REF,EXPOSE}_{GLOBAL,PURVIEW}_BIT'. (depset::is_tu_local): Support checking either for only purview TU-local entities or any entity described TU-local by standard. (depset::refs_tu_local): Likewise. (depset::is_exposure): Likewise. (depset::hash::make_dependency): A constant initialized to a TU-local variable is always considered a purview exposure. (is_exposure_of_member_type): Adjust sanity checks to handle if we ever relax requirements for TU-local types. (depset::hash::add_dependency): Differentiate referencing purview or GMF TU-local entities. (depset::hash::diagnose_bad_internal_ref): New function. (depset::hash::diagnose_template_names_tu_local): New function. (depset::hash::finalize_dependencies): Handle new warnings that might be needed for GMF TU-local entities. gcc/testsuite/ChangeLog: * g++.dg/modules/internal-17_a.C: New test. * g++.dg/modules/internal-17_b.C: New test. Signed-off-by: Nathaniel Shead Reviewed-by: Jason Merrill --- gcc/c-family/c.opt | 4 + gcc/c-family/c.opt.urls | 3 + gcc/cp/module.cc | 286 ++++++++++++++----- gcc/doc/invoke.texi | 28 +- gcc/testsuite/g++.dg/modules/internal-17_a.C | 64 +++++ gcc/testsuite/g++.dg/modules/internal-17_b.C | 58 ++++ 6 files changed, 372 insertions(+), 71 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/internal-17_a.C create mode 100644 gcc/testsuite/g++.dg/modules/internal-17_b.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index b7ce67a5bbe..85dc3d82004 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -765,6 +765,10 @@ Wexternal-tu-local C++ ObjC++ Var(warn_tu_local) Warning Init(1) Warn about naming a TU-local entity declared in another translation unit. +Wexpose-global-module-tu-local +C++ ObjC++ Var(warn_expose_global_module_tu_local) Init(1) Warning +Warn when a module exposes a TU-local entity from the global module fragment. + Wextra C ObjC C++ ObjC++ Warning ; in common.opt diff --git a/gcc/c-family/c.opt.urls b/gcc/c-family/c.opt.urls index 399f9f8a659..55dbbcdcf22 100644 --- a/gcc/c-family/c.opt.urls +++ b/gcc/c-family/c.opt.urls @@ -379,6 +379,9 @@ UrlSuffix(gcc/Warning-Options.html#index-Wexpansion-to-defined) Wexternal-tu-local UrlSuffix(gcc/C_002b_002b-Dialect-Options.html#index-Wexternal-tu-local) +Wexpose-global-module-tu-local +UrlSuffix(gcc/C_002b_002b-Dialect-Options.html#index-Wexpose-global-module-tu-local) + Wextra UrlSuffix(gcc/Warning-Options.html#index-Wextra) LangUrlSuffix_D(gdc/Warnings.html#index-Wextra) LangUrlSuffix_Fortran(gfortran/Error-and-Warning-Options.html#index-Wextra) diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index e9cacf157c4..3aa6686fd86 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2365,10 +2365,11 @@ private: DB_KIND_BITS = EK_BITS, DB_DEFN_BIT = DB_KIND_BIT + DB_KIND_BITS, DB_IS_PENDING_BIT, /* Is a maybe-pending entity. */ - DB_TU_LOCAL_BIT, /* It is a TU-local entity. */ - DB_REFS_TU_LOCAL_BIT, /* Refers to a TU-local entity (but is not - necessarily an exposure.) */ - DB_EXPOSURE_BIT, /* Exposes a TU-local entity. */ + DB_TU_LOCAL_BIT, /* Is a TU-local entity. */ + DB_REF_GLOBAL_BIT, /* Refers to a GMF TU-local entity. */ + DB_REF_PURVIEW_BIT, /* Refers to a purview TU-local entity. */ + DB_EXPOSE_GLOBAL_BIT, /* Exposes a GMF TU-local entity. */ + DB_EXPOSE_PURVIEW_BIT, /* Exposes a purview TU-local entity. */ DB_IMPORTED_BIT, /* An imported entity. */ DB_UNREACHED_BIT, /* A yet-to-be reached entity. */ DB_MAYBE_RECURSIVE_BIT, /* An entity maybe in a recursive cluster. */ @@ -2458,19 +2459,40 @@ public: || (get_entity_kind () == EK_DECL && get_flag_bit ())); } + public: - bool is_tu_local () const + /* Only consider global module entities as being TU-local + when STRICT is set; otherwise, as an extension we support + emitting declarations referencing TU-local GMF entities + (and only check purview entities), to assist in migration. */ + bool is_tu_local (bool strict = false) const { - return get_flag_bit (); + /* Non-strict is only intended for migration purposes, so + for simplicity's sake we only care about whether this is + a non-purview variable or function at namespace scope; + these are the most common cases (coming from C), and + that way we don't have to care about diagnostics for + nested types and so forth. */ + tree inner = STRIP_TEMPLATE (get_entity ()); + return (get_flag_bit () + && (strict + || !VAR_OR_FUNCTION_DECL_P (inner) + || !NAMESPACE_SCOPE_P (inner) + || (DECL_LANG_SPECIFIC (inner) + && DECL_MODULE_PURVIEW_P (inner)))); } - bool refs_tu_local () const + bool refs_tu_local (bool strict = false) const { - return get_flag_bit (); + return (get_flag_bit () + || (strict && get_flag_bit ())); } - bool is_exposure () const + bool is_exposure (bool strict = false) const { - return get_flag_bit (); + return (get_flag_bit () + || (strict && get_flag_bit ())); } + +public: bool is_import () const { return get_flag_bit (); @@ -2662,6 +2684,10 @@ public: void find_dependencies (module_state *); bool finalize_dependencies (); vec connect (); + + private: + bool diagnose_bad_internal_ref (depset *dep, bool strict = false); + bool diagnose_template_names_tu_local (depset *dep, bool strict = false); }; public: @@ -14335,8 +14361,14 @@ depset::hash::make_dependency (tree decl, entity_kind ek) if (DECL_DECLARED_CONSTEXPR_P (decl) || DECL_INLINE_VAR_P (decl)) /* A constexpr variable initialized to a TU-local value, - or an inline value (PR c++/119996), is an exposure. */ - dep->set_flag_bit (); + or an inline value (PR c++/119996), is an exposure. + + For simplicity, we don't support "non-strict" TU-local + values: even if the TU-local entity we refer to in the + initialiser is in the GMF, we still won't consider this + valid in constant expressions in other TUs, and so + complain accordingly. */ + dep->set_flag_bit (); } } @@ -14426,11 +14458,13 @@ depset::hash::make_dependency (tree decl, entity_kind ek) static bool is_exposure_of_member_type (depset *source, depset *ref) { - gcc_checking_assert (source->refs_tu_local () && ref->is_tu_local ()); + gcc_checking_assert (source->refs_tu_local (/*strict=*/true) + && ref->is_tu_local (/*strict=*/true)); tree source_entity = STRIP_TEMPLATE (source->get_entity ()); tree ref_entity = STRIP_TEMPLATE (ref->get_entity ()); - if (source_entity + if (!source->is_tu_local (/*strict=*/true) + && source_entity && ref_entity && DECL_IMPLICIT_TYPEDEF_P (source_entity) && DECL_IMPLICIT_TYPEDEF_P (ref_entity) @@ -14453,11 +14487,20 @@ depset::hash::add_dependency (depset *dep) gcc_checking_assert (current && !is_key_order ()); current->deps.safe_push (dep); - if (dep->is_tu_local ()) + if (dep->is_tu_local (/*strict=*/true)) { - current->set_flag_bit (); + if (dep->is_tu_local ()) + current->set_flag_bit (); + else + current->set_flag_bit (); + if (!ignore_tu_local && !is_exposure_of_member_type (current, dep)) - current->set_flag_bit (); + { + if (dep->is_tu_local ()) + current->set_flag_bit (); + else + current->set_flag_bit (); + } } if (current->get_entity_kind () == EK_USING @@ -15342,6 +15385,128 @@ template_has_explicit_inst (tree tmpl) return false; } +/* Complain about DEP that exposes a TU-local entity. + + If STRICT, DEP only referenced entities from the GMF. Returns TRUE + if we explained anything. */ + +bool +depset::hash::diagnose_bad_internal_ref (depset *dep, bool strict) +{ + tree decl = dep->get_entity (); + + /* Don't need to walk if we're not going to be emitting + any diagnostics anyway. */ + if (strict && !warning_enabled_at (DECL_SOURCE_LOCATION (decl), + OPT_Wexpose_global_module_tu_local)) + return false; + + for (depset *rdep : dep->deps) + if (!rdep->is_binding () && rdep->is_tu_local (strict) + && !is_exposure_of_member_type (dep, rdep)) + { + // FIXME:QOI Better location information? We're + // losing, so it doesn't matter about efficiency. + tree exposed = rdep->get_entity (); + auto_diagnostic_group d; + if (strict) + { + /* Allow suppressing the warning from the point of declaration + of the otherwise-exposed decl, for cases we know that + exposures will never be 'bad'. */ + if (warning_enabled_at (DECL_SOURCE_LOCATION (exposed), + OPT_Wexpose_global_module_tu_local) + && pedwarn (DECL_SOURCE_LOCATION (decl), + OPT_Wexpose_global_module_tu_local, + "%qD exposes TU-local entity %qD", decl, exposed)) + { + bool informed = is_tu_local_entity (exposed, /*explain=*/true); + gcc_checking_assert (informed); + return true; + } + } + else + { + error_at (DECL_SOURCE_LOCATION (decl), + "%qD exposes TU-local entity %qD", decl, exposed); + bool informed = is_tu_local_entity (exposed, /*explain=*/true); + gcc_checking_assert (informed); + if (dep->is_tu_local (/*strict=*/true)) + inform (DECL_SOURCE_LOCATION (decl), + "%qD is also TU-local but has been exposed elsewhere", + decl); + return true; + } + } + + return false; +} + +/* Warn about a template DEP that references a TU-local entity. + + If STRICT, DEP only referenced entities from the GMF. Returns TRUE + if we explained anything. */ + +bool +depset::hash::diagnose_template_names_tu_local (depset *dep, bool strict) +{ + tree decl = dep->get_entity (); + + /* Don't bother walking if we know we won't be emitting anything. */ + if (!warning_enabled_at (DECL_SOURCE_LOCATION (decl), + OPT_Wtemplate_names_tu_local) + /* Only warn strictly if users haven't silenced this warning here. */ + || (strict && !warning_enabled_at (DECL_SOURCE_LOCATION (decl), + OPT_Wexpose_global_module_tu_local))) + return false; + + /* Friend decls in a class body are ignored, but this is harmless: + it should not impact any consumers. */ + if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl))) + return false; + + /* We should now only be warning about templates. */ + gcc_checking_assert + (TREE_CODE (decl) == TEMPLATE_DECL + && VAR_OR_FUNCTION_DECL_P (DECL_TEMPLATE_RESULT (decl))); + + /* Don't warn if we've seen any explicit instantiation definitions, + the intent might be for importers to only use those. */ + if (template_has_explicit_inst (decl)) + return false; + + for (depset *rdep : dep->deps) + if (!rdep->is_binding () && rdep->is_tu_local (strict)) + { + tree ref = rdep->get_entity (); + auto_diagnostic_group d; + if (strict) + { + if (warning_enabled_at (DECL_SOURCE_LOCATION (ref), + OPT_Wexpose_global_module_tu_local) + && warning_at (DECL_SOURCE_LOCATION (decl), + OPT_Wtemplate_names_tu_local, + "%qD refers to TU-local entity %qD, which may " + "cause issues when instantiating in other TUs", + decl, ref)) + { + is_tu_local_entity (ref, /*explain=*/true); + return true; + } + } + else if (warning_at (DECL_SOURCE_LOCATION (decl), + OPT_Wtemplate_names_tu_local, + "%qD refers to TU-local entity %qD and cannot " + "be instantiated in other TUs", decl, ref)) + { + is_tu_local_entity (ref, /*explain=*/true); + return true; + } + } + + return false; +} + /* Sort the bindings, issue errors about bad internal refs. */ bool @@ -15366,30 +15531,21 @@ depset::hash::finalize_dependencies () if (CHECKING_P) for (depset *entity : dep->deps) gcc_checking_assert (!entity->is_import ()); + continue; } - else if (dep->is_exposure () && !dep->is_tu_local ()) - { - ok = false; - bool explained = false; - tree decl = dep->get_entity (); - for (depset *rdep : dep->deps) - if (!rdep->is_binding () - && rdep->is_tu_local () - && !is_exposure_of_member_type (dep, rdep)) - { - // FIXME:QOI Better location information? We're - // losing, so it doesn't matter about efficiency - tree exposed = rdep->get_entity (); - auto_diagnostic_group d; - error_at (DECL_SOURCE_LOCATION (decl), - "%qD exposes TU-local entity %qD", decl, exposed); - bool informed = is_tu_local_entity (exposed, /*explain=*/true); - gcc_checking_assert (informed); - explained = true; - break; - } + /* Otherwise, we'll check for bad internal refs. + Don't complain about any references from TU-local entities. */ + if (dep->is_tu_local ()) + continue; + + if (dep->is_exposure ()) + { + bool explained = diagnose_bad_internal_ref (dep); + /* A TU-local variable will always be considered an exposure, + so we don't have to worry about strict-only handling. */ + tree decl = dep->get_entity (); if (!explained && VAR_P (decl) && (DECL_DECLARED_CONSTEXPR_P (decl) @@ -15414,42 +15570,34 @@ depset::hash::finalize_dependencies () explained = true; } - /* We should have emitted an error above. */ + /* We should have emitted an error above, unless the warning was + silenced. */ gcc_checking_assert (explained); + ok = false; + continue; } - else if (warn_template_names_tu_local - && dep->refs_tu_local () && !dep->is_tu_local ()) - { - tree decl = dep->get_entity (); - /* Friend decls in a class body are ignored, but this is harmless: - it should not impact any consumers. */ - if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl))) - continue; - - /* We should now only be warning about templates. */ - gcc_checking_assert - (TREE_CODE (decl) == TEMPLATE_DECL - && VAR_OR_FUNCTION_DECL_P (DECL_TEMPLATE_RESULT (decl))); + /* In all other cases, we're just warning (rather than erroring). + We don't want to do too much warning, so let's just bail after + the first warning we successfully emit. */ + if (warn_expose_global_module_tu_local + && !dep->is_tu_local (/*strict=*/true) + && dep->is_exposure (/*strict=*/true) + && diagnose_bad_internal_ref (dep, /*strict=*/true)) + continue; - /* Don't warn if we've seen any explicit instantiation definitions, - the intent might be for importers to only use those. */ - if (template_has_explicit_inst (decl)) - continue; + if (warn_template_names_tu_local + && dep->refs_tu_local () + && diagnose_template_names_tu_local (dep)) + continue; - for (depset *rdep : dep->deps) - if (!rdep->is_binding () && rdep->is_tu_local ()) - { - tree ref = rdep->get_entity (); - auto_diagnostic_group d; - if (warning_at (DECL_SOURCE_LOCATION (decl), - OPT_Wtemplate_names_tu_local, - "%qD refers to TU-local entity %qD and cannot " - "be instantiated in other TUs", decl, ref)) - is_tu_local_entity (ref, /*explain=*/true); - break; - } - } + if (warn_template_names_tu_local + && warn_expose_global_module_tu_local + && !dep->is_tu_local (/*strict=*/true) + && dep->refs_tu_local (/*strict=*/true) + && !dep->is_exposure (/*strict=*/true) + && diagnose_template_names_tu_local (dep, /*strict=*/true)) + continue; } return ok; diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 32b9c48f155..e4a595251e6 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -257,7 +257,8 @@ in the following sections. -Wdeprecated-copy -Wdeprecated-copy-dtor -Wno-deprecated-enum-enum-conversion -Wno-deprecated-enum-float-conversion -Weffc++ -Wno-elaborated-enum-base --Wno-exceptions -Wextra-semi -Wno-global-module -Wno-inaccessible-base +-Wno-exceptions -Wno-expose-global-module-tu-local -Wno-external-tu-local +-Wextra-semi -Wno-global-module -Wno-inaccessible-base -Wno-inherited-variadic-ctor -Wno-init-list-lifetime -Winvalid-constexpr -Winvalid-imported-macros -Wno-invalid-offsetof -Wno-literal-suffix @@ -272,7 +273,7 @@ in the following sections. -Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo -Wsized-deallocation -Wsuggest-final-methods -Wsuggest-final-types -Wsuggest-override -Wno-template-body --Wno-template-id-cdtor -Wtemplate-names-tu-local -Wno-external-tu-local +-Wno-template-id-cdtor -Wtemplate-names-tu-local -Wno-terminate -Wno-vexing-parse -Wvirtual-inheritance -Wno-virtual-move-assign -Wvolatile} @@ -4759,6 +4760,29 @@ The presence of an explicit instantiation silences the warning. This flag is enabled by @option{-Wextra}. +@opindex Wexpose-global-module-tu-local +@opindex Wno-expose-global-module-tu-local +@item -Wno-expose-global-module-tu-local +An exposure of a translation-unit-local entity from a module interface is +invalid, as this may cause ODR violations and manifest in link errors or other +unexpected behaviour. However, many existing libraries declare TU-local +entities in their interface, and avoiding exposures of these entities may be +difficult in some cases. + +As an extension, GCC allows exposures of internal variables and functions that +were declared in the global module fragment. This warning indicates when such +an invalid exposure has occurred, and can be silenced using diagnostic pragmas +either at the site of the exposure, or at the point of declaration of the +internal declaration. + +When combined with @option{-Wtemplate-names-tu-local}, GCC will also warn about +non-exposure references to TU-local entities in template bodies. Such templates +can still be instantiated in other TUs but the above risks regarding exposures +of translation-unit-local entities apply. + +This warning is enabled by default, and is upgraded to an error by +@option{-pedantic-errors}. + @opindex Wexternal-tu-local @opindex Wno-external-tu-local @item -Wno-external-tu-local diff --git a/gcc/testsuite/g++.dg/modules/internal-17_a.C b/gcc/testsuite/g++.dg/modules/internal-17_a.C new file mode 100644 index 00000000000..17eef47844a --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/internal-17_a.C @@ -0,0 +1,64 @@ +// PR c++/121574 +// { dg-additional-options "-fmodules -Wno-error=expose-global-module-tu-local -Wtemplate-names-tu-local -Wno-global-module" } +// { dg-module-cmi M } + +module; + +namespace { + void foo() {} + inline int bar = 123; + template void qux() {} + template void qux(); + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wexpose-global-module-tu-local" + void foo_ignored() {} + inline int bar_ignored = 123; + template void qux_ignored() {} + template void qux_ignored(); +#pragma GCC diagnostic pop +}; + +export module M; + +export inline void a() { // { dg-warning "exposes TU-local" } + foo(); + int x = bar; +} + +export inline void b() { + foo_ignored(); + int x = bar_ignored; +} + +export template +void c() { // { dg-warning "refers to TU-local" } + foo(); + int x = bar; +} + +export template +void d() { + foo_ignored(); + int x = bar_ignored; +} + +export inline void e() { // { dg-warning "exposes TU-local" } + foo(); + int result = bar_ignored; +} + +export template +void f() { // { dg-warning "refers to TU-local" } + foo_ignored(); + int result = bar; +} + +export inline void g() { // { dg-warning "exposes TU-local" } + qux(); +} + +export template +void h() { + qux_ignored(); +} diff --git a/gcc/testsuite/g++.dg/modules/internal-17_b.C b/gcc/testsuite/g++.dg/modules/internal-17_b.C new file mode 100644 index 00000000000..fded871d82a --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/internal-17_b.C @@ -0,0 +1,58 @@ +// PR c++/121576 +// { dg-additional-options "-fmodules -Wno-error=expose-global-module-tu-local -Wtemplate-names-tu-local -Wno-global-module" } +// { dg-module-cmi !X } + +module; + +static inline int x // { dg-error "TU-local" } + // { dg-message "exposed elsewhere" "" { target *-*-* } .-1 } + = []{ return 1; }(); // { dg-message "internal" } + +static inline int y = []{ return 2; }(); // { dg-bogus "" } + +namespace { + struct S {}; + template void tmpl(); + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wexpose-global-module-tu-local" + struct S_ignored {}; + template void tmpl_ignored(); +#pragma GCC diagnostic pop +} + +export module X; +import M; + +void test_usage() { + a(); + b(); + c(); + d(); + e(); + f(); + g(); + h(); +} + +inline void expose() { // { dg-warning "exposes TU-local" } + int result = x; +} + +// Internal linkage types always hard error +inline void expose_struct() { // { dg-error "exposes TU-local" } + S s; +} +inline void still_expose_struct() { // { dg-error "exposes TU-local" } + S_ignored s; +} + +// Template instantiations occuring in module purview are not ignored, +// as it's too hard to tell if the instantiation will accidentally rely +// on something in the purview or not. +inline void expose_tmpl() { // { dg-error "exposes TU-local" } + tmpl(); +} +inline void still_expose_tmpl() { // { dg-error "exposes TU-local" } + tmpl_ignored(); +} -- 2.47.3