diff mbox series

[pushed] c++: Copy elision and [[no_unique_address]]. [PR93711]

Message ID 20200814165825.20686-1-jason@redhat.com
State New
Headers show
Series [pushed] c++: Copy elision and [[no_unique_address]]. [PR93711] | expand

Commit Message

Jason Merrill Aug. 14, 2020, 4:58 p.m. UTC 
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.

Tested x86_64-pc-linux-gnu, applying to trunk.

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/cp-tree.h                              |  1 +
 gcc/cp/call.c                                 | 45 ++++++++++++-------
 gcc/cp/init.c                                 |  3 +-
 gcc/cp/typeck2.c                              | 12 ++++-
 gcc/testsuite/g++.dg/cpp1z/aggr-base10.C      | 16 +++++++
 .../g++.dg/cpp2a/no_unique_address7.C         | 13 ++++++
 .../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


base-commit: 22dc89f8073cd0126efa72aa3ab88e80c78d45e3
diff mbox series

Patch

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index fc54e6bb9bd..921bf954647 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6372,6 +6372,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/call.c b/gcc/cp/call.c
index 47a368d069d..e6c14084127 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<tree, va_gc> **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/init.c b/gcc/cp/init.c
index 3f089404cf2..872c23453fd 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 dac135a2e11..b95f112a744 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 00000000000..10370daa6e6
--- /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 00000000000..edd82d1eccc
--- /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 00000000000..453baceb905
--- /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" }