diff mbox

RFC: PATCH to allow passing non-trivial types through ...

Message ID 53E92F6C.5050808@redhat.com
State New
Headers show

Commit Message

Jason Merrill Aug. 11, 2014, 9:02 p.m. UTC 
A customer was recently complaining about G++ rejecting code that tries 
to pass a type with a non-trivial copy constructor through ..., which is 
undefined in C++98 and conditionally-supported in C++11.  In GCC 3.1 and 
below we gave a warning and then did a bitwise copy.  From GCC 3.2 to 
4.4 we gave a warning and then aborted at runtime.  From 4.5 on we have 
just given an error.

On considering the request, it occurred to me that we could handle 
variadic arguments of non-trivial types the same way we handle normal 
value arguments of such types: pass by invisible reference.  So this 
patch implements that.  Since it's been so long since this was allowed 
at all, I don't think we need to worry about ABI incompatibility with 
the 3.1 behavior.

Thoughts?

Comments

Mike Stump Aug. 11, 2014, 11:31 p.m. UTC | #1 
On Aug 11, 2014, at 2:02 PM, Jason Merrill <jason@redhat.com> wrote:
> 
> On considering the request, it occurred to me that we could handle variadic arguments of non-trivial types the same way we handle normal value arguments of such types: pass by invisible reference.  So this patch implements that.  Since it's been so long since this was allowed at all, I don't think we need to worry about ABI incompatibility with the 3.1 behavior.

So, it would be nice if the C++ abi document defined how to pass them.  Once it says do it this way, then all people have to do it that way if they want to claim conformance to that doc, and when all do, we then get C++ abi compatibility across compilers (which is nice).  If they want to dictate some other convention, then it sure would be nice to know _before_ we ship an abi incompatible version.  I’m thinking compatibility with clang for example would be nice.  Also, would be nice if the standard mandated it just work, though, that might take another standard to get it in there.
Nathan Sidwell Aug. 12, 2014, 5:23 a.m. UTC | #2 
On 08/11/14 14:02, Jason Merrill wrote:
> A customer was recently complaining about G++ rejecting code that tries to pass
> a type with a non-trivial copy constructor through ..., which is undefined in
> C++98 and conditionally-supported in C++11.  In GCC 3.1 and below we gave a
> warning and then did a bitwise copy.  From GCC 3.2 to 4.4 we gave a warning and
> then aborted at runtime.  From 4.5 on we have just given an error.
>
> On considering the request, it occurred to me that we could handle variadic
> arguments of non-trivial types the same way we handle normal value arguments of
> such types: pass by invisible reference.  So this patch implements that.  Since
> it's been so long since this was allowed at all, I don't think we need to worry
> about ABI incompatibility with the 3.1 behavior.
>
> Thoughts?

Sounds sensible to me.

nathan
diff mbox

Patch

commit 68a0f48b2c82f157a04842e996fe340428fcfcc3
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Aug 8 17:23:14 2014 -0400

    	* call.c (build_x_va_arg): Support passing non-POD through ....
    	(convert_arg_to_ellipsis): Likewise.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 56b3c5a..fbe92a9 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6570,8 +6570,8 @@  convert_arg_to_ellipsis (tree arg, tsubst_flags_t complain)
 	 with no corresponding parameter is conditionally-supported, with
 	 implementation-defined semantics.
 
-	 We used to just warn here and do a bitwise copy, but now
-	 cp_expr_size will abort if we try to do that.
+	 We support it as pass-by-invisible-reference, just like a normal
+	 value parameter.
 
 	 If the call appears in the context of a sizeof expression,
 	 it is not potentially-evaluated.  */
@@ -6579,10 +6579,12 @@  convert_arg_to_ellipsis (tree arg, tsubst_flags_t complain)
 	  && (type_has_nontrivial_copy_init (arg_type)
 	      || TYPE_HAS_NONTRIVIAL_DESTRUCTOR (arg_type)))
 	{
-	  if (complain & tf_error)
-	    error_at (loc, "cannot pass objects of non-trivially-copyable "
-		      "type %q#T through %<...%>", arg_type);
-	  return error_mark_node;
+	  if (complain & tf_warning)
+	    warning (OPT_Wconditionally_supported,
+		     "passing objects of non-trivially-copyable "
+		     "type %q#T through %<...%> is conditionally supported",
+		     arg_type);
+	  return cp_build_addr_expr (arg, complain);
 	}
     }
 
