| Message ID | 668b6ef5.170a0220.fa269.4683@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: Keep entity mapping info across duplicate_decls [PR99241] | expand |
On Mon, 8 Jul 2024, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14? > > -- >8 -- > > When duplicate_decls finds a match with an existing imported > declaration, it clears DECL_LANG_SPECIFIC of the olddecl and replaces it > with the contents of newdecl; this clears DECL_MODULE_ENTITY_P causing > an ICE if the same declaration is imported again later. > > This fixes the issue by ensuring that the flag is transferred to newdecl > before clearing so that it ends up on olddecl again. > > For future-proofing we also do the same with DECL_MODULE_KEYED_DECLS_P, > though because we don't yet support textual redefinition merging we > can't yet test this works as intended. I don't expect it's possible for > a new declaration already to have extra keyed decls mismatching that of > the old declaration though, so I don't do anything with 'keyed_map' at > this time. Makes sense, thanks for tracking this down! > > PR c++/99241 > > gcc/cp/ChangeLog: > > * decl.cc (duplicate_decls): Merge module entity information. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/pr99241_a.H: New test. > * g++.dg/modules/pr99241_b.H: New test. > * g++.dg/modules/pr99241_c.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/decl.cc | 17 +++++++++++++++++ > gcc/testsuite/g++.dg/modules/pr99241_a.H | 3 +++ > gcc/testsuite/g++.dg/modules/pr99241_b.H | 3 +++ > gcc/testsuite/g++.dg/modules/pr99241_c.C | 5 +++++ > 4 files changed, 28 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/modules/pr99241_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/pr99241_b.H > create mode 100644 gcc/testsuite/g++.dg/modules/pr99241_c.C > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 29616100cfe..b3a770df926 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -3149,6 +3149,23 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden) > if (TREE_CODE (newdecl) == FIELD_DECL) > DECL_PACKED (olddecl) = DECL_PACKED (newdecl); > > + /* Merge module entity mapping information. */ > + if (modules_p ()) > + { > + tree old_nontmpl = STRIP_TEMPLATE (olddecl); It seems duplicate_decls has an unconditional early exit code path for TEMPLATE_DECL if (TREE_CODE (newdecl) == TEMPLATE_DECL) { tree old_result = DECL_TEMPLATE_RESULT (olddecl); ... return olddecl; } so I think these STRIP_TEMPLATEs are unneeded? (And I guess this early exit explains why the testcase doesn't ICE when 'terminate' is a function template.) > + if (DECL_LANG_SPECIFIC (old_nontmpl) > + && (DECL_MODULE_ENTITY_P (old_nontmpl) > + || DECL_MODULE_KEYED_DECLS_P (old_nontmpl))) > + { > + tree new_nontmpl = STRIP_TEMPLATE (newdecl); > + retrofit_lang_decl (new_nontmpl); > + DECL_MODULE_ENTITY_P (new_nontmpl) > + = DECL_MODULE_ENTITY_P (old_nontmpl); > + DECL_MODULE_KEYED_DECLS_P (new_nontmpl) > + = DECL_MODULE_KEYED_DECLS_P (old_nontmpl); > + } > + } > + > /* The DECL_LANG_SPECIFIC information in OLDDECL will be replaced > with that from NEWDECL below. */ > if (DECL_LANG_SPECIFIC (olddecl)) > diff --git a/gcc/testsuite/g++.dg/modules/pr99241_a.H b/gcc/testsuite/g++.dg/modules/pr99241_a.H > new file mode 100644 > index 00000000000..c7031f08eb5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr99241_a.H > @@ -0,0 +1,3 @@ > +// { dg-additional-options "-fmodule-header" } > +// { dg-module-cmi {} } > +void terminate(); > diff --git a/gcc/testsuite/g++.dg/modules/pr99241_b.H b/gcc/testsuite/g++.dg/modules/pr99241_b.H > new file mode 100644 > index 00000000000..c7031f08eb5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr99241_b.H > @@ -0,0 +1,3 @@ > +// { dg-additional-options "-fmodule-header" } > +// { dg-module-cmi {} } > +void terminate(); > diff --git a/gcc/testsuite/g++.dg/modules/pr99241_c.C b/gcc/testsuite/g++.dg/modules/pr99241_c.C > new file mode 100644 > index 00000000000..7f2b1bb43ea > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr99241_c.C > @@ -0,0 +1,5 @@ > +// { dg-additional-options "-fmodules-ts -fno-module-lazy" } > + > +import "pr99241_a.H"; > +void terminate(); > +import "pr99241_b.H"; > -- > 2.43.2 > >
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index 29616100cfe..b3a770df926 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -3149,6 +3149,23 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden) if (TREE_CODE (newdecl) == FIELD_DECL) DECL_PACKED (olddecl) = DECL_PACKED (newdecl); + /* Merge module entity mapping information. */ + if (modules_p ()) + { + tree old_nontmpl = STRIP_TEMPLATE (olddecl); + if (DECL_LANG_SPECIFIC (old_nontmpl) + && (DECL_MODULE_ENTITY_P (old_nontmpl) + || DECL_MODULE_KEYED_DECLS_P (old_nontmpl))) + { + tree new_nontmpl = STRIP_TEMPLATE (newdecl); + retrofit_lang_decl (new_nontmpl); + DECL_MODULE_ENTITY_P (new_nontmpl) + = DECL_MODULE_ENTITY_P (old_nontmpl); + DECL_MODULE_KEYED_DECLS_P (new_nontmpl) + = DECL_MODULE_KEYED_DECLS_P (old_nontmpl); + } + } + /* The DECL_LANG_SPECIFIC information in OLDDECL will be replaced with that from NEWDECL below. */ if (DECL_LANG_SPECIFIC (olddecl)) diff --git a/gcc/testsuite/g++.dg/modules/pr99241_a.H b/gcc/testsuite/g++.dg/modules/pr99241_a.H new file mode 100644 index 00000000000..c7031f08eb5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99241_a.H @@ -0,0 +1,3 @@ +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } +void terminate(); diff --git a/gcc/testsuite/g++.dg/modules/pr99241_b.H b/gcc/testsuite/g++.dg/modules/pr99241_b.H new file mode 100644 index 00000000000..c7031f08eb5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99241_b.H @@ -0,0 +1,3 @@ +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } +void terminate(); diff --git a/gcc/testsuite/g++.dg/modules/pr99241_c.C b/gcc/testsuite/g++.dg/modules/pr99241_c.C new file mode 100644 index 00000000000..7f2b1bb43ea --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99241_c.C @@ -0,0 +1,5 @@ +// { dg-additional-options "-fmodules-ts -fno-module-lazy" } + +import "pr99241_a.H"; +void terminate(); +import "pr99241_b.H";
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14? -- >8 -- When duplicate_decls finds a match with an existing imported declaration, it clears DECL_LANG_SPECIFIC of the olddecl and replaces it with the contents of newdecl; this clears DECL_MODULE_ENTITY_P causing an ICE if the same declaration is imported again later. This fixes the issue by ensuring that the flag is transferred to newdecl before clearing so that it ends up on olddecl again. For future-proofing we also do the same with DECL_MODULE_KEYED_DECLS_P, though because we don't yet support textual redefinition merging we can't yet test this works as intended. I don't expect it's possible for a new declaration already to have extra keyed decls mismatching that of the old declaration though, so I don't do anything with 'keyed_map' at this time. PR c++/99241 gcc/cp/ChangeLog: * decl.cc (duplicate_decls): Merge module entity information. gcc/testsuite/ChangeLog: * g++.dg/modules/pr99241_a.H: New test. * g++.dg/modules/pr99241_b.H: New test. * g++.dg/modules/pr99241_c.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/decl.cc | 17 +++++++++++++++++ gcc/testsuite/g++.dg/modules/pr99241_a.H | 3 +++ gcc/testsuite/g++.dg/modules/pr99241_b.H | 3 +++ gcc/testsuite/g++.dg/modules/pr99241_c.C | 5 +++++ 4 files changed, 28 insertions(+) create mode 100644 gcc/testsuite/g++.dg/modules/pr99241_a.H create mode 100644 gcc/testsuite/g++.dg/modules/pr99241_b.H create mode 100644 gcc/testsuite/g++.dg/modules/pr99241_c.C