On Tue, Sep 24, 2024 at 09:47:17AM +1000, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >8 --
>
> [module.import] p6 says "A header unit shall not contain a definition of
> a non-inline function or variable whose name has external linkage."
>
> This patch implements this requirement, and cleans up some issues in the
> testsuite where this was already violated. To handle deduction guides
> we mark them as inline, since although we give them a definition for
> implementation by the standard they have no definition, and so should
> not error in this case.
>
> One remaining question is the behaviour of code like:
>
> struct S { static const int x = 123; };
>
> 'S::x' is not 'inline' here, but this is legal code as long as there is
> exactly one definition elsewhere if 'x' is ever odr-used, as specified
> by [class.static.data] p4. However, since it's not 'inline' then the
> exemption for [module.import] does not apply, and so it appears uses of
> this in header units should error.
>
> Unfortunately the standard library headers do this, and there doesn't
> appear to be an easy C++98-compatible way to adjust this. Additionally
> I'm not entirely certain that this wasn't an oversight; as such this
> patch reduces this specific case to merely a pedwarn.
>
While testing another change I found that this causes test regressions
now since r15-3859-g63a598deb0c9fc; the pedwarn for usages in the
standard library headers gets picked up as an error in tests that build
the standard library headers as header units (e.g. xtreme-header*,
binding*, etc.).
What would be the best approach here in your opinion:
- Silence the pedwarn in the libstdc++ headers with a #pragma?
- Change modules.exp to treat these as system headers again?
- Make a dedicated flag for this specific pedwarn and disable it?
- Assume that this is meant to be legal and silence the warning entirely?
Nathaniel
> PR c++/116401
>
> gcc/cp/ChangeLog:
>
> * decl.cc (grokfndecl): Mark deduction guides as 'inline'.
> * module.cc (check_module_decl_linkage): Implement checks for
> non-inline external linkage definitions in headers.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/macro-4_c.H: Add missing 'inline'.
> * g++.dg/modules/pr106761.h: Likewise.
> * g++.dg/modules/pr98843_b.H: Likewise.
> * g++.dg/modules/pr99468.H: Likewise.
> * g++.dg/modules/pragma-1_a.H: Likewise.
> * g++.dg/modules/tpl-ary-1.h: Likewise.
> * g++.dg/modules/hdr-2.H: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/decl.cc | 1 +
> gcc/cp/module.cc | 33 +++++
> gcc/testsuite/g++.dg/modules/hdr-2.H | 164 ++++++++++++++++++++++
> gcc/testsuite/g++.dg/modules/macro-4_c.H | 2 +-
> gcc/testsuite/g++.dg/modules/pr106761.h | 2 +-
> gcc/testsuite/g++.dg/modules/pr98843_b.H | 2 +-
> gcc/testsuite/g++.dg/modules/pr99468.H | 2 +-
> gcc/testsuite/g++.dg/modules/pragma-1_a.H | 2 +-
> gcc/testsuite/g++.dg/modules/tpl-ary-1.h | 2 +-
> 9 files changed, 204 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/hdr-2.H
>
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 2190ede745b..0203bbb682b 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -10818,6 +10818,7 @@ grokfndecl (tree ctype,
> have one: the restriction that you can't repeat a deduction guide
> makes them more like a definition anyway. */
> DECL_INITIAL (decl) = void_node;
> + DECL_DECLARED_INLINE_P (decl) = true;
> break;
> default:
> break;
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index d7e6fe2c54f..143eb676ce9 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -19937,6 +19937,39 @@ check_module_decl_linkage (tree decl)
> if (!module_has_cmi_p ())
> return;
>
> + /* A header unit shall not contain a definition of a non-inline function
> + or variable (not template) whose name has external linkage. */
> + if (header_module_p ()
> + && !processing_template_decl
> + && ((TREE_CODE (decl) == FUNCTION_DECL
> + && !DECL_DECLARED_INLINE_P (decl)
> + && DECL_INITIAL (decl))
> + || (TREE_CODE (decl) == VAR_DECL
> + && !DECL_INLINE_VAR_P (decl)
> + && DECL_INITIALIZED_P (decl)))
> + && !(DECL_LANG_SPECIFIC (decl)
> + && DECL_TEMPLATE_INSTANTIATION (decl))
> + && decl_linkage (decl) == lk_external)
> + {
> + /* Strictly speaking,
> +
> + struct S { static const int x = 123; };
> +
> + is not valid in a header unit as currently specified. But this is
> + done within the standard library, and there doesn't seem to be a
> + C++98-compatible alternative, so we support this with a pedwarn. */
> + if (VAR_P (decl)
> + && DECL_CLASS_SCOPE_P (decl)
> + && DECL_INITIALIZED_IN_CLASS_P (decl))
> + pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wpedantic,
> + "external linkage definition of %qD in header module must "
> + "be declared %<inline%>", decl);
> + else
> + error_at (DECL_SOURCE_LOCATION (decl),
> + "external linkage definition of %qD in header module must "
> + "be declared %<inline%>", 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. */
> diff --git a/gcc/testsuite/g++.dg/modules/hdr-2.H
> b/gcc/testsuite/g++.dg/modules/hdr-2.H
> new file mode 100644
> index 00000000000..bb3d7d70123
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/hdr-2.H
> @@ -0,0 +1,164 @@
> +// { dg-additional-options "-fmodule-header -Wno-error=pedantic" }
> +// { dg-module-cmi !{} }
> +// external linkage variables or functions in header units must
> +// not have non-inline definitions
> +
> +int x_err; // { dg-error "inline" }
> +int y_err = 123; // { dg-error "inline" }
> +void f_err() {} // { dg-error "inline" }
> +
> +struct Err {
> + Err();
> + void m();
> + static void s();
> + static int x;
> + static int y;
> +};
> +Err::Err() = default; // { dg-error "inline" }
> +void Err::m() {} // { dg-error "inline" }
> +void Err::s() {} // { dg-error "inline" }
> +int Err::x; // { dg-error "inline" }
> +int Err::y = 123; // { dg-error "inline" }
> +
> +// Strictly speaking erroneous, but we support anyway with pedwarn:
> +struct Ped {
> + enum E { V };
> + static const int x = 123; // { dg-warning "inline" }
> + static const E e = V; // { dg-warning "inline" }
> +};
> +
> +// No definition, OK
> +extern int y_decl;
> +void f_decl();
> +
> +template <typename> struct DeductionGuide {};
> +DeductionGuide() -> DeductionGuide<int>;
> +
> +// Inline decls are OK
> +inline int x_inl;
> +inline int y_inl = 123;
> +inline void f_inl() {}
> +constexpr void g_inl() {}
> +void h_inl() = delete;
> +
> +struct Inl {
> + void m() {}
> + static void s() {}
> + static inline int x;
> + static inline int y = 123;
> +};
> +
> +// Internal linkage decls are OK
> +static int x_internal;
> +static int y_internal = 123;
> +static void f_internal() {}
> +
> +namespace {
> + struct Internal {
> + void m();
> + static void s();
> + static int x;
> + static int y;
> + };
> + void Internal::m() {}
> + void Internal::s() {}
> + int Internal::x;
> + int Internal::y = 123;
> +}
> +
> +// Function-scope entities are OK
> +inline void f_static() {
> + static int x_static;
> + static int y_static = 123;
> + thread_local int x_thread_local;
> + thread_local int y_thread_local = 123;
> +
> + x_static = y_static;
> + x_thread_local = y_thread_local;
> +}
> +
> +// Templates (not functions or variables) are OK
> +template <typename> int x_tpl;
> +template <typename> int y_tpl = 123;
> +template <typename> void f_tpl() {}
> +
> +struct Template_Body {
> + template <typename> void m();
> + template <typename> static void s();
> + template <typename> static int x;
> + template <typename> static int y;
> +};
> +template <typename> void Template_Body::m() {}
> +template <typename> void Template_Body::s() {}
> +template <typename> int Template_Body::x;
> +template <typename> int Template_Body::y = 123;
> +
> +template <typename> struct Template_Type {
> + void m();
> + static void s();
> + static int x;
> + static int y;
> +};
> +template <typename T> void Template_Type<T>::m() {}
> +template <typename T> void Template_Type<T>::s() {}
> +template <typename T> int Template_Type<T>::x;
> +template <typename T> int Template_Type<T>::y = 123;
> +
> +// Implicit instantiations are OK
> +inline void instantiate_tmpls() {
> + x_tpl<int> = y_tpl<int>;
> + f_tpl<int>();
> +
> + Template_Body{}.m<int>();
> + Template_Body::s<int>();
> + Template_Body::x<int> = Template_Body::y<int>;
> +
> + using TT = Template_Type<int>;
> + TT{}.m();
> + TT::s();
> + TT::x = TT::y;
> +}
> +
> +// Explicit instantiations are also OK (extern or otherwise)
> +template int x_tpl<char>;
> +template int y_tpl<char>;
> +template void f_tpl<char>();
> +
> +template void Template_Body::m<char>();
> +template void Template_Body::s<char>();
> +template int Template_Body::x<char>;
> +template int Template_Body::y<char>;
> +
> +template void Template_Type<char>::m();
> +template void Template_Type<char>::s();
> +template int Template_Type<char>::x;
> +template int Template_Type<char>::y;
> +
> +extern template int x_tpl<double>;
> +extern template int y_tpl<double>;
> +extern template void f_tpl<double>();
> +
> +extern template void Template_Body::m<double>();
> +extern template void Template_Body::s<double>();
> +extern template int Template_Body::x<double>;
> +extern template int Template_Body::y<double>;
> +
> +extern template void Template_Type<double>::m();
> +extern template void Template_Type<double>::s();
> +extern template int Template_Type<double>::x;
> +extern template int Template_Type<double>::y;
> +
> +// But explicit specialisations are not (though note [temp.expl.spec] p13)
> +template <> int x_tpl<short>; // { dg-error "inline" }
> +template <> int y_tpl<short> = 123; // { dg-error "inline" }
> +template <> void f_tpl<short>() {} // { dg-error "inline" }
> +
> +template <> void Template_Body::m<short>() {} // { dg-error "inline" }
> +template <> void Template_Body::s<short>() {} // { dg-error "inline" }
> +template <> int Template_Body::x<short>; // { dg-bogus "inline" "not a
> definition" }
> +template <> int Template_Body::y<short> = 123; // { dg-error "inline" }
> +
> +template <> void Template_Type<short>::m() {} // { dg-error "inline" }
> +template <> void Template_Type<short>::s() {} // { dg-error "inline" }
> +template <> int Template_Type<short>::x; // { dg-bogus "inline" "not a
> definition" }
> +template <> int Template_Type<short>::y = 123; // { dg-error "inline" }
> diff --git a/gcc/testsuite/g++.dg/modules/macro-4_c.H
> b/gcc/testsuite/g++.dg/modules/macro-4_c.H
> index ec2bed91ccd..5692e8e41d2 100644
> --- a/gcc/testsuite/g++.dg/modules/macro-4_c.H
> +++ b/gcc/testsuite/g++.dg/modules/macro-4_c.H
> @@ -6,7 +6,7 @@
>
> #undef FIVE // no effect
> import "macro-4_a.H";
> -int a;
> +inline int a;
> #undef THREE
> #undef FOUR
> #define FOUR 4c
> diff --git a/gcc/testsuite/g++.dg/modules/pr106761.h
> b/gcc/testsuite/g++.dg/modules/pr106761.h
> index 9f22a22a45d..5c13fc0f118 100644
> --- a/gcc/testsuite/g++.dg/modules/pr106761.h
> +++ b/gcc/testsuite/g++.dg/modules/pr106761.h
> @@ -19,4 +19,4 @@ struct tuple {
> = typename _TupleConstraints<Ts...>::template __constructible<Us...>;
> };
>
> -tuple<int, int> t;
> +inline tuple<int, int> t;
> diff --git a/gcc/testsuite/g++.dg/modules/pr98843_b.H
> b/gcc/testsuite/g++.dg/modules/pr98843_b.H
> index d6734bd382d..af504a840c7 100644
> --- a/gcc/testsuite/g++.dg/modules/pr98843_b.H
> +++ b/gcc/testsuite/g++.dg/modules/pr98843_b.H
> @@ -6,7 +6,7 @@ template<int I> int Fn ()
> return I;
> }
>
> -template<> int Fn<1> ()
> +template<> inline int Fn<1> ()
> {
> return 0;
> }
> diff --git a/gcc/testsuite/g++.dg/modules/pr99468.H
> b/gcc/testsuite/g++.dg/modules/pr99468.H
> index d7da3a83e1c..df63c613b2c 100644
> --- a/gcc/testsuite/g++.dg/modules/pr99468.H
> +++ b/gcc/testsuite/g++.dg/modules/pr99468.H
> @@ -3,4 +3,4 @@
>
> module M; // { dg-error "module-declaration not permitted" }
>
> -int i;
> +inline int i;
> diff --git a/gcc/testsuite/g++.dg/modules/pragma-1_a.H
> b/gcc/testsuite/g++.dg/modules/pragma-1_a.H
> index 6eb0a5900a7..a69be2fd05e 100644
> --- a/gcc/testsuite/g++.dg/modules/pragma-1_a.H
> +++ b/gcc/testsuite/g++.dg/modules/pragma-1_a.H
> @@ -1,4 +1,4 @@
> // { dg-additional-options -fmodule-header }
>
> // { dg-module-cmi {} }
> -int i;
> +inline int i;
> diff --git a/gcc/testsuite/g++.dg/modules/tpl-ary-1.h
> b/gcc/testsuite/g++.dg/modules/tpl-ary-1.h
> index 2f745afba36..2f9a8106412 100644
> --- a/gcc/testsuite/g++.dg/modules/tpl-ary-1.h
> +++ b/gcc/testsuite/g++.dg/modules/tpl-ary-1.h
> @@ -1,5 +1,5 @@
>
> -int ary[4];
> +inline int ary[4];
> extern int unb[];
> typedef int z[0];
>
> --
> 2.46.0
>