@@ -6595,7 +6597,11 @@  tree
 build_x_va_arg (source_location loc, tree expr, tree type)
 {
   if (processing_template_decl)
-    return build_min (VA_ARG_EXPR, type, expr);
+    {
+      tree r = build_min (VA_ARG_EXPR, type, expr);
+      SET_EXPR_LOCATION (r, loc);
+      return r;
+    }
 
   type = complete_type_or_else (type, NULL_TREE);
 
@@ -6604,18 +6610,24 @@  build_x_va_arg (source_location loc, tree expr, tree type)
 
   expr = mark_lvalue_use (expr);
 
+  if (TREE_CODE (type) == REFERENCE_TYPE)
+    {
+      error ("cannot receive reference type %qT through %<...%>", type);
+      return error_mark_node;
+    }
+
   if (type_has_nontrivial_copy_init (type)
-      || TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type)
-      || TREE_CODE (type) == REFERENCE_TYPE)
+      || TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type))
     {
-      /* Remove reference types so we don't ICE later on.  */
-      tree type1 = non_reference (type);
-      /* conditionally-supported behavior [expr.call] 5.2.2/7.  */
-      error ("cannot receive objects of non-trivially-copyable type %q#T "
-	     "through %<...%>; ", type);
-      expr = convert (build_pointer_type (type1), null_node);
-      expr = cp_build_indirect_ref (expr, RO_NULL, tf_warning_or_error);
-      return expr;
+      /* conditionally-supported behavior [expr.call] 5.2.2/7.  Let's treat
+	 it as pass by invisible reference.  */
+      warning_at (loc, OPT_Wconditionally_supported,
+		 "receiving objects of non-trivially-copyable type %q#T "
+		 "through %<...%> is conditionally-supported", type);
+
+      tree ptr = build_pointer_type (type);
+      ptr = build_va_arg (loc, expr, ptr);
+      return cp_build_indirect_ref (ptr, RO_NULL, tf_warning_or_error);
     }
 
   return build_va_arg (loc, expr, type);
