Re: C++ PATCH for c++/71913 (copy elision choices)

2016-07-25 Thread Jason Merrill
On Mon, Jul 25, 2016 at 7:15 AM, Renlin Li  wrote:
> Hi Jason,
>
> On 22/07/16 04:01, Jason Merrill wrote:
>>
>> 71913 is a case where unsafe_copy_elision_p was being too
>> conservative. We can allow copy elision in a new expression; the only
>> way we could end up initializing a base subobject without knowing it
>> would be through a placement new, in which case we would already be
>> using the wrong (complete object) constructor, so copy elision doesn't
>> make it any worse.
>>
>
>> diff --git a/gcc/testsuite/g++.dg/init/elide5.C
>> b/gcc/testsuite/g++.dg/init/elide5.C
>> new file mode 100644
>> index 000..0a9978c
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/init/elide5.C
>> @@ -0,0 +1,27 @@
>> +// PR c++/71913
>> +// { dg-do link { target c++11 } }
>> +
>> +void* operator new(unsigned long, void* p) { return p; }
>
>
> g++.dg/init/elide5.C fails on target whose SIZE_TYPE is not "long unsigned
> int".
>
> testsuite/g++.dg/init/elide5.C:4:42: error: 'operator new' takes type
> 'size_t' ('unsigned int') as first parameter [-fpermissive]
>
> I have checked, for most 32 bit architectures or ABI, the SIZE_TYPE is
> "unsigned int". arm is one of them.
>
> To make this test case portable, will __SIZE_TYPE__ be better in this case,
> instead of "unsigned long" as first argument of new operator?

Thanks, I'll fix that shortly.

Jason


Re: C++ PATCH for c++/71913 (copy elision choices)

2016-07-25 Thread Renlin Li

Hi Jason,

On 22/07/16 04:01, Jason Merrill wrote:

71913 is a case where unsafe_copy_elision_p was being too
conservative. We can allow copy elision in a new expression; the only
way we could end up initializing a base subobject without knowing it
would be through a placement new, in which case we would already be
using the wrong (complete object) constructor, so copy elision doesn't
make it any worse.




diff --git a/gcc/testsuite/g++.dg/init/elide5.C 
b/gcc/testsuite/g++.dg/init/elide5.C
new file mode 100644
index 000..0a9978c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/elide5.C
@@ -0,0 +1,27 @@
+// PR c++/71913
+// { dg-do link { target c++11 } }
+
+void* operator new(unsigned long, void* p) { return p; }


g++.dg/init/elide5.C fails on target whose SIZE_TYPE is not "long 
unsigned int".


testsuite/g++.dg/init/elide5.C:4:42: error: 'operator new' takes type 
'size_t' ('unsigned int') as first parameter [-fpermissive]


I have checked, for most 32 bit architectures or ABI, the SIZE_TYPE is 
"unsigned int". arm is one of them.


To make this test case portable, will __SIZE_TYPE__ be better in this 
case, instead of "unsigned long" as first argument of new operator?


(sorry for the duplicate reply in the bugzilla, I just found the email here)

Regards,
Renlin


C++ PATCH for c++/71913 (copy elision choices)

2016-07-21 Thread Jason Merrill
71913 is a case where unsafe_copy_elision_p was being too
conservative. We can allow copy elision in a new expression; the only
way we could end up initializing a base subobject without knowing it
would be through a placement new, in which case we would already be
using the wrong (complete object) constructor, so copy elision doesn't
make it any worse.

The second patch is for a case where we weren't being conservative
enough; we need to beware of tail padding even when the copy
constructor is trivial.

Tested x86_64-pc-linux-gnu, applying to trunk.  71913 patch also
applied to 6 branch.
commit 32e2ba606cd8993bd3de82708711ae26fc251d0a
Author: Jason Merrill 
Date:   Thu Jul 21 16:55:56 2016 -0400

PR c++/71913 - missing copy elision with new.

* call.c (unsafe_copy_elision_p): It's OK to elide when
initializing an unknown object.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index d917d9a..061e708 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7275,10 +7275,11 @@ unsafe_copy_elision_p (tree target, tree exp)
   if (TREE_CODE (exp) != TARGET_EXPR)
 return false;
   tree type = TYPE_MAIN_VARIANT (TREE_TYPE (exp));
