| Message ID | 66c09cc7.170a0220.31e94b.763d@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: Disable streaming definitions of non-vague-linkage GMF decls [PR115020] | expand |
On 8/17/24 8:51 AM, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > The error in the linked PR is caused because 'DECL_THIS_STATIC' is true > for the static member function, causing the streaming code to assume > that this is an internal linkage GM entity that needs to be explicitly > streamed, which then on read-in gets marked as a vague linkage function > (despite being non-inline) causing import_export_decl to complain. > > However, I don't see any reason why we should care about this: > definitions in the GMF should just be emitted as per usual regardless of > whether they're internal-linkage or not. Actually the only thing we > care about here are header modules, since they have no TU to write > definitions into. As such this patch removes these conditions from > 'has_definition' and updates some comments to clarify. > > PR c++/115020 > > gcc/cp/ChangeLog: > > * module.cc (has_definition): Only force writing definitions for > header_module_p. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/pr115020_a.C: New test. > * g++.dg/modules/pr115020_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 16 ++++++++-------- > gcc/testsuite/g++.dg/modules/pr115020_a.C | 10 ++++++++++ > gcc/testsuite/g++.dg/modules/pr115020_b.C | 10 ++++++++++ > 3 files changed, 28 insertions(+), 8 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/pr115020_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/pr115020_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index f4d137b13a1..ae6cab0aac4 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -11782,13 +11782,12 @@ has_definition (tree decl) > /* Not defined. */ > break; > > - if (DECL_DECLARED_INLINE_P (decl)) > + if (header_module_p ()) > + /* We always need to write definitions in header modules, > + since there's no TU to emit them in otherwise. */ > return true; > > - if (DECL_THIS_STATIC (decl) > - && (header_module_p () > - || (!DECL_LANG_SPECIFIC (decl) || !DECL_MODULE_PURVIEW_P (decl)))) > - /* GM static function. */ > + if (DECL_DECLARED_INLINE_P (decl)) > return true; Let's keep the header_module check after the INLINE_P check to make the change smaller. OK with that adjustment. Jason
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index f4d137b13a1..ae6cab0aac4 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -11782,13 +11782,12 @@ has_definition (tree decl) /* Not defined. */ break; - if (DECL_DECLARED_INLINE_P (decl)) + if (header_module_p ()) + /* We always need to write definitions in header modules, + since there's no TU to emit them in otherwise. */ return true; - if (DECL_THIS_STATIC (decl) - && (header_module_p () - || (!DECL_LANG_SPECIFIC (decl) || !DECL_MODULE_PURVIEW_P (decl)))) - /* GM static function. */ + if (DECL_DECLARED_INLINE_P (decl)) return true; if (DECL_TEMPLATE_INFO (decl)) @@ -11821,11 +11820,12 @@ has_definition (tree decl) else { if (!DECL_INITIALIZED_P (decl)) + /* Not defined. */ return false; - if (header_module_p () - || (!DECL_LANG_SPECIFIC (decl) || !DECL_MODULE_PURVIEW_P (decl))) - /* GM static variable. */ + if (header_module_p ()) + /* We always need to write definitions in header modules, + since there's no TU to emit them in otherwise. */ return true; if (!TREE_CONSTANT (decl)) diff --git a/gcc/testsuite/g++.dg/modules/pr115020_a.C b/gcc/testsuite/g++.dg/modules/pr115020_a.C new file mode 100644 index 00000000000..8c190f13b1e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr115020_a.C @@ -0,0 +1,10 @@ +// PR c++/115020 +// { dg-additional-options "-fmodules-ts -Wno-global-module" } +// { dg-module-cmi M:a } + +module; +struct Check { static void assertion(); }; +void Check::assertion() {} + +module M:a; +Check c; diff --git a/gcc/testsuite/g++.dg/modules/pr115020_b.C b/gcc/testsuite/g++.dg/modules/pr115020_b.C new file mode 100644 index 00000000000..e299454ed54 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr115020_b.C @@ -0,0 +1,10 @@ +// PR c++/115020 +// { dg-additional-options "-fmodules-ts -Wno-global-module" } +// { dg-module-cmi M } + +module; +struct Check { static void assertion(); }; + +export module M; +import :a; +void foo() { Check::assertion(); }
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- The error in the linked PR is caused because 'DECL_THIS_STATIC' is true for the static member function, causing the streaming code to assume that this is an internal linkage GM entity that needs to be explicitly streamed, which then on read-in gets marked as a vague linkage function (despite being non-inline) causing import_export_decl to complain. However, I don't see any reason why we should care about this: definitions in the GMF should just be emitted as per usual regardless of whether they're internal-linkage or not. Actually the only thing we care about here are header modules, since they have no TU to write definitions into. As such this patch removes these conditions from 'has_definition' and updates some comments to clarify. PR c++/115020 gcc/cp/ChangeLog: * module.cc (has_definition): Only force writing definitions for header_module_p. gcc/testsuite/ChangeLog: * g++.dg/modules/pr115020_a.C: New test. * g++.dg/modules/pr115020_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 16 ++++++++-------- gcc/testsuite/g++.dg/modules/pr115020_a.C | 10 ++++++++++ gcc/testsuite/g++.dg/modules/pr115020_b.C | 10 ++++++++++ 3 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/pr115020_a.C create mode 100644 gcc/testsuite/g++.dg/modules/pr115020_b.C