From 320054784250e572cb75d6f69ab44b2330d61d8b Mon Sep 17 00:00:00 2001 From: Jason Merrill Date: Wed, 12 Aug 2020 05:45:02 -0400 Subject: [PATCH] c++: Copy elision and [[no_unique_address]]. [PR93711] We don't elide a copy from a function returning a class by value into a base because that can overwrite data laid out in the tail padding of the base class; we need to handle [[no_unique_address]] fields the same way, or we ICE when the middle-end wants to create a temporary object of a TYPE_NEEDS_CONSTRUCTING type. This means that we can't always express initialization of a field with INIT_EXPR from a TARGET_EXPR the way we usually do, so I needed to change several places that were assuming that was sufficient. This also fixes 90254, the same problem with C++17 aggregate initialization of a base. gcc/cp/ChangeLog: PR c++/90254 PR c++/93711 * cp-tree.h (unsafe_return_slot_p): Declare. * call.c (is_base_field_ref): Rename to unsafe_return_slot_p. (build_over_call): Check unsafe_return_slot_p. (build_special_member_call): Likewise. * init.c (expand_default_init): Likewise. * typeck2.c (split_nonconstant_init_1): Likewise. gcc/testsuite/ChangeLog: PR c++/90254 PR c++/93711 * g++.dg/cpp1z/aggr-base10.C: New test. * g++.dg/cpp2a/no_unique_address7.C: New test. * g++.dg/cpp2a/no_unique_address7a.C: New test. --- gcc/cp/call.c | 45 +++++++++++++++--------- gcc/cp/cp-tree.h | 1 + gcc/cp/init.c | 3 +- gcc/cp/typeck2.c | 12 ++++++- gcc/testsuite/g++.dg/cpp1z/aggr-base10.C | 16 +++++++++ gcc/testsuite/g++.dg/cpp2a/no_unique_address7.C | 13 +++++++ gcc/testsuite/g++.dg/cpp2a/no_unique_address7a.C | 14 ++++++++ 7 files changed, 85 insertions(+), 19 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp1z/aggr-base10.C create mode 100644 gcc/testsuite/g++.dg/cpp2a/no_unique_address7.C create mode 100644 gcc/testsuite/g++.dg/cpp2a/no_unique_address7a.C diff --git a/gcc/cp/call.c b/gcc/cp/call.c index d4935bd..94aaf65 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -8347,24 +8347,34 @@ call_copy_ctor (tree a, tsubst_flags_t complain) return r; } -/* Return true iff T refers to a base field. */ +/* 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. -static bool -is_base_field_ref (tree t) + 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. */ + +bool +unsafe_return_slot_p (tree t) { STRIP_NOPS (t); if (TREE_CODE (t) == ADDR_EXPR) t = TREE_OPERAND (t, 0); if (TREE_CODE (t) == COMPONENT_REF) t = TREE_OPERAND (t, 1); - if (TREE_CODE (t) == FIELD_DECL) - return DECL_FIELD_IS_BASE (t); - return false; + if (TREE_CODE (t) != FIELD_DECL) + return false; + 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))); } -/* We can't elide a copy from a function returning by value to a base - subobject, as the callee might clobber tail padding. Return true iff this - could be that case. */ +/* 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. */ static bool unsafe_copy_elision_p (tree target, tree exp) @@ -8374,10 +8384,11 @@ unsafe_copy_elision_p (tree target, tree exp) 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 (tree_int_cst_equal (TYPE_SIZE (type), CLASSTYPE_SIZE (type))) + 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 (!is_base_field_ref (target)) + if (!unsafe_return_slot_p (target)) return false; tree init = TARGET_EXPR_INITIAL (exp); /* build_compound_expr pushes COMPOUND_EXPR inside TARGET_EXPR. */ @@ -8569,6 +8580,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) && 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])) { force_elide = true; @@ -8953,7 +8965,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) { tree targ; tree arg = argarray[num_artificial_parms_for (fn)]; - tree fa; + tree fa = argarray[0]; bool trivial = trivial_fn_p (fn); /* Pull out the real argument, disregarding const-correctness. */ @@ -8983,8 +8995,8 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) else arg = cp_build_fold_indirect_ref (arg); - /* In C++17 we shouldn't be copying a TARGET_EXPR except into a base - subobject. */ + /* In C++17 we shouldn't be copying a TARGET_EXPR except into a + potentially-overlapping subobject. */ if (CHECKING_P && cxx_dialect >= cxx17) gcc_assert (TREE_CODE (arg) != TARGET_EXPR || force_elide @@ -8992,9 +9004,8 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) || convs[0]->need_temporary_p || seen_error () /* See unsafe_copy_elision_p. */ - || DECL_BASE_CONSTRUCTOR_P (fn)); + || unsafe_return_slot_p (fa)); - fa = argarray[0]; bool unsafe = unsafe_copy_elision_p (fa, arg); bool eliding_temp = (TREE_CODE (arg) == TARGET_EXPR && !unsafe); @@ -9806,7 +9817,7 @@ build_special_member_call (tree instance, tree name, vec **args, resolution. */ if (cxx_dialect >= cxx17 && args && vec_safe_length (*args) == 1 - && name == complete_ctor_identifier) + && !unsafe_return_slot_p (instance)) { tree arg = (**args)[0]; diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 82834e6..59ab230 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -6359,6 +6359,7 @@ 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 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/init.c b/gcc/cp/init.c index 3f08940..872c234 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -1895,7 +1895,8 @@ expand_default_init (tree binfo, tree true_exp, tree exp, tree init, int flags, } if (init && TREE_CODE (init) != TREE_LIST - && (flags & LOOKUP_ONLYCONVERTING)) + && (flags & LOOKUP_ONLYCONVERTING) + && !unsafe_return_slot_p (exp)) { /* Base subobjects should only get direct-initialization. */ gcc_assert (true_exp == exp); diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c index dac135a..b95f112 100644 --- a/gcc/cp/typeck2.c +++ b/gcc/cp/typeck2.c @@ -711,7 +711,17 @@ split_nonconstant_init_1 (tree dest, tree init, bool nested) sub = build3 (COMPONENT_REF, inner_type, dest, field_index, NULL_TREE); - code = build2 (INIT_EXPR, inner_type, sub, value); + if (unsafe_return_slot_p (sub)) + { + /* We may need to add a copy constructor call if + the field has [[no_unique_address]]. */ + releasing_vec args = make_tree_vector_single (value); + code = build_special_member_call + (sub, complete_ctor_identifier, &args, inner_type, + LOOKUP_NORMAL, tf_warning_or_error); + } + else + 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/aggr-base10.C b/gcc/testsuite/g++.dg/cpp1z/aggr-base10.C new file mode 100644 index 0000000..10370da --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1z/aggr-base10.C @@ -0,0 +1,16 @@ +// PR c++/90254 +// { dg-do compile { target c++17 } } + +struct A { + A(); + A(const A &); +}; +struct B : A { }; + +A foo (); + +int +main () +{ + B b{foo()}; +} diff --git a/gcc/testsuite/g++.dg/cpp2a/no_unique_address7.C b/gcc/testsuite/g++.dg/cpp2a/no_unique_address7.C new file mode 100644 index 0000000..edd82d1 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/no_unique_address7.C @@ -0,0 +1,13 @@ +// PR c++/93711 +// { dg-do compile { target c++11 } } + +struct A { A(const A&) = delete; }; + +A f(); + +struct C +{ + [[no_unique_address]] A a; +}; + +C c{f()}; // { dg-error "deleted" } diff --git a/gcc/testsuite/g++.dg/cpp2a/no_unique_address7a.C b/gcc/testsuite/g++.dg/cpp2a/no_unique_address7a.C new file mode 100644 index 0000000..453bace --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/no_unique_address7a.C @@ -0,0 +1,14 @@ +// PR c++/93711 +// { dg-do compile { target c++11 } } + +struct B { }; +struct A: B { A(const A&) = delete; }; + +A f(); + +struct C +{ + [[no_unique_address]] A a; +}; + +C c{f()}; // { dg-error "deleted" } -- 2.7.4