On Fri, 23 Aug 2024, Nathaniel Shead wrote:
> On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote:
> > On Mon, 12 Aug 2024, Nathaniel Shead wrote:
> >
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > >
> > > I tried to implement a remapping of the slots for TARGET_EXPRs for the
> > > FIXME but I wasn't able to work out how to do so effectively. Given
> > > that I doubt this will be a common issue I felt probably easiest to
> > > leave it for now and focus on other issues in the meantime; thoughts?
> > >
> > > The other thing to note is that most of this function just has a single
> > > error message always indicated by a 'goto mismatch;' but I felt that it
> > > seemed reasonable to provide more specific error messages where we can.
> > > But given that in the long term we probably want to replace this
> > > function with an appropriately enhanced 'duplicate_decls' anyway maybe
> > > it's not worth worrying about; this patch is still useful in the
> > > meantime if only for the testcases, I hope.
> > >
> > > -- >8 --
> > >
> > > When merging a newly imported declaration with an existing declaration
> > > we don't currently propagate new default arguments, which causes issues
> > > when modularising header units. This patch adds logic to propagate
> > > default arguments to existing declarations on import, and error if the
> > > defaults do not match.
> > >
> > > PR c++/99274
> > >
> > > gcc/cp/ChangeLog:
> > >
> > > * module.cc (trees_in::is_matching_decl): Merge default
> > > arguments.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * g++.dg/modules/default-arg-1_a.H: New test.
> > > * g++.dg/modules/default-arg-1_b.C: New test.
> > > * g++.dg/modules/default-arg-2_a.H: New test.
> > > * g++.dg/modules/default-arg-2_b.C: New test.
> > > * g++.dg/modules/default-arg-3.h: New test.
> > > * g++.dg/modules/default-arg-3_a.H: New test.
> > > * g++.dg/modules/default-arg-3_b.C: New test.
> > >
> > > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > > ---
> > > gcc/cp/module.cc | 62 ++++++++++++++++++-
> > > .../g++.dg/modules/default-arg-1_a.H | 17 +++++
> > > .../g++.dg/modules/default-arg-1_b.C | 26 ++++++++
> > > .../g++.dg/modules/default-arg-2_a.H | 17 +++++
> > > .../g++.dg/modules/default-arg-2_b.C | 28 +++++++++
> > > gcc/testsuite/g++.dg/modules/default-arg-3.h | 13 ++++
> > > .../g++.dg/modules/default-arg-3_a.H | 5 ++
> > > .../g++.dg/modules/default-arg-3_b.C | 6 ++
> > > 8 files changed, 171 insertions(+), 3 deletions(-)
> > > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> > > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> > > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> > > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> > > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3.h
> > > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> > > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> > >
> > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > > index f4d137b13a1..87f34bac578 100644
> > > --- a/gcc/cp/module.cc
> > > +++ b/gcc/cp/module.cc
> > > @@ -11551,8 +11551,6 @@ trees_in::is_matching_decl (tree existing, tree
> > > decl, bool is_typedef)
> > >
> > > if (!same_type_p (TREE_VALUE (d_args), TREE_VALUE (e_args)))
> > > goto mismatch;
> > > -
> > > - // FIXME: Check default values
> > > }
> > >
> > > /* If EXISTING has an undeduced or uninstantiated exception
> > > @@ -11690,7 +11688,65 @@ trees_in::is_matching_decl (tree existing, tree
> > > decl, bool is_typedef)
> > > if (!DECL_EXTERNAL (d_inner))
> > > DECL_EXTERNAL (e_inner) = false;
> > >
> > > - // FIXME: Check default tmpl and fn parms here
> > > + if (TREE_CODE (decl) == TEMPLATE_DECL)
> > > + {
> > > + /* Merge default template arguments. */
> > > + tree d_parms = DECL_INNERMOST_TEMPLATE_PARMS (decl);
> > > + tree e_parms = DECL_INNERMOST_TEMPLATE_PARMS (existing);
> > > + gcc_checking_assert (TREE_VEC_LENGTH (d_parms)
> > > + == TREE_VEC_LENGTH (e_parms));
> > > + for (int i = 0; i < TREE_VEC_LENGTH (d_parms); ++i)
> > > + {
> > > + tree d_default = TREE_PURPOSE (TREE_VEC_ELT (d_parms, i));
> > > + tree& e_default = TREE_PURPOSE (TREE_VEC_ELT (e_parms, i));
> > > + if (e_default == NULL_TREE)
> > > + e_default = d_default;
> > > + else if (d_default != NULL_TREE
> > > + && !cp_tree_equal (d_default, e_default))
> > > + {
> > > + auto_diagnostic_group d;
> > > + tree d_parm = TREE_VALUE (TREE_VEC_ELT (d_parms, i));
> > > + tree e_parm = TREE_VALUE (TREE_VEC_ELT (e_parms, i));
> > > + error_at (DECL_SOURCE_LOCATION (d_parm),
> > > + "conflicting default argument for %#qD", d_parm);
> > > + inform (DECL_SOURCE_LOCATION (e_parm),
> > > + "existing default declared here");
> > > + return false;
> > > + }
> > > + }
> > > + }
> >
> > FWIW for ordinary textual redeclarations we merge default template
> > arguments via merge_default_template_args. But the semantics are
> > slightly different on stream in because we want to allow a default
> > argument redefinition as long as it's cp_tree_equal, so I don't think
> > we can or want to use merge_default_template_args here.
> >
> > > +
> > > + if (TREE_CODE (d_inner) == FUNCTION_DECL)
> > > + {
> > > + /* Merge default function arguments. */
> > > + tree d_parm = FUNCTION_FIRST_USER_PARMTYPE (d_inner);
> > > + tree e_parm = FUNCTION_FIRST_USER_PARMTYPE (e_inner);
> > > + int i = 0;
> > > + for (; d_parm && d_parm != void_list_node;
> > > + d_parm = TREE_CHAIN (d_parm), e_parm = TREE_CHAIN (e_parm), ++i)
> > > + {
> > > + tree d_default = TREE_PURPOSE (d_parm);
> > > + tree& e_default = TREE_PURPOSE (e_parm);
> > > + if (e_default == NULL_TREE)
> > > + e_default = d_default;
> > > + else if (d_default != NULL_TREE
> > > + && !cp_tree_equal (d_default, e_default)
> > > + /* FIXME: We need to remap the target to match the existing
> > > + parms so that cp_tree_equal works correctly, as with
> > > + break_out_target_exprs. */
> >
> > Is the problem that cp_tree_equal expects TARGET_EXPR_SLOT to be
> > identical (despite the probably bitrotted special case for empty
> > DECL_NAME)? Maybe it'd make sense to always only compare
> > TARGET_EXPR_INITIAL, not TARGET_EXPR_SLOT..
> >
>
> It's actually the opposite: the special case for empty DECL_NAME does
> kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL.
> But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the
> contents of the slot is an AGGR_INIT_EXPR which depends on the variable
> declared in the TARGET_EXPR_SLOT.
>
> So in this case within the default-arg-3 testcase, d_default is
> (some details elided):
>
> <target_expr 0x7ffff74ea7e0
> type <record_type 0x7ffff76b70a8 nontrivial ...>
> side-effects tree_0
> arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8
> nontrivial> ...>
> arg:1 <aggr_init_expr 0x7ffff76b1f40
> type <void_type 0x7ffff7512f18 void ...>
> side-effects tree_0
> arg:0 <integer_cst 0x7ffff76ae468 constant 5>
> arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28>
> constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp
> >> arg:2 <var_decl 0x7ffff76bb000 D.2886>
> arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8>
> ...>
> arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type
> 0x7ffff75125e8 int>
> constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123>
> ...
>
> While e_default is
>
> <target_expr 0x7ffff74ea7a8
> type <record_type 0x7ffff76b70a8 nontrivial ...>
> side-effects tree_0
> arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8
> nontrivial> ...>
> arg:1 <aggr_init_expr 0x7ffff76b1ac0
> type <void_type 0x7ffff7512f18 void ...>
> side-effects tree_0
> arg:0 <integer_cst 0x7ffff76ae468 constant 5>
> arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28>
> constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp
> >> arg:2 <var_decl 0x7ffff74faea0 D.2860>
> arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8>
> ...>
> arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type
> 0x7ffff75125e8 int>
> constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123>
> ...
>
> -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the
> AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different.
> I imagine probably we would need to merge the VAR_DECLs here but in the
> presence of nested TARGET_EXPRs that started to get hard to figure out,
> and at some point I found I was just reimplementing a limited version of
> cp_tree_equal with an additional map so I decided to punt on it for now.
>
> Maybe a better option then would be to add a parameter to cp_tree_equal
> with an optional hash map that it could use to treat certain decls as
> 'equal' (even if they weren't), used within this special handling for
> TARGET_EXPRs, but that seems overkill for what is ultimately a pretty
> rare error case IMO.
Ah, makes sense, agreed FWIW
>
> > The workaround seems fine with me though, LGTM.
> >
> > > + && !(TREE_CODE (d_default) == TARGET_EXPR
> > > + && TREE_CODE (e_default) == TARGET_EXPR))
> > > + {
> > > + auto_diagnostic_group d;
> > > + error_at (get_fndecl_argument_location (d_inner, i),
> > > + "conflicting default argument for parameter %P of %#qD",
> > > + i, decl);
> > > + inform (get_fndecl_argument_location (e_inner, i),
> > > + "existing default declared here");
> > > + return false;
> > > + }
> > > + }
> > > + }
> > >
> > > return true;
> > > }
> > > diff --git a/gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> > > b/gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> > > new file mode 100644
> > > index 00000000000..65334930f74
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> > > @@ -0,0 +1,17 @@
> > > +// PR c++/99274
> > > +// { dg-additional-options "-fmodule-header" }
> > > +// { dg-module-cmi {} }
> > > +
> > > +void f(int a, int b = 123);
> > > +template <typename T> void g(T a, T b = {});
> > > +
> > > +template <typename T, typename U = int> struct A;
> > > +template <typename T, int N = 5> struct B;
> > > +
> > > +struct S {
> > > + template <typename T = int> void x();
> > > + void y(int n = 123);
> > > +};
> > > +
> > > +struct nontrivial { nontrivial(int); };
> > > +void h(nontrivial p = nontrivial(123));
> > > diff --git a/gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> > > b/gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> > > new file mode 100644
> > > index 00000000000..80822896de1
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> > > @@ -0,0 +1,26 @@
> > > +// PR c++/99274
> > > +// { dg-additional-options "-fmodules-ts" }
> > > +// Propagate default args from import to existing decls
> > > +
> > > +void f(int a, int b);
> > > +template <typename T> void g(T a, T b);
> > > +template <typename T, typename U> struct A;
> > > +template <typename T, int N> struct B;
> > > +struct S {
> > > + template <typename T = int> void x();
> > > + void y(int n = 123);
> > > +};
> > > +struct nontrivial { nontrivial(int); };
> > > +void h(nontrivial p);
> > > +
> > > +import "default-arg-1_a.H";
> > > +
> > > +template <typename = A<int>, typename = B<int>> struct Q;
> > > +
> > > +int main() {
> > > + f(1);
> > > + g(2);
> > > + h();
> > > + S{}.x();
> > > + S{}.y();
> > > +}
> > > diff --git a/gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> > > b/gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> > > new file mode 100644
> > > index 00000000000..085d4c7f792
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> > > @@ -0,0 +1,17 @@
> > > +// PR c++/99274
> > > +// { dg-additional-options "-fmodule-header" }
> > > +// { dg-module-cmi {} }
> > > +
> > > +void f(int a, int b = 123);
> > > +template <typename T> void g(T a, T b = 123);
> > > +
> > > +template <typename U = int> struct A;
> > > +template <int N = 123> struct B;
> > > +
> > > +struct S {
> > > + template <typename T = int> void x();
> > > + void y(int n = 123);
> > > +};
> > > +
> > > +struct nontrivial { nontrivial(int); };
> > > +void h(nontrivial p = nontrivial(123));
> > > diff --git a/gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> > > b/gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> > > new file mode 100644
> > > index 00000000000..a6cc58738c2
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> > > @@ -0,0 +1,28 @@
> > > +// PR c++/99274
> > > +// { dg-additional-options "-fmodules-ts -fno-module-lazy" }
> > > +// Check for conflicting defaults.
> > > +
> > > +void f(int a, int b = 456); // { dg-message "existing default declared
> > > here" }
> > > +
> > > +template <typename T>
> > > +void g(T a, T b = {}); // { dg-message "existing default declared here"
> > > }
> > > +
> > > +template <typename U = double> // { dg-message "existing default
> > > declared here" }
> > > +struct A;
> > > +
> > > +template <int N = 456> // { dg-message "existing default declared here"
> > > }
> > > +struct B;
> > > +
> > > +struct S {
> > > + template <typename T = double> // { dg-message "existing default
> > > declared here" }
> > > + void x();
> > > +
> > > + void y(int n = 456); // { dg-message "existing default declared here"
> > > }
> > > +};
> > > +
> > > +struct nontrivial { nontrivial(int); };
> > > +void h(nontrivial p = nontrivial(456)); // { dg-message "existing
> > > default declared here" "" { xfail *-*-* } }
> > > +
> > > +import "default-arg-2_a.H";
> > > +
> > > +// { dg-error "conflicting default argument" "" { target *-*-* } 0 }
> > > diff --git a/gcc/testsuite/g++.dg/modules/default-arg-3.h
> > > b/gcc/testsuite/g++.dg/modules/default-arg-3.h
> > > new file mode 100644
> > > index 00000000000..b55a6690f4a
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/default-arg-3.h
> > > @@ -0,0 +1,13 @@
> > > +void f(int a, int b = 123);
> > > +template <typename T> void g(T a, T b = 123);
> > > +
> > > +template <typename U = int> struct A;
> > > +template <int N = 123> struct B;
> > > +
> > > +struct S {
> > > + template <typename T = int> void x();
> > > + void y(int n = 123);
> > > +};
> > > +
> > > +struct nontrivial { nontrivial(int); };
> > > +void h(nontrivial p = nontrivial(123));
> > > diff --git a/gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> > > b/gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> > > new file mode 100644
> > > index 00000000000..879bedce67f
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> > > @@ -0,0 +1,5 @@
> > > +// PR c++/99274
> > > +// { dg-additional-options "-fmodule-header" }
> > > +// { dg-module-cmi {} }
> > > +
> > > +#include "default-arg-3.h"
> > > diff --git a/gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> > > b/gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> > > new file mode 100644
> > > index 00000000000..402dca6ff76
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> > > @@ -0,0 +1,6 @@
> > > +// PR c++/99274
> > > +// { dg-additional-options "-fmodules-ts -fno-module-lazy" }
> > > +// Don't complain about matching defaults.
> > > +
> > > +#include "default-arg-3.h"
> > > +import "default-arg-3_a.H";
> > > --
> > > 2.43.2
> > >
> > >
> >
>
>
