[PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588]

[PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588]

[Nathaniel Shead]

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 <nathanieloshead@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>;
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;
-- 
2.42.0

[PATCH] c++/modules: Prevent overwriting arguments when merging duplicates [PR112588]

[Nathaniel Shead]

Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637768.html.
(I've changed the summary message a little from that email but the patch
is otherwise unchanged.)

On Wed, Nov 22, 2023 at 10:33:15PM +1100, 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 <nathanieloshead@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>;
> 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;
> -- 
> 2.42.0
> 

Re: [PATCH] c++/modules: Prevent overwriting arguments when merging duplicates [PR112588]

[Nathaniel Shead]

Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637768.html.

On Sat, Dec 16, 2023 at 09:50:10PM +1100, Nathaniel Shead wrote:
> Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637768.html.
> (I've changed the summary message a little from that email but the patch
> is otherwise unchanged.)
> 
> On Wed, Nov 22, 2023 at 10:33:15PM +1100, 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 <nathanieloshead@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>;
> > 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;
> > -- 
> > 2.42.0
> > 

Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588]

[Nathan Sidwell]

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?

nathan

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 <nathanieloshead@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>;
> 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

Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588]

[Nathaniel Shead]

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?
> 
> nathan

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.

Nathaniel

> 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 <nathanieloshead@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>;
> > 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
> 

Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588]

[Patrick Palka]

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?
> > 
> > nathan
> 
> 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)

> 
> Nathaniel
> 
> > 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 <nathanieloshead@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
> > 
> 
> 

Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588]

[Jason Merrill]

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 <nathanieloshead@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
>>>
>>
>>
>