Message ID | 65e12aa5.630a0220.6b431.a00b@mx.google.com |
---|---|
State | New |
Headers | show |
Series | c++/modules: Stream definitions for implicit instantiations [PR114170] | expand |
On 2/29/24 20:08, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > An implicit instantiation has an initializer depending on whether > DECL_INITIALIZED_P is set (like normal VAR_DECLs) which needs to be > written to ensure that consumers of header modules properly emit > definitions for these instantiations. This patch ensures that we > correctly fallback to checking this flag when DECL_INITIAL is not set > for a template instantiation. Can you say more about how and why DECL_INITIAL and DECL_INITIALIZED_P are inconsistent here? > As a drive-by fix, also ensures that the count of initializers matches > the actual number of initializers written. This doesn't seem to be > necessary for correctness in the current testsuite, but feels wrong and > makes debugging harder when initializers aren't properly written for > other reasons. > > PR c++/114170 > > gcc/cp/ChangeLog: > > * module.cc (has_definition): Fall back to DECL_INITIALIZED_P > when DECL_INITIAL is not set on a template. > (module_state::write_inits): Only increment count when > initializers are actually written. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/var-tpl-2_a.H: New test. > * g++.dg/modules/var-tpl-2_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 8 +++++--- > gcc/testsuite/g++.dg/modules/var-tpl-2_a.H | 10 ++++++++++ > gcc/testsuite/g++.dg/modules/var-tpl-2_b.C | 10 ++++++++++ > 3 files changed, 25 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 1b2ba2e0fa8..09578de41ec 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -11586,8 +11586,9 @@ has_definition (tree decl) > > case VAR_DECL: > if (DECL_LANG_SPECIFIC (decl) > - && DECL_TEMPLATE_INFO (decl)) > - return DECL_INITIAL (decl); > + && DECL_TEMPLATE_INFO (decl) > + && DECL_INITIAL (decl)) > + return true; > else > { > if (!DECL_INITIALIZED_P (decl)) > @@ -17528,13 +17529,14 @@ module_state::write_inits (elf_out *to, depset::hash &table, unsigned *crc_ptr) > tree list = static_aggregates; > for (int passes = 0; passes != 2; passes++) > { > - for (tree init = list; init; init = TREE_CHAIN (init), count++) > + for (tree init = list; init; init = TREE_CHAIN (init)) > if (TREE_LANG_FLAG_0 (init)) > { > tree decl = TREE_VALUE (init); > > dump ("Initializer:%u for %N", count, decl); > sec.tree_node (decl); > + ++count; > } > > list = tls_aggregates; > diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H > new file mode 100644 > index 00000000000..607fc0b808e > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H > @@ -0,0 +1,10 @@ > +// PR c++/114170 > +// { dg-additional-options "-fmodule-header" } > +// { dg-module-cmi {} } > + > +inline int f() { return 42; } > + > +template<class> > +inline int v = f(); > + > +inline int g() { return v<int>; } > diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C > new file mode 100644 > index 00000000000..6d2ef4004e6 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C > @@ -0,0 +1,10 @@ > +// PR c++/114170 > +// { dg-module-do run } > +// { dg-additional-options "-fmodules-ts" } > + > +import "var-tpl-2_a.H"; > + > +int main() { > + if (v<int> != 42) > + __builtin_abort(); > +}
On Fri, Mar 01, 2024 at 08:18:09AM -0500, Jason Merrill wrote: > On 2/29/24 20:08, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > -- >8 -- > > > > An implicit instantiation has an initializer depending on whether > > DECL_INITIALIZED_P is set (like normal VAR_DECLs) which needs to be > > written to ensure that consumers of header modules properly emit > > definitions for these instantiations. This patch ensures that we > > correctly fallback to checking this flag when DECL_INITIAL is not set > > for a template instantiation. > > Can you say more about how and why DECL_INITIAL and DECL_INITIALIZED_P are > inconsistent here? For variables with non-trivial dynamic initialization, DECL_INITIAL can be empty after 'split_nonconstant_init' but DECL_INITIALIZED_P is still set; we need to check the latter to determine if we need to go looking for a definition to emit (often in 'static_aggregates' here). This is the case in the linked testcase. However, for template specialisations (not instantiations?) we primarily care about DECL_INITIAL; if the variable has initialization depending on a template parameter then we'll need to emit that definition even though it doesn't yet have DECL_INITIALIZED_P set; this is the case in e.g. template <int N> int value = N; > > As a drive-by fix, also ensures that the count of initializers matches > > the actual number of initializers written. This doesn't seem to be > > necessary for correctness in the current testsuite, but feels wrong and > > makes debugging harder when initializers aren't properly written for > > other reasons. > > > > PR c++/114170 > > > > gcc/cp/ChangeLog: > > > > * module.cc (has_definition): Fall back to DECL_INITIALIZED_P > > when DECL_INITIAL is not set on a template. > > (module_state::write_inits): Only increment count when > > initializers are actually written. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/modules/var-tpl-2_a.H: New test. > > * g++.dg/modules/var-tpl-2_b.C: New test. > > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > > --- > > gcc/cp/module.cc | 8 +++++--- > > gcc/testsuite/g++.dg/modules/var-tpl-2_a.H | 10 ++++++++++ > > gcc/testsuite/g++.dg/modules/var-tpl-2_b.C | 10 ++++++++++ > > 3 files changed, 25 insertions(+), 3 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_a.H > > create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_b.C > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > index 1b2ba2e0fa8..09578de41ec 100644 > > --- a/gcc/cp/module.cc > > +++ b/gcc/cp/module.cc > > @@ -11586,8 +11586,9 @@ has_definition (tree decl) > > case VAR_DECL: > > if (DECL_LANG_SPECIFIC (decl) > > - && DECL_TEMPLATE_INFO (decl)) > > - return DECL_INITIAL (decl); > > + && DECL_TEMPLATE_INFO (decl) > > + && DECL_INITIAL (decl)) > > + return true; > > else > > { > > if (!DECL_INITIALIZED_P (decl)) > > @@ -17528,13 +17529,14 @@ module_state::write_inits (elf_out *to, depset::hash &table, unsigned *crc_ptr) > > tree list = static_aggregates; > > for (int passes = 0; passes != 2; passes++) > > { > > - for (tree init = list; init; init = TREE_CHAIN (init), count++) > > + for (tree init = list; init; init = TREE_CHAIN (init)) > > if (TREE_LANG_FLAG_0 (init)) > > { > > tree decl = TREE_VALUE (init); > > dump ("Initializer:%u for %N", count, decl); > > sec.tree_node (decl); > > + ++count; > > } > > list = tls_aggregates; > > diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H > > new file mode 100644 > > index 00000000000..607fc0b808e > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H > > @@ -0,0 +1,10 @@ > > +// PR c++/114170 > > +// { dg-additional-options "-fmodule-header" } > > +// { dg-module-cmi {} } > > + > > +inline int f() { return 42; } > > + > > +template<class> > > +inline int v = f(); > > + > > +inline int g() { return v<int>; } > > diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C > > new file mode 100644 > > index 00000000000..6d2ef4004e6 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C > > @@ -0,0 +1,10 @@ > > +// PR c++/114170 > > +// { dg-module-do run } > > +// { dg-additional-options "-fmodules-ts" } > > + > > +import "var-tpl-2_a.H"; > > + > > +int main() { > > + if (v<int> != 42) > > + __builtin_abort(); > > +} >
On 3/1/24 08:41, Nathaniel Shead wrote: > On Fri, Mar 01, 2024 at 08:18:09AM -0500, Jason Merrill wrote: >> On 2/29/24 20:08, Nathaniel Shead wrote: >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? >>> >>> -- >8 -- >>> >>> An implicit instantiation has an initializer depending on whether >>> DECL_INITIALIZED_P is set (like normal VAR_DECLs) which needs to be >>> written to ensure that consumers of header modules properly emit >>> definitions for these instantiations. This patch ensures that we >>> correctly fallback to checking this flag when DECL_INITIAL is not set >>> for a template instantiation. >> >> Can you say more about how and why DECL_INITIAL and DECL_INITIALIZED_P are >> inconsistent here? > > For variables with non-trivial dynamic initialization, DECL_INITIAL can > be empty after 'split_nonconstant_init' but DECL_INITIALIZED_P is still > set; we need to check the latter to determine if we need to go looking > for a definition to emit (often in 'static_aggregates' here). This is > the case in the linked testcase. Ah, right. > However, for template specialisations (not instantiations?) we primarily > care about DECL_INITIAL; if the variable has initialization depending on > a template parameter then we'll need to emit that definition even though > it doesn't yet have DECL_INITIALIZED_P set; this is the case in e.g. > > template <int N> int value = N; It seems odd that DECL_INITIALIZED_P wouldn't be set for the pattern of value. But given that, the patch makes sense. Let's add a comment like /* DECL_INITIALIZED_P might not be set on a dependent VAR_DECL. */ to explain the DECL_TEMPLATE_INFO special case. OK with that change. >>> As a drive-by fix, also ensures that the count of initializers matches >>> the actual number of initializers written. This doesn't seem to be >>> necessary for correctness in the current testsuite, but feels wrong and >>> makes debugging harder when initializers aren't properly written for >>> other reasons. >>> >>> PR c++/114170 >>> >>> gcc/cp/ChangeLog: >>> >>> * module.cc (has_definition): Fall back to DECL_INITIALIZED_P >>> when DECL_INITIAL is not set on a template. >>> (module_state::write_inits): Only increment count when >>> initializers are actually written. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/modules/var-tpl-2_a.H: New test. >>> * g++.dg/modules/var-tpl-2_b.C: New test. >>> >>> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> >>> --- >>> gcc/cp/module.cc | 8 +++++--- >>> gcc/testsuite/g++.dg/modules/var-tpl-2_a.H | 10 ++++++++++ >>> gcc/testsuite/g++.dg/modules/var-tpl-2_b.C | 10 ++++++++++ >>> 3 files changed, 25 insertions(+), 3 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_a.H >>> create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_b.C >>> >>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>> index 1b2ba2e0fa8..09578de41ec 100644 >>> --- a/gcc/cp/module.cc >>> +++ b/gcc/cp/module.cc >>> @@ -11586,8 +11586,9 @@ has_definition (tree decl) >>> case VAR_DECL: >>> if (DECL_LANG_SPECIFIC (decl) >>> - && DECL_TEMPLATE_INFO (decl)) >>> - return DECL_INITIAL (decl); >>> + && DECL_TEMPLATE_INFO (decl) >>> + && DECL_INITIAL (decl)) >>> + return true; >>> else >>> { >>> if (!DECL_INITIALIZED_P (decl)) >>> @@ -17528,13 +17529,14 @@ module_state::write_inits (elf_out *to, depset::hash &table, unsigned *crc_ptr) >>> tree list = static_aggregates; >>> for (int passes = 0; passes != 2; passes++) >>> { >>> - for (tree init = list; init; init = TREE_CHAIN (init), count++) >>> + for (tree init = list; init; init = TREE_CHAIN (init)) >>> if (TREE_LANG_FLAG_0 (init)) >>> { >>> tree decl = TREE_VALUE (init); >>> dump ("Initializer:%u for %N", count, decl); >>> sec.tree_node (decl); >>> + ++count; >>> } >>> list = tls_aggregates; >>> diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H >>> new file mode 100644 >>> index 00000000000..607fc0b808e >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H >>> @@ -0,0 +1,10 @@ >>> +// PR c++/114170 >>> +// { dg-additional-options "-fmodule-header" } >>> +// { dg-module-cmi {} } >>> + >>> +inline int f() { return 42; } >>> + >>> +template<class> >>> +inline int v = f(); >>> + >>> +inline int g() { return v<int>; } >>> diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C >>> new file mode 100644 >>> index 00000000000..6d2ef4004e6 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C >>> @@ -0,0 +1,10 @@ >>> +// PR c++/114170 >>> +// { dg-module-do run } >>> +// { dg-additional-options "-fmodules-ts" } >>> + >>> +import "var-tpl-2_a.H"; >>> + >>> +int main() { >>> + if (v<int> != 42) >>> + __builtin_abort(); >>> +} >> >
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 1b2ba2e0fa8..09578de41ec 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -11586,8 +11586,9 @@ has_definition (tree decl) case VAR_DECL: if (DECL_LANG_SPECIFIC (decl) - && DECL_TEMPLATE_INFO (decl)) - return DECL_INITIAL (decl); + && DECL_TEMPLATE_INFO (decl) + && DECL_INITIAL (decl)) + return true; else { if (!DECL_INITIALIZED_P (decl)) @@ -17528,13 +17529,14 @@ module_state::write_inits (elf_out *to, depset::hash &table, unsigned *crc_ptr) tree list = static_aggregates; for (int passes = 0; passes != 2; passes++) { - for (tree init = list; init; init = TREE_CHAIN (init), count++) + for (tree init = list; init; init = TREE_CHAIN (init)) if (TREE_LANG_FLAG_0 (init)) { tree decl = TREE_VALUE (init); dump ("Initializer:%u for %N", count, decl); sec.tree_node (decl); + ++count; } list = tls_aggregates; diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H new file mode 100644 index 00000000000..607fc0b808e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_a.H @@ -0,0 +1,10 @@ +// PR c++/114170 +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +inline int f() { return 42; } + +template<class> +inline int v = f(); + +inline int g() { return v<int>; } diff --git a/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C new file mode 100644 index 00000000000..6d2ef4004e6 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/var-tpl-2_b.C @@ -0,0 +1,10 @@ +// PR c++/114170 +// { dg-module-do run } +// { dg-additional-options "-fmodules-ts" } + +import "var-tpl-2_a.H"; + +int main() { + if (v<int> != 42) + __builtin_abort(); +}
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- An implicit instantiation has an initializer depending on whether DECL_INITIALIZED_P is set (like normal VAR_DECLs) which needs to be written to ensure that consumers of header modules properly emit definitions for these instantiations. This patch ensures that we correctly fallback to checking this flag when DECL_INITIAL is not set for a template instantiation. As a drive-by fix, also ensures that the count of initializers matches the actual number of initializers written. This doesn't seem to be necessary for correctness in the current testsuite, but feels wrong and makes debugging harder when initializers aren't properly written for other reasons. PR c++/114170 gcc/cp/ChangeLog: * module.cc (has_definition): Fall back to DECL_INITIALIZED_P when DECL_INITIAL is not set on a template. (module_state::write_inits): Only increment count when initializers are actually written. gcc/testsuite/ChangeLog: * g++.dg/modules/var-tpl-2_a.H: New test. * g++.dg/modules/var-tpl-2_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 8 +++++--- gcc/testsuite/g++.dg/modules/var-tpl-2_a.H | 10 ++++++++++ gcc/testsuite/g++.dg/modules/var-tpl-2_b.C | 10 ++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_a.H create mode 100644 gcc/testsuite/g++.dg/modules/var-tpl-2_b.C