diff --git a/gcc/doc/implement-cxx.texi b/gcc/doc/implement-cxx.texi
index 50efcc3..5802311 100644
--- a/gcc/doc/implement-cxx.texi
+++ b/gcc/doc/implement-cxx.texi
@@ -42,7 +42,9 @@  all conditionally-supported constructs that it does not support (C++0x
 @cite{Whether an argument of class type with a non-trivial copy
 constructor or destructor can be passed to ... (C++0x 5.2.2).}
 
-Such argument passing is not supported.
+Such argument passing is supported, using the same
+pass-by-invisible-reference approach used for normal function
+arguments of such types.
 
 @end itemize
 
diff --git a/gcc/testsuite/g++.dg/ext/varargs1.C b/gcc/testsuite/g++.dg/ext/varargs1.C
new file mode 100644
index 0000000..b67d788
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/varargs1.C
@@ -0,0 +1,34 @@ 
+// Test that passing an object with non-trivial copy constructor and
+// destructor is (conditionally) supported and has sensible semantics.
+
+#include <stdarg.h>
+extern "C" void abort();
+
+void *as[5];
+int i;
+
+struct A {
+  A() { as[i++] = this; }
+  A(const A& a) {
+    if (&a != as[i-1])
+      abort();
+    as[i++] = this;
+  }
+  ~A() {
+    if (this != as[--i])
+      abort();
+  }
+};
+
+void f(int i, ...) {
+  va_list ap;
+  va_start (ap, i);
+  A ar = va_arg (ap, A);
+}
+
+int main()
+{
+  f(42,A());
+  if (i != 0)
+    abort();
+}
diff --git a/gcc/testsuite/g++.dg/overload/ellipsis1.C b/gcc/testsuite/g++.dg/overload/ellipsis1.C
index 3dedaa6..1dde2bc 100644
--- a/gcc/testsuite/g++.dg/overload/ellipsis1.C
+++ b/gcc/testsuite/g++.dg/overload/ellipsis1.C
@@ -1,5 +1,6 @@ 
 // PR c++/15142
 // Bug: We were aborting after giving a warning about passing a non-POD.
+// { dg-options "-Wconditionally-supported" }
 
 struct B { 
     B() throw() { } 
@@ -14,5 +15,5 @@  struct X {
 struct S { S(...); }; 
  
 void SillyFunc() { 
-  throw S(X()); 		// { dg-error "copy" }
+  throw S(X()); 		// { dg-message "copy" }
 } 
diff --git a/gcc/testsuite/g++.dg/overload/ellipsis2.C b/gcc/testsuite/g++.dg/overload/ellipsis2.C
index d9118ba..c226e1c 100644
--- a/gcc/testsuite/g++.dg/overload/ellipsis2.C
+++ b/gcc/testsuite/g++.dg/overload/ellipsis2.C
@@ -1,4 +1,5 @@ 
 // PR c++/60253
+// { dg-options "-Wconditionally-supported" }
 
 struct A
 {
@@ -10,4 +11,4 @@  struct B
   B(...);
 };
 
-B b(0, A());  // { dg-error "cannot pass" }
+B b(0, A());  // { dg-message "pass" }
diff --git a/gcc/testsuite/g++.dg/warn/var-args1.C b/gcc/testsuite/g++.dg/warn/var-args1.C
index 9bd84a7..35deb09 100644
--- a/gcc/testsuite/g++.dg/warn/var-args1.C
+++ b/gcc/testsuite/g++.dg/warn/var-args1.C
@@ -6,6 +6,6 @@  void foo(int, ...)
 {
     va_list va;
     int i;
-    i = va_arg(va, int&); /* { dg-error "cannot receive objects" } */
+    i = va_arg(va, int&); /* { dg-error "cannot receive" } */
 }
 
diff --git a/gcc/testsuite/g++.old-deja/g++.brendan/crash63.C b/gcc/testsuite/g++.old-deja/g++.brendan/crash63.C
index 89685fc..badd926 100644
--- a/gcc/testsuite/g++.old-deja/g++.brendan/crash63.C
+++ b/gcc/testsuite/g++.old-deja/g++.brendan/crash63.C
@@ -13,4 +13,4 @@  class UnitList
    UnitList (...);
    };
 
-UnitList unit_list (String("keV")); // { dg-error "" } cannot pass non-pod
+UnitList unit_list (String("keV"));
diff --git a/gcc/testsuite/g++.old-deja/g++.other/vaarg3.C b/gcc/testsuite/g++.old-deja/g++.other/vaarg3.C
index 134a89c..98f7877 100644
--- a/gcc/testsuite/g++.old-deja/g++.other/vaarg3.C
+++ b/gcc/testsuite/g++.old-deja/g++.other/vaarg3.C
@@ -1,5 +1,6 @@ 
 // { dg-do assemble  }
-// { dg-options "-Wno-abi" { target arm_eabi } }
+// { dg-options "-Wconditionally-supported" }
+// { dg-options "-Wno-abi -Wconditionally-supported" { target arm_eabi } }
 
 // Copyright (C) 1999 Free Software Foundation, Inc.
 // Contributed by Nathan Sidwell 4 Oct 1999 <nathan@acm.org>
@@ -14,19 +15,19 @@  struct Z;   // { dg-message "forward decl" }
 void fn1(va_list args)
 {
   int i = va_arg (args, int);
-  Y x = va_arg (args, Y);         // { dg-error "cannot receive" }
-  Y y = va_arg (args, struct Y);  // { dg-error "cannot receive" }
+  Y x = va_arg (args, Y);         // { dg-message "receiv" }
+  Y y = va_arg (args, struct Y);  // { dg-message "receiv" }
   int &r = va_arg (args, int &);  // { dg-error "cannot receive" }
   
   Z z1 = va_arg (args, Z);        // { dg-error "incomplete" } 
   const Z &z2 = va_arg (args, Z);       // { dg-error "incomplete" } 
 
   va_arg (args, char);    // { dg-warning "promote" } 
-  // { dg-message "should pass" "pass" { target *-*-* } 24 }
-  // { dg-message "abort" "abort" { target *-*-* } 24 }
+  // { dg-message "should pass" "pass" { target *-*-* } 25 }
+  // { dg-message "abort" "abort" { target *-*-* } 25 }
   va_arg (args, int []);  // { dg-error "array with unspecified bounds" } promote
   va_arg (args, int ());  // { dg-warning "promoted" } promote
-  // { dg-message "abort" "abort" { target *-*-* } 28 }
+  // { dg-message "abort" "abort" { target *-*-* } 29 }
   va_arg (args, bool);    // { dg-warning "promote" "promote" } 
-  // { dg-message "abort" "abort" { target *-*-* } 30 }
+  // { dg-message "abort" "abort" { target *-*-* } 31 }
 }
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/vaarg3.C b/gcc/testsuite/g++.old-deja/g++.pt/vaarg3.C
index e880119..ee84db9 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/vaarg3.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/vaarg3.C
@@ -1,4 +1,5 @@ 
 // { dg-do assemble  }
+// { dg-options "-Wconditionally-supported" }
 // Copyright (C) 2000 Free Software Foundation
 // Contributed by Nathan Sidwell 22 June 2000 <nathan@codesourcery.com>
 
@@ -14,14 +15,14 @@  void PrintArgs (Type somearg, ...)
 va_list argp;
 va_start (argp, somearg);
 Type value;
-value = va_arg (argp, Type); // { dg-error "cannot receive" } cannot pass non-POD
+value = va_arg (argp, Type); // { dg-message "receiv" } cannot pass non-POD
 va_end (argp);
 }
 
 int main (void)
 {
 A dummy;
-PrintArgs (dummy, dummy); // { dg-error "cannot pass" } cannot pass non-POD
-// { dg-message "required" "inst" { target *-*-* } 24 }
+PrintArgs (dummy, dummy); // { dg-message "pass" } cannot pass non-POD
+// { dg-message "required" "inst" { target *-*-* } 25 }
 return 0;
 }