* [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] @ 2023-11-22 11:33 Nathaniel Shead 2023-12-16 10:50 ` [PATCH] c++/modules: Prevent overwriting arguments when merging " Nathaniel Shead 2024-01-06 22:32 ` [PATCH] c++/modules: Prevent overwriting arguments for " Nathan Sidwell 0 siblings, 2 replies; 7+ messages in thread From: Nathaniel Shead @ 2023-11-22 11:33 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] c++/modules: Prevent overwriting arguments when merging duplicates [PR112588] 2023-11-22 11:33 [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] Nathaniel Shead @ 2023-12-16 10:50 ` Nathaniel Shead 2024-01-02 22:49 ` Nathaniel Shead 2024-01-06 22:32 ` [PATCH] c++/modules: Prevent overwriting arguments for " Nathan Sidwell 1 sibling, 1 reply; 7+ messages in thread From: Nathaniel Shead @ 2023-12-16 10:50 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++/modules: Prevent overwriting arguments when merging duplicates [PR112588] 2023-12-16 10:50 ` [PATCH] c++/modules: Prevent overwriting arguments when merging " Nathaniel Shead @ 2024-01-02 22:49 ` Nathaniel Shead 0 siblings, 0 replies; 7+ messages in thread From: Nathaniel Shead @ 2024-01-02 22:49 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] 2023-11-22 11:33 [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] Nathaniel Shead 2023-12-16 10:50 ` [PATCH] c++/modules: Prevent overwriting arguments when merging " Nathaniel Shead @ 2024-01-06 22:32 ` Nathan Sidwell 2024-01-07 14:29 ` Nathaniel Shead 1 sibling, 1 reply; 7+ messages in thread From: Nathan Sidwell @ 2024-01-06 22:32 UTC (permalink / raw) To: Nathaniel Shead, gcc-patches; +Cc: Jason Merrill 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] 2024-01-06 22:32 ` [PATCH] c++/modules: Prevent overwriting arguments for " Nathan Sidwell @ 2024-01-07 14:29 ` Nathaniel Shead 2024-01-08 17:04 ` Patrick Palka 0 siblings, 1 reply; 7+ messages in thread From: Nathaniel Shead @ 2024-01-07 14:29 UTC (permalink / raw) To: Nathan Sidwell; +Cc: gcc-patches, Jason Merrill, Patrick Palka 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] 2024-01-07 14:29 ` Nathaniel Shead @ 2024-01-08 17:04 ` Patrick Palka 2024-01-17 16:28 ` Jason Merrill 0 siblings, 1 reply; 7+ messages in thread From: Patrick Palka @ 2024-01-08 17:04 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Nathan Sidwell, gcc-patches, Jason Merrill, 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 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] 2024-01-08 17:04 ` Patrick Palka @ 2024-01-17 16:28 ` Jason Merrill 0 siblings, 0 replies; 7+ messages in thread From: Jason Merrill @ 2024-01-17 16:28 UTC (permalink / raw) To: Patrick Palka, Nathaniel Shead; +Cc: Nathan Sidwell, gcc-patches 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 >>> >> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-01-17 16:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-22 11:33 [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] Nathaniel Shead 2023-12-16 10:50 ` [PATCH] c++/modules: Prevent overwriting arguments when merging " Nathaniel Shead 2024-01-02 22:49 ` Nathaniel Shead 2024-01-06 22:32 ` [PATCH] c++/modules: Prevent overwriting arguments for " Nathan Sidwell 2024-01-07 14:29 ` Nathaniel Shead 2024-01-08 17:04 ` Patrick Palka 2024-01-17 16:28 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).