Ping for https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660134.html.
On Thu, Sep 12, 2024 at 01:35:38PM -0400, Patrick Palka wrote:
> 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
> > > >
> > > >
> > >
> >
> >
>
