From d75199f782348bfc401f925b60f33ffc9822b7cc Mon Sep 17 00:00:00 2001 From: Jason Merrill Date: Wed, 13 Jan 2021 13:27:06 -0500 Subject: [PATCH] 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. --- gcc/cp/call.c | 91 ++++++++++++++++++++--------- gcc/cp/cp-tree.h | 3 +- gcc/cp/typeck2.c | 21 +++++-- gcc/testsuite/g++.dg/cpp1z/elide5.C | 15 +++++ 4 files changed, 97 insertions(+), 33 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp1z/elide5.C 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/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/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 -- 2.30.2