diff mbox

[C++] Don't promote bitfields in last arg of __builtin_*_overflow_p

Message ID 20160615195122.GP7387@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek June 15, 2016, 7:51 p.m. UTC 
On Wed, Jun 15, 2016 at 08:08:22AM -0600, Martin Sebor wrote:
> I like the idea of being able to use the built-ins for this, but
> I think it would be confusing for them to follow subtly different
> rules for C than for C++.  Since the value of the last argument

Here is incremental patch to the patch I've posted earlier today,
which doesn't promote the last argument of __builtin_*_overflow_p
and thus for bitfields it behaves pretty much like the C FE.

Bootstrapped/regtested (together with the other patch) on x86_64-linux and
i686-linux, ok for trunk?

2016-06-15  Jakub Jelinek  <jakub@redhat.com>

	* call.c (magic_varargs_p): Return 3 for __builtin_*_overflow_p.
	(build_over_call): For magic == 3, do no conversion only on 3rd
	argument.

	* c-c++-common/torture/builtin-arith-overflow-p-19.c: Run for C++ too.
	* g++.dg/ext/builtin-arith-overflow-2.C: New test.



	Jakub

Comments

Martin Sebor June 16, 2016, 2:24 a.m. UTC | #1 
On 06/15/2016 01:51 PM, Jakub Jelinek wrote:
> On Wed, Jun 15, 2016 at 08:08:22AM -0600, Martin Sebor wrote:
>> I like the idea of being able to use the built-ins for this, but
>> I think it would be confusing for them to follow subtly different
>> rules for C than for C++.  Since the value of the last argument
>
> Here is incremental patch to the patch I've posted earlier today,
> which doesn't promote the last argument of __builtin_*_overflow_p
> and thus for bitfields it behaves pretty much like the C FE.

Looks fine to me.  The bit-field handling should be explained
in the manual.  Though useful, it's unusual enough that I don't
think people will expect it (there have been bug reports or
questions in the past about the C handling of bit-fields from
users familiar with the C++ semantics).

Martin
Joseph Myers June 16, 2016, 2:51 p.m. UTC | #2 
On Wed, 15 Jun 2016, Martin Sebor wrote:

> Looks fine to me.  The bit-field handling should be explained
> in the manual.  Though useful, it's unusual enough that I don't
> think people will expect it (there have been bug reports or
> questions in the past about the C handling of bit-fields from
> users familiar with the C++ semantics).

And at least once bug for C++ (70733) that was closed on the basis of C 
semantics where I don't see that closure as correct under C++ semantics (I 
haven't verified whether the bug report is correct, however).
diff mbox

Patch

--- gcc/cp/call.c.jj	2016-06-15 09:17:22.000000000 +0200
+++ gcc/cp/call.c	2016-06-15 17:06:57.097793446 +0200
@@ -7132,7 +7132,8 @@  convert_for_arg_passing (tree type, tree
    which just decay_conversion or no conversions at all should be done.
    This is true for some builtins which don't act like normal functions.
    Return 2 if no conversions at all should be done, 1 if just
-   decay_conversion.  */
+   decay_conversion.  Return 3 for special treatment of the 3rd argument
+   for __builtin_*_overflow_p.  */
 
 int
 magic_varargs_p (tree fn)
@@ -7149,6 +7150,11 @@  magic_varargs_p (tree fn)
       case BUILT_IN_VA_START:
 	return 1;
 
+      case BUILT_IN_ADD_OVERFLOW_P:
+      case BUILT_IN_SUB_OVERFLOW_P:
+      case BUILT_IN_MUL_OVERFLOW_P:
+	return 3;
+
       default:;
 	return lookup_attribute ("type generic",
 				 TYPE_ATTRIBUTES (TREE_TYPE (fn))) != 0;
@@ -7606,14 +7612,14 @@  build_over_call (struct z_candidate *can
   for (; arg_index < vec_safe_length (args); ++arg_index)
     {
       tree a = (*args)[arg_index];
-      if (magic == 2)
+      if ((magic == 3 && arg_index == 2) || magic == 2)
 	{
 	  /* Do no conversions for certain magic varargs.  */
 	  a = mark_type_use (a);
 	  if (TREE_CODE (a) == FUNCTION_DECL && reject_gcc_builtin (a))
 	    return error_mark_node;
 	}
-      else if (magic == 1)
+      else if (magic != 0)
 	/* For other magic varargs only do decay_conversion.  */
 	a = decay_conversion (a, complain);
       else if (DECL_CONSTRUCTOR_P (fn)
--- gcc/testsuite/c-c++-common/torture/builtin-arith-overflow-p-19.c.jj	2016-06-15 17:13:29.839717235 +0200
+++ gcc/testsuite/c-c++-common/torture/builtin-arith-overflow-p-19.c	2016-06-15 13:05:21.000000000 +0200
@@ -1,5 +1,5 @@ 
 /* Test __builtin_{add,sub,mul}_overflow_p.  */
-/* { dg-do run { target c } } */
+/* { dg-do run } */
 /* { dg-skip-if "" { ! run_expensive_tests }  { "*" } { "-O0" "-O2" } } */
 
 #include "builtin-arith-overflow.h"
--- gcc/testsuite/g++.dg/ext/builtin-arith-overflow-2.C.jj	2016-06-15 17:34:37.617298196 +0200
+++ gcc/testsuite/g++.dg/ext/builtin-arith-overflow-2.C	2016-06-15 17:34:11.000000000 +0200
@@ -0,0 +1,53 @@ 
+// { dg-do run }
+// { dg-options "-O2" }
+
+struct A { int i : 1; };
+struct B { int j : 3; };
+
+template <typename S>
+int
+foo (int x, int y)
+{
+  A a = {};
+  S s = {};
+  return __builtin_add_overflow_p (x, y, a.i) + 2 * __builtin_mul_overflow_p (x, y, s.j);
+}
+
+__attribute__((noinline, noclone)) int
+bar (int x, int y)
+{
+  return foo <B> (x, y);
+}
+
+#if __cplusplus >= 201402L
+template <typename S>
+constexpr int
+baz (int x, int y)
+{
+  A a = {};
+  S s = {};
+  return __builtin_add_overflow_p (x, y, a.i) + 2 * __builtin_mul_overflow_p (x, y, s.j);
+}
+
+constexpr int t1 = baz <B> (0, 0);
+constexpr int t2 = baz <B> (1, -1);
+constexpr int t3 = baz <B> (1, -4);
+constexpr int t4 = baz <B> (-4, 4);
+constexpr int t5 = baz <B> (4, 2);
+static_assert (t1 == 0, "");
+static_assert (t2 == 0, "");
+static_assert (t3 == 1, "");
+static_assert (t4 == 2, "");
+static_assert (t5 == 3, "");
+#endif
+
+int
+main ()
+{
+  if (bar (0, 0) != 0
+      || bar (-1, 1) != 0
+      || bar (-4, 1) != 1
+      || bar (4, -4) != 2
+      || bar (2, 4) != 3)
+    __builtin_abort ();
+}