On 8/8/24 1:37 PM, Marek Polacek wrote:
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?-- >8 -- The problem in this PR is that we ended up with {.rows=(&<PLACEHOLDER_EXPR struct Widget>)->n, .outer_stride=(&<PLACEHOLDER_EXPR struct MatrixLayout>)->rows} that is, two PLACEHOLDER_EXPRs for different types on the same level in one { }. That should not happen; we may, for instance, neglect to replace a PLACEHOLDER_EXPR due to CONSTRUCTOR_PLACEHOLDER_BOUNDARY on the constructor. The same problem happened in PR100252, which I fixed by introducing replace_placeholders_for_class_temp_r. That didn't work here, though, because r_p_for_c_t_r only works for non-eliding TARGET_EXPRs: replacing a PLACEHOLDER_EXPR with a temporary that is going to be elided will result in a crash in gimplify_var_or_parm_decl when it encounters such a loose decl. But leaving the PLACEHOLDER_EXPRs in is also bad because then we end up with this PR. TARGET_EXPRs for function arguments are elided in gimplify_arg. The argument will get a real temporary only in get_formal_tmp_var. One idea was to use the temporary that is going to be elided anyway, and then replace_decl it with the real object once we get it. But that didn't work out: one problem is that we elide the TARGET_EXPR for an argument before we create the real temporary for the argument, and when we get it, the context that this was a TARGET_EXPR for an argument has been lost. We're also in the middle end territory now, even though this is a C++-specific problem.
How complex!
I figured that since the to-be-elided temporary is going to stay around until gimplification, the front end is free to use it. Once we're done with things like store_init_value, which replaces PLACEHOLDER_EXPRs with the decl it is initializing, we can turn those to-be-elided temporaries into PLACEHOLDER_EXPRs again, so that cp_gimplify_init_expr can replace them with the real object once available. The context is not lost so we do not need an extra flag for these makeshift temporaries.
Clever, that makes a lot of sense. But I wonder if we can avoid the problem more simply than working around it?
I see that the get_formal_tmp_var happens directly from gimplify_arg, so it strips the TARGET_EXPR to avoid a temporary...and then immediately turns around and creates a new temporary.
Would it work to stop stripping the TARGET_EXPR in gimplify_arg and therefore stop setting TARGET_EXPR_ELIDING_P in convert_for_arg_passing?
PR c++/116015 gcc/cp/ChangeLog: * cp-gimplify.cc (replace_argument_temps_with_placeholders): New. (cp_genericize_r) <case CALL_EXPR>: Call it. * typeck2.cc (replace_placeholders_for_class_temp_r): Do replace placeholders in TARGET_EXPRs for function arguments. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/nsdmi-aggr23.C: New test. --- gcc/cp/cp-gimplify.cc | 24 +++++++++++++++++++++ gcc/cp/typeck2.cc | 23 ++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr23.C | 26 +++++++++++++++++++++++ 3 files changed, 73 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr23.C diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc index 003e68f1ea7..7f203cd6804 100644 --- a/gcc/cp/cp-gimplify.cc +++ b/gcc/cp/cp-gimplify.cc @@ -1597,6 +1597,29 @@ predeclare_vla (tree expr) } }+/* Replace the temporaries used for function arguments with PLACEHOLDER_EXPRs+ so that they can be replaced again. See the comment in + replace_placeholders_for_class_temp_r for more context. CALL is either + the CALL_EXPR or AGGR_INIT_EXPR whose arguments we're going to adjust. */ + +static void +replace_argument_temps_with_placeholders (tree call) +{ + for (int i = 0; i < call_expr_nargs (call); ++i) + { + tree arg = get_nth_callarg (call, i); + if (TREE_CODE (arg) == TARGET_EXPR && TARGET_EXPR_ELIDING_P (arg)) + { + tree slot = TARGET_EXPR_SLOT (arg); + tree placeholder = build0 (PLACEHOLDER_EXPR, TREE_TYPE (slot)); + tree &init = TARGET_EXPR_INITIAL (arg); + if (replace_decl (&init, slot, placeholder) + && TREE_CODE (init) == CONSTRUCTOR) + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = true; + } + } +} + /* Perform any pre-gimplification lowering of C++ front end trees to GENERIC. */@@ -2125,6 +2148,7 @@ cp_genericize_r (tree *stmt_p, int *walk_subtrees, void *data)version is inlinable, a direct call to this version can be made otherwise the call should go through the dispatcher. */ { + replace_argument_temps_with_placeholders (stmt); tree fn = cp_get_callee_fndecl_nofold (stmt); if (fn && DECL_FUNCTION_VERSIONED (fn) && (current_function_decl == NULL diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc index 30a6fbe95c9..9b6109ac3ff 100644 --- a/gcc/cp/typeck2.cc +++ b/gcc/cp/typeck2.cc @@ -1425,6 +1425,29 @@ replace_placeholders_for_class_temp_r (tree *tp, int *, void *) { tree t = *tp;+ /* If a TARGET_EXPR is an initializer of a function argument, it is+ going to be elided in gimplify_arg. So, we should not be using + its slot to replace the PLACEHOLDER_EXPR. But we will only have + the real object once we've gimplified the argument, which is too + late: we should replace the PLACEHOLDER_EXPR now to avoid winding + up with two different PLACEHOLDER_EXPRs in a single {} (c++/116015). + + We can get away with using the temporary...temporarily, because it's + going to survive at least until genericization, where we replace it + with a PLACEHOLDER_EXPR again, so that it can be replaced with a real + object once the argument has been gimplified. */ + if (TREE_CODE (t) == CALL_EXPR || TREE_CODE (t) == AGGR_INIT_EXPR) + for (int i = 0; i < call_expr_nargs (t); ++i) + { + tree arg = get_nth_callarg (t, i); + if (TREE_CODE (arg) == TARGET_EXPR && TARGET_EXPR_ELIDING_P (arg)) + { + TARGET_EXPR_ELIDING_P (arg) = false; + replace_placeholders_for_class_temp_r (&arg, nullptr, nullptr); + TARGET_EXPR_ELIDING_P (arg) = true; + } + } + /* We're looking for a TARGET_EXPR nested in the whole expression. */ if (TREE_CODE (t) == TARGET_EXPR /* That serves as temporary materialization, not an initializer. */ diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr23.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr23.C new file mode 100644 index 00000000000..2f5b8ca97bf --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr23.C @@ -0,0 +1,26 @@ +// PR c++/116015 +// { dg-do compile { target c++14 } } +// { dg-additional-options "-Wno-c++20-extensions" } + +struct MatrixLayout { + int rows = 0; + int outer_stride = rows; +}; +struct Matrix { + Matrix(MatrixLayout m) {} +}; +struct Widget { + int n = 5; + Matrix A0{{}}; + Matrix A1{{n}}; + Matrix A1_{{.rows = n}}; + Matrix A2{{n, n}}; +}; + +int +main () +{ + Widget w{}; + Widget w1; + Widget w2 = {}; +} base-commit: c4d3dba253b49fb0e8e32109783f76453bc53653
