Three patches:
1) Rewrite a complete constructor call to call a base constructor if
we're eliding a copy into a base subobject.
2) Elide the copy from a prvalue built for list-initialization into a
base subobject.
3) Elide other copies from prvalues representing a constructor call into
base subobjects.
The first two are clear bugfixes.
The third is a behavior change to elide more copies, as specified by
C++17, but makes constructor prvalues more different from other function
call prvalues, from which we can't consistently elide copies (CWG2403).
Getting closer to the C++17 prvalue model seems desirable, but it's a
departure from the behavior that we and other compilers have settled on.
So perhaps I'll hold off on the last one for now.
Tested x86_64-pc-linux-gnu, applying patches 1 and 2 to trunk.
commit c2afbe403389d32a3f36e35a461beda75d0e82f4
Author: Jason Merrill <ja...@redhat.com>
Date: Wed Jan 13 13:27:53 2021 -0500
c++: Fix copy elision for base initialization
While working on PR98642 I noticed that in this testcase we were eliding the
copy, calling the complete default constructor to initialize the B base
subobject, and therefore wrongly initializing the non-existent A subobject
of B. The test doesn't care whether the copy is elided or not, but checks
that we are actually calling a base constructor for B.
The patch preserves the elision, but changes the initializer to call the
base constructor instead of the complete constructor.
gcc/cp/ChangeLog:
* call.c (base_ctor_for, make_base_init_ok): New.
(build_over_call): Use make_base_init_ok.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1z/elide4.C: New test.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 218157088ef..c194af74612 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8425,6 +8425,60 @@ call_copy_ctor (tree a, tsubst_flags_t complain)
return r;
}
+/* Return the base constructor corresponding to COMPLETE_CTOR or NULL_TREE. */
+
+static tree
+base_ctor_for (tree complete_ctor)
+{
+ tree clone;
+ FOR_EACH_CLONE (clone, DECL_CLONED_FUNCTION (complete_ctor))
+ if (DECL_BASE_CONSTRUCTOR_P (clone))
+ return clone;
+ return NULL_TREE;
+}
+
+/* Try to make EXP suitable to be used as the initializer for a base subobject,
+ and return whether we were successful. EXP must have already been cleared
+ by unsafe_copy_elision_p. */
+
+static bool
+make_base_init_ok (tree exp)
+{
+ if (TREE_CODE (exp) == TARGET_EXPR)
+ exp = TARGET_EXPR_INITIAL (exp);
+ while (TREE_CODE (exp) == COMPOUND_EXPR)
+ exp = TREE_OPERAND (exp, 1);
+ if (TREE_CODE (exp) == COND_EXPR)
+ {
+ bool ret = make_base_init_ok (TREE_OPERAND (exp, 2));
+ if (tree op1 = TREE_OPERAND (exp, 1))
+ {
+ bool r1 = make_base_init_ok (op1);
+ /* If unsafe_copy_elision_p was false, the arms should match. */
+ gcc_assert (r1 == ret);
+ }
+ return ret;
+ }
+ if (TREE_CODE (exp) != AGGR_INIT_EXPR)
+ /* A trivial copy is OK. */
+ return true;
+ if (!AGGR_INIT_VIA_CTOR_P (exp))
+ /* unsafe_copy_elision_p must have said this is OK. */
+ return true;
+ tree fn = cp_get_callee_fndecl_nofold (exp);
+ if (DECL_BASE_CONSTRUCTOR_P (fn))
+ return true;
+ gcc_assert (DECL_COMPLETE_CONSTRUCTOR_P (fn));
+ fn = base_ctor_for (fn);
+ if (!fn || DECL_HAS_IN_CHARGE_PARM_P (fn))
+ /* The base constructor has more parameters, so we can't just change the
+ call target. It would be possible to splice in the appropriate
+ arguments, but probably not worth the complexity. */
+ return false;
+ AGGR_INIT_EXPR_FN (exp) = build_address (fn);
+ return true;
+}
+
/* Return true iff T refers to a base or potentially-overlapping field, which
cannot be used for return by invisible reference. We avoid doing C++17
mandatory copy elision when this is true.
@@ -9152,6 +9206,10 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
else
cp_warn_deprecated_use (fn, complain);
+ if (eliding_temp && DECL_BASE_CONSTRUCTOR_P (fn)
+ && !make_base_init_ok (arg))
+ unsafe = true;
+
/* If we're creating a temp and we already have one, don't create a
new one. If we're not creating a temp but we get one, use
INIT_EXPR to collapse the temp into our target. Otherwise, if the
diff --git a/gcc/testsuite/g++.dg/cpp1z/elide4.C b/gcc/testsuite/g++.dg/cpp1z/elide4.C
new file mode 100644
index 00000000000..03335e4ffbd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/elide4.C
@@ -0,0 +1,24 @@
+// { dg-do compile { target c++11 } }
+
+// Check that there's a call to some base constructor of B: either the default
+// constructor, if the copy is elided, or the copy constructor.
+
+// { dg-final { scan-assembler {call[ \t]*_?_ZN1BC2} { target { i?86-*-* x86_64-*-* } } } }
+
+int count;
+struct A { int i = count++; };
+struct B: virtual A {
+ B() { }
+ B(const B& b);
+};
+bool x;
+struct C: B
+{
+ C() : B(x ? (0,B()) : B()) { }
+};
+
+int main()
+{
+ C c;
+ return count;
+}
commit 8501c4e2dd7fe50066d06de9fa17e7dbe22fb884
Author: Jason Merrill <ja...@redhat.com>
Date: Wed Jan 13 13:27:06 2021 -0500
c++: Avoid redundant copy in {} init [PR98642]
Here, initializing from { } implies a call to the default constructor for
base. We were then seeing that we're initializing a base subobject, so we
tried to copy the result of that call. This is clearly wrong; we should
initialize the base directly from its default constructor.
This patch does a lot of refactoring of unsafe_copy_elision_p and adds
make_safe_copy_elision that will also try to do the base constructor
rewriting from the last patch.
gcc/cp/ChangeLog:
PR c++/98642
* call.c (unsafe_return_slot_p): Return int.
(init_by_return_slot_p): Split out from...
(unsafe_copy_elision_p): ...here.
(unsafe_copy_elision_p_opt): New name for old meaning.
(build_over_call): Adjust.
(make_safe_copy_elision): New.
* typeck2.c (split_nonconstant_init_1): Elide copy from safe
list-initialization.
* cp-tree.h: Adjust.
gcc/testsuite/ChangeLog:
PR c++/98642
* g++.dg/cpp1z/elide5.C: New test.
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index e7b46b7b53b..51139f4a4be 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6471,7 +6471,8 @@ extern bool is_std_init_list (tree);
extern bool is_list_ctor (tree);
extern void validate_conversion_obstack (void);
extern void mark_versions_used (tree);
-extern bool unsafe_return_slot_p (tree);
+extern int unsafe_return_slot_p (tree);
+extern bool make_safe_copy_elision (tree, tree);
extern bool cp_warn_deprecated_use (tree, tsubst_flags_t = tf_warning_or_error);
extern void cp_warn_deprecated_use_scopes (tree);
extern tree get_function_version_dispatcher (tree);
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c194af74612..b6e9f125aeb 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8439,7 +8439,7 @@ base_ctor_for (tree complete_ctor)
/* Try to make EXP suitable to be used as the initializer for a base subobject,
and return whether we were successful. EXP must have already been cleared
- by unsafe_copy_elision_p. */
+ by unsafe_copy_elision_p{,_opt}. */
static bool
make_base_init_ok (tree exp)
@@ -8463,7 +8463,7 @@ make_base_init_ok (tree exp)
/* A trivial copy is OK. */
return true;
if (!AGGR_INIT_VIA_CTOR_P (exp))
- /* unsafe_copy_elision_p must have said this is OK. */
+ /* unsafe_copy_elision_p_opt must have said this is OK. */
return true;
tree fn = cp_get_callee_fndecl_nofold (exp);
if (DECL_BASE_CONSTRUCTOR_P (fn))
@@ -8479,20 +8479,20 @@ make_base_init_ok (tree exp)
return true;
}
-/* Return true iff T refers to a base or potentially-overlapping field, which
- cannot be used for return by invisible reference. We avoid doing C++17
- mandatory copy elision when this is true.
+/* Return 2 if T refers to a base, 1 if a potentially-overlapping field,
+ neither of which can be used for return by invisible reference. We avoid
+ doing C++17 mandatory copy elision for either of these cases.
- This returns true even if the type of T has no tail padding that other data
- could be allocated into, because that depends on the particular ABI.
- unsafe_copy_elision_p, below, does consider whether there is padding. */
+ This returns non-zero even if the type of T has no tail padding that other
+ data could be allocated into, because that depends on the particular ABI.
+ unsafe_copy_elision_p_opt does consider whether there is padding. */
-bool
+int
unsafe_return_slot_p (tree t)
{
/* Check empty bases separately, they don't have fields. */
if (is_empty_base_ref (t))
- return true;
+ return 2;
STRIP_NOPS (t);
if (TREE_CODE (t) == ADDR_EXPR)
@@ -8504,28 +8504,21 @@ unsafe_return_slot_p (tree t)
if (!CLASS_TYPE_P (TREE_TYPE (t)))
/* The middle-end will do the right thing for scalar types. */
return false;
- return (DECL_FIELD_IS_BASE (t)
- || lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)));
+ if (DECL_FIELD_IS_BASE (t))
+ return 2;
+ if (lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
+ return 1;
+ return 0;
}
-/* We can't elide a copy from a function returning by value to a
- potentially-overlapping subobject, as the callee might clobber tail padding.
- Return true iff this could be that case. */
+/* True IFF EXP is a prvalue that represents return by invisible reference. */
static bool
-unsafe_copy_elision_p (tree target, tree exp)
+init_by_return_slot_p (tree exp)
{
/* Copy elision only happens with a TARGET_EXPR. */
if (TREE_CODE (exp) != TARGET_EXPR)
return false;
- tree type = TYPE_MAIN_VARIANT (TREE_TYPE (exp));
- /* It's safe to elide the copy for a class with no tail padding. */
- if (!is_empty_class (type)
- && tree_int_cst_equal (TYPE_SIZE (type), CLASSTYPE_SIZE (type)))
- return false;
- /* It's safe to elide the copy if we aren't initializing a base object. */
- if (!unsafe_return_slot_p (target))
- return false;
tree init = TARGET_EXPR_INITIAL (exp);
/* build_compound_expr pushes COMPOUND_EXPR inside TARGET_EXPR. */
while (TREE_CODE (init) == COMPOUND_EXPR)
@@ -8533,16 +8526,58 @@ unsafe_copy_elision_p (tree target, tree exp)
if (TREE_CODE (init) == COND_EXPR)
{
/* We'll end up copying from each of the arms of the COND_EXPR directly
- into the target, so look at them. */
+ into the target, so look at them. */
if (tree op = TREE_OPERAND (init, 1))
- if (unsafe_copy_elision_p (target, op))
+ if (init_by_return_slot_p (op))
return true;
- return unsafe_copy_elision_p (target, TREE_OPERAND (init, 2));
+ return init_by_return_slot_p (TREE_OPERAND (init, 2));
}
return (TREE_CODE (init) == AGGR_INIT_EXPR
&& !AGGR_INIT_VIA_CTOR_P (init));
}
+/* We can't elide a copy from a function returning by value to a
+ potentially-overlapping subobject, as the callee might clobber tail padding.
+ Return true iff this could be that case.
+
+ Places that use this function (or _opt) to decide to elide a copy should
+ probably use make_safe_copy_elision instead. */
+
+static bool
+unsafe_copy_elision_p (tree target, tree exp)
+{
+ return unsafe_return_slot_p (target) && init_by_return_slot_p (exp);
+}
+
+/* As above, but for optimization allow more cases that are actually safe. */
+
+static bool
+unsafe_copy_elision_p_opt (tree target, tree exp)
+{
+ tree type = TYPE_MAIN_VARIANT (TREE_TYPE (exp));
+ /* It's safe to elide the copy for a class with no tail padding. */
+ if (!is_empty_class (type)
+ && tree_int_cst_equal (TYPE_SIZE (type), CLASSTYPE_SIZE (type)))
+ return false;
+ return unsafe_copy_elision_p (target, exp);
+}
+
+/* Try to make EXP suitable to be used as the initializer for TARGET,
+ and return whether we were successful. */
+
+bool
+make_safe_copy_elision (tree target, tree exp)
+{
+ int uns = unsafe_return_slot_p (target);
+ if (!uns)
+ return true;
+ if (init_by_return_slot_p (exp))
+ return false;
+ if (uns == 1)
+ return true;
+ return make_base_init_ok (exp);
+}
+
/* True IFF the result of the conversion C is a prvalue. */
static bool
@@ -9188,7 +9223,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
/* See unsafe_copy_elision_p. */
|| unsafe_return_slot_p (fa));
- bool unsafe = unsafe_copy_elision_p (fa, arg);
+ bool unsafe = unsafe_copy_elision_p_opt (fa, arg);
bool eliding_temp = (TREE_CODE (arg) == TARGET_EXPR && !unsafe);
/* [class.copy]: the copy constructor is implicitly defined even if the
diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
index 9b7059d04c4..d9362500f06 100644
--- a/gcc/cp/typeck2.c
+++ b/gcc/cp/typeck2.c
@@ -569,17 +569,30 @@ split_nonconstant_init_1 (tree dest, tree init, bool nested)
sub = build3 (COMPONENT_REF, inner_type, dest, field_index,
NULL_TREE);
+ /* We may need to add a copy constructor call if
+ the field has [[no_unique_address]]. */
if (unsafe_return_slot_p (sub))
{
- /* We may need to add a copy constructor call if
- the field has [[no_unique_address]]. */
+ /* But not if the initializer is an implicit ctor call
+ we just built in digest_init. */
+ if (TREE_CODE (value) == TARGET_EXPR
+ && TARGET_EXPR_LIST_INIT_P (value)
+ && make_safe_copy_elision (sub, value))
+ goto build_init;
+
+ tree name = (DECL_FIELD_IS_BASE (field_index)
+ ? base_ctor_identifier
+ : complete_ctor_identifier);
releasing_vec args = make_tree_vector_single (value);
code = build_special_member_call
- (sub, complete_ctor_identifier, &args, inner_type,
+ (sub, name, &args, inner_type,
LOOKUP_NORMAL, tf_warning_or_error);
}
else
- code = build2 (INIT_EXPR, inner_type, sub, value);
+ {
+ build_init:
+ code = build2 (INIT_EXPR, inner_type, sub, value);
+ }
code = build_stmt (input_location, EXPR_STMT, code);
code = maybe_cleanup_point_expr_void (code);
add_stmt (code);
diff --git a/gcc/testsuite/g++.dg/cpp1z/elide5.C b/gcc/testsuite/g++.dg/cpp1z/elide5.C
new file mode 100644
index 00000000000..abe80ec0bc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/elide5.C
@@ -0,0 +1,15 @@
+// PR c++/98642
+// { dg-do compile { target c++11 } }
+
+struct base {
+ base(void) {}
+ base(base &&) = delete;
+};
+
+struct foo : public base { };
+
+static foo c1 { };
+
+#if __cpp_aggregate_bases
+static foo c2 { {} };
+#endif
commit 724b57377117c74d90f9fda4cce5641f63a89373
Author: Jason Merrill <ja...@redhat.com>
Date: Wed Jan 13 17:17:07 2021 -0500
c++: Elide copy from ctor call to base subobject
While looking at PR98642 it occurred to me that if we rewrite complete ctor
calls to base ctor calls, we can generally do copy elision from prvalues
that represent a constructor call. So this patch makes
unsafe_copy_elision_p layout-independent and available outside call.c; the
variant that depends on layout is renamed to unsafe_copy_elision_p_opt. And
places that decide whether to elide a copy now use unsafe_copy_elision_p
instead of unsafe_return_slot_p.
This change gets us closer to the impossible C++17 mandate to elide all
copies, but makes prvalues from a constructor more different from prvalues
from a non-constructor function.
gcc/cp/ChangeLog:
* call.c (unsafe_copy_elision_p): Add conversion overload.
(build_over_call): Use it.
(build_special_member_call): Use make_safe_copy_elision.
* typeck2.c (split_nonconstant_init_1): Likewise.
* init.c (expand_default_init): Move unsafe_return_slot_p check to
assert.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1z/elide6.C: New test.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index b6e9f125aeb..430e2468839 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8562,6 +8562,31 @@ unsafe_copy_elision_p_opt (tree target, tree exp)
return unsafe_copy_elision_p (target, exp);
}
+/* As above, but the second parameter is a conversion for the first argument of
+ a copy/move constructor. This assumes that conv_binds_ref_to_prvalue has
+ already returned true. */
+
+static bool
+unsafe_copy_elision_p (tree target, conversion *conv)
+{
+ if (!unsafe_return_slot_p (target))
+ return false;
+ gcc_checking_assert (conv->kind == ck_ref_bind);
+ conv = next_conversion (conv);
+ if (conv->kind == ck_identity && conv->u.expr)
+ /* The argument was already a prvalue, pass to the fn above. */
+ return init_by_return_slot_p (conv->u.expr);
+ else if (conv->kind == ck_user && DECL_CONSTRUCTOR_P (conv->cand->fn))
+ /* The argument is a constructor call, safe to elide. */
+ return false;
+ else if (conv->kind == ck_base || conv->kind == ck_rvalue)
+ /* These will also be a constructor call. */
+ return false;
+ else
+ /* Anything else is unsafe if the target is unsafe. */
+ return true;
+}
+
/* Try to make EXP suitable to be used as the initializer for TARGET,
and return whether we were successful. */
@@ -8794,11 +8819,10 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
bool force_elide = false;
if (cxx_dialect >= cxx17
&& cand->num_convs == 1
- && DECL_COMPLETE_CONSTRUCTOR_P (fn)
&& (DECL_COPY_CONSTRUCTOR_P (fn)
|| DECL_MOVE_CONSTRUCTOR_P (fn))
- && !unsafe_return_slot_p (first_arg)
- && conv_binds_ref_to_prvalue (convs[0]))
+ && conv_binds_ref_to_prvalue (convs[0])
+ && !unsafe_copy_elision_p (first_arg, convs[0]))
{
force_elide = true;
goto not_really_used;
@@ -9220,8 +9244,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
/* It's from binding the ref parm to a packed field. */
|| convs[0]->need_temporary_p
|| seen_error ()
- /* See unsafe_copy_elision_p. */
- || unsafe_return_slot_p (fa));
+ || unsafe_copy_elision_p (fa, arg));
bool unsafe = unsafe_copy_elision_p_opt (fa, arg);
bool eliding_temp = (TREE_CODE (arg) == TARGET_EXPR && !unsafe);
@@ -10033,8 +10056,7 @@ build_special_member_call (tree instance, tree name, vec<tree, va_gc> **args,
destination object." Handle that here to avoid doing overload
resolution. */
if (cxx_dialect >= cxx17
- && args && vec_safe_length (*args) == 1
- && !unsafe_return_slot_p (instance))
+ && args && vec_safe_length (*args) == 1)
{
tree arg = (**args)[0];
@@ -10046,7 +10068,8 @@ build_special_member_call (tree instance, tree name, vec<tree, va_gc> **args,
if ((TREE_CODE (arg) == TARGET_EXPR
|| TREE_CODE (arg) == CONSTRUCTOR)
&& (same_type_ignoring_top_level_qualifiers_p
- (class_type, TREE_TYPE (arg))))
+ (class_type, TREE_TYPE (arg)))
+ && make_safe_copy_elision (instance, arg))
{
if (is_dummy_object (instance))
return arg;
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index e6de2e10dc3..b63f721e383 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -1908,11 +1908,11 @@ expand_default_init (tree binfo, tree true_exp, tree exp, tree init, int flags,
}
if (init && TREE_CODE (init) != TREE_LIST
- && (flags & LOOKUP_ONLYCONVERTING)
- && !unsafe_return_slot_p (exp))
+ && (flags & LOOKUP_ONLYCONVERTING))
{
/* Base subobjects should only get direct-initialization. */
gcc_assert (true_exp == exp);
+ gcc_assert (!unsafe_return_slot_p (exp));
if (flags & DIRECT_BIND)
/* Do nothing. We hit this in two cases: Reference initialization,
diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
index d9362500f06..c2bf7b1ac4a 100644
--- a/gcc/cp/typeck2.c
+++ b/gcc/cp/typeck2.c
@@ -571,15 +571,8 @@ split_nonconstant_init_1 (tree dest, tree init, bool nested)
/* We may need to add a copy constructor call if
the field has [[no_unique_address]]. */
- if (unsafe_return_slot_p (sub))
+ if (!make_safe_copy_elision (sub, value))
{
- /* But not if the initializer is an implicit ctor call
- we just built in digest_init. */
- if (TREE_CODE (value) == TARGET_EXPR
- && TARGET_EXPR_LIST_INIT_P (value)
- && make_safe_copy_elision (sub, value))
- goto build_init;
-
tree name = (DECL_FIELD_IS_BASE (field_index)
? base_ctor_identifier
: complete_ctor_identifier);
@@ -589,10 +582,7 @@ split_nonconstant_init_1 (tree dest, tree init, bool nested)
LOOKUP_NORMAL, tf_warning_or_error);
}
else
- {
- build_init:
- code = build2 (INIT_EXPR, inner_type, sub, value);
- }
+ code = build2 (INIT_EXPR, inner_type, sub, value);
code = build_stmt (input_location, EXPR_STMT, code);
code = maybe_cleanup_point_expr_void (code);
add_stmt (code);
diff --git a/gcc/testsuite/g++.dg/cpp1z/elide6.C b/gcc/testsuite/g++.dg/cpp1z/elide6.C
new file mode 100644
index 00000000000..a4d5d1f6f87
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/elide6.C
@@ -0,0 +1,14 @@
+// Test that we do elide copies from ctor calls to bases.
+// { dg-do compile { target c++17 } }
+
+struct base {
+ base(void) {}
+ base(base &&) = delete;
+};
+
+struct foo : public base { };
+static foo c3 { base{} };
+
+struct bar : public base {
+ bar(): base(base()) { }
+};