-  if (type == CLASSTYPE_AS_BASE (type))
+  /* 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)))
 return false;
-  if (!is_base_field_ref (target)
-  && resolves_to_fixed_type_p (target, NULL))
+  /* It's safe to elide the copy if we aren't initializing a base object.  */
+  if (!is_base_field_ref (target))
 return false;
   tree init = TARGET_EXPR_INITIAL (exp);
   /* build_compound_expr pushes COMPOUND_EXPR inside TARGET_EXPR.  */
diff --git a/gcc/testsuite/g++.dg/init/elide5.C 
b/gcc/testsuite/g++.dg/init/elide5.C
new file mode 100644
index 000..0a9978c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/elide5.C
@@ -0,0 +1,27 @@
+// PR c++/71913
+// { dg-do link { target c++11 } }
+
+void* operator new(unsigned long, void* p) { return p; }
+
+struct IndirectReturn {
+  IndirectReturn() {}
+  // Undefined so we get a link error if the indirect return value is copied
+  IndirectReturn(const IndirectReturn&);
+  IndirectReturn& operator=(const IndirectReturn&) = delete;
+  ~IndirectReturn() {}
+};
+
+IndirectReturn foo() { return IndirectReturn(); }
+
+void bar(void* ptr) {
+  new (ptr) IndirectReturn(foo());
+}
+
+alignas (alignof (IndirectReturn))
+unsigned char c[sizeof(IndirectReturn)];
+
+int main()
+{
+  bar(c);
+}
+
commit 5d549fb4e1ee815daa678bd369a2650055d61311
Author: Jason Merrill 
Date:   Thu Jul 21 15:57:35 2016 -0400

* call.c (build_over_call): Check unsafe_copy_elision_p even for
trivial constructors.
* method.c (do_build_copy_constructor): Don't copy tail padding
even in a trivial constructor.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f929fb2..d917d9a 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7271,6 +7271,9 @@ is_base_field_ref (tree t)
 static bool
 unsafe_copy_elision_p (tree target, 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));
   if (type == CLASSTYPE_AS_BASE (type))
 return false;
@@ -7726,9 +7729,8 @@ build_over_call (struct z_candidate *cand, int flags, 
tsubst_flags_t complain)
  else if (trivial)
return force_target_expr (DECL_CONTEXT (fn), arg, complain);
}
-  else if (trivial
-  || (TREE_CODE (arg) == TARGET_EXPR
-  && !unsafe_copy_elision_p (fa, arg)))
+  else if ((trivial || TREE_CODE (arg) == TARGET_EXPR)
+  && !unsafe_copy_elision_p (fa, arg))
{
  tree to = cp_stabilize_reference (cp_build_indirect_ref (fa,
   RO_NULL,
diff --git a/gcc/cp/method.c b/gcc/cp/method.c
index cd8faaf..63aa53e 100644
--- a/gcc/cp/method.c
+++ b/gcc/cp/method.c
@@ -542,14 +542,32 @@ do_build_copy_constructor (tree fndecl)
   if (!inh)
 parm = convert_from_reference (parm);
 
-  if (trivial
-  && is_empty_class (current_class_type))
-/* Don't copy the padding byte; it might not have been allocated
-   if *this is a base subobject.  */;
-  else if (trivial)
+  if (trivial)
 {
-  tree t = build2 (INIT_EXPR, void_type_node, current_class_ref, parm);
-  finish_expr_stmt (t);
+  if (is_empty_class (current_class_type))
+   /* Don't copy the padding byte; it might not have been allocated
+  if *this is a base subobject.  */;
+  else if (tree_int_cst_equal (TYPE_SIZE (current_class_type),
+  CLASSTYPE_SIZE (current_class_type)))
+   {
+ tree t = build2 (INIT_EXPR, void_type_node, current_class_ref, parm);
+ finish_expr_stmt (t);
+   }
+  else
+   {
+ /* We must only copy the non-tail padding parts.  */
+ tree b