On 1/8/24 12:04, Patrick Palka wrote:
On Mon, 8 Jan 2024, Nathaniel Shead wrote:
On Sat, Jan 06, 2024 at 05:32:37PM -0500, Nathan Sidwell wrote:
I;m not sure about this, there was clearly a reason I did it the way it is,
but perhaps that reasoning became obsolete -- something about an existing
declaration and reading in a definition maybe?
So I took a bit of a closer look and this is actually a regression,
seeming to start with r13-3134-g09df0d8b14dda6. I haven't looked more
closely at the actual change though to see whether this implies a
different fix yet though.
Interesting.. FWIW I applied your patch to the gcc 12 release branch,
which doesn't have r13-3134, and there were no modules testsuite
regressions there either, which at least suggests that this maybe_dup
logic isn't directly related to the optimization that r13-3134 removed.
Your patch also seems to fix PR99244 (which AFAICT is not a regression)
It seems to me we always want the DECL_ARGUMENTS corresponding to the
actual definition we're using, which since "installing" is true, is the
new definition. In duplicate_decls when we merge a new definition into
an old declaration, we give the old declaration the new DECL_ARGUMENTS.
The patch is OK.
On 11/22/23 06:33, Nathaniel Shead wrote:
Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write
access.
-- >8 --
When merging duplicate instantiations of function templates, currently
read_function_def overwrites the arguments with that of the existing
duplicate. This is problematic, however, since this means that the
PARM_DECLs in the body of the function definition no longer match with
the PARM_DECLs in the argument list, which causes issues when it comes
to generating RTL.
There doesn't seem to be any reason to do this replacement, so this
patch removes that logic.
PR c++/112588
gcc/cp/ChangeLog:
* module.cc (trees_in::read_function_def): Don't overwrite
arguments.
gcc/testsuite/ChangeLog:
* g++.dg/modules/merge-16.h: New test.
* g++.dg/modules/merge-16_a.C: New test.
* g++.dg/modules/merge-16_b.C: New test.
Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
---
gcc/cp/module.cc | 2 --
gcc/testsuite/g++.dg/modules/merge-16.h | 10 ++++++++++
gcc/testsuite/g++.dg/modules/merge-16_a.C | 7 +++++++
gcc/testsuite/g++.dg/modules/merge-16_b.C | 5 +++++
4 files changed, 22 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/merge-16.h
create mode 100644 gcc/testsuite/g++.dg/modules/merge-16_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/merge-16_b.C
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 4f5b6e2747a..2520ab659cc 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -11665,8 +11665,6 @@ trees_in::read_function_def (tree decl, tree
maybe_template)
DECL_RESULT (decl) = result;
DECL_INITIAL (decl) = initial;
DECL_SAVED_TREE (decl) = saved;
- if (maybe_dup)
- DECL_ARGUMENTS (decl) = DECL_ARGUMENTS (maybe_dup);
if (context)
SET_DECL_FRIEND_CONTEXT (decl, context);
diff --git a/gcc/testsuite/g++.dg/modules/merge-16.h
b/gcc/testsuite/g++.dg/modules/merge-16.h
new file mode 100644
index 00000000000..fdb38551103
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/merge-16.h
@@ -0,0 +1,10 @@
+// PR c++/112588
+
+void f(int*);
+
+template <typename T>
+struct S {
+ void g(int n) { f(&n); }
+};
+
+template struct S<void>;
If we use a partial specialization here instead (which would have disabled
the removed optimization, demonstrating how fragile/inconsistent it was)
void f(int*);
template <typename T>
struct S { };
template<typename T>
struct S<T*> {
void g(int n) { f(&n); }
};
template struct S<void*>;
then the ICE appears earlier, since GCC 12 instead of 13.
diff --git a/gcc/testsuite/g++.dg/modules/merge-16_a.C
b/gcc/testsuite/g++.dg/modules/merge-16_a.C
new file mode 100644
index 00000000000..c243224c875
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/merge-16_a.C
@@ -0,0 +1,7 @@
+// PR c++/112588
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi merge16 }
+
+module;
+#include "merge-16.h"
+export module merge16;
diff --git a/gcc/testsuite/g++.dg/modules/merge-16_b.C
b/gcc/testsuite/g++.dg/modules/merge-16_b.C
new file mode 100644
index 00000000000..8c7b1f0511f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/merge-16_b.C
@@ -0,0 +1,5 @@
+// PR c++/112588
+// { dg-additional-options "-fmodules-ts" }
+
+#include "merge-16.h"
+import merge16;
--
Nathan Sidwell
