Reject boolean/enum types in last arg of __builtin_*_overflow_p

Hi!

As mentioned in PR71479, for __builtin_*_overflow we right now require
the last argument to be pointer to INTEGER_TYPE, not INTEGRAL_TYPE_P,
but for __builtin_*_overflow_p we were using INTEGRAL_TYPE_P.

For _Bool/bool, I'd think we could make it well defined if we wanted
(check if the infinite precision result is 0 or 1), but it is hardly useful
and as can be seen, right now we don't implement that (for bool we actually
check if the result fits into unsigned 8 bit integer, plus _Complex bool
is kind of weird), but for enums I think the definition on what is an enum
overflow is quite fuzzy, would it for C++ e.g. work differently between
-fstrict-enums and -fno-strict-enums, would it test overflows only on the
underlying type, or exact precision of the enum, something else?

So, I think it is better to continue what we've been doing already for
__builtin_*_overflow before.  We likely need to adjust the documentation
and possibly the diagnostics.  Maybe have one wording for !INTEGRAL_TYPE_P
and another wording for the != INTEGER_TYPE case (where it would complain
that the type is {pointer to,} {enum,bool,_Bool}?  Any preferences?

The patch below just uses the preexisting wording (where integral stood
for integer types including bool/enums, and integer for non-bool/enum
integer types), I'd just like to see it in soon so that people don't start
to rely on something that doesn't really work and is hard to define.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

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

	* c-common.c (check_builtin_function_arguments): Require last
	argument of BUILT_IN_*_OVERFLOW_P to have INTEGER_TYPE type.

	* c-c++-common/builtin-arith-overflow-1.c (f3): Adjust expected
	diagnostics.
	* g++.dg/ext/builtin-arith-overflow-1.C: Pass 0 instead of C
	as last argument to __builtin_add_overflow_p.


	Jakub

On 06/10/2016 01:08 PM, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in PR71479, for __builtin_*_overflow we right now require
> the last argument to be pointer to INTEGER_TYPE, not INTEGRAL_TYPE_P,
> but for __builtin_*_overflow_p we were using INTEGRAL_TYPE_P.
>
> For _Bool/bool, I'd think we could make it well defined if we wanted
> (check if the infinite precision result is 0 or 1), but it is hardly useful
> and as can be seen, right now we don't implement that (for bool we actually
> check if the result fits into unsigned 8 bit integer

I agree that computing the bool result isn't terribly useful when
the type is known, but allowing it could be handy in generic code
(macros, C _Generic, and C++ templates).  Rejecting it, OTOH, means
that users will have to work a little harder to avoid an error in
generic contexts.  Unless there is some potential problem that I'm
not thinking of I would suggest to accept bool.

IMO, the right semantics for bool are to match what the standards
specify (i.e., no overflow, and the result is a logical OR of the
operands).

Continuing to reject enumerated and complex integer types makes
sense to me.

Martin

On Fri, Jun 10, 2016 at 01:28:55PM -0600, Martin Sebor wrote:
> IMO, the right semantics for bool are to match what the standards
> specify (i.e., no overflow, and the result is a logical OR of the
> operands).

???  It is far from that.
The documentation says that say for __builtin_add_overflow (x, y, &z), you compute
infinite precision signed result of x + y, cast the result to __typeof (z),
extend again to infinite precision signed result and compare to the value
before casting.  So, if z is bool/_Bool, then this means
signed_intNNN_t t = (signed_intNNN_t) x + (signed_intNNN_t) y;
z = (__typeof (z)) t;
ovf = (signed_intNNN_t) z != t;

But, unlike normal signed or unsigned integral types or char, cast to
bool/_Bool is special, it is actually a comparison != 0.
So, I'd say if we want to support arith overflow to bool/_Bool, it should be
above with:
signed_intNNN_t t = (signed_intNNN_t) x + (signed_intNNN_t) y;
z = t != 0;
ovf = (signed_intNNN_t) z != t;
so in the end, if we'd use
movb x, r
addb y, r
seto ovf
to compute whether the result overflowed 8-bits, we'd need to
or in (r & 254) != 0 into the ovf and then or in ovf into r.
That means changes everywhere where we handle the overflow builtins. :(

	Jakub

On Tue, 14 Jun 2016, Jakub Jelinek wrote:

> But, unlike normal signed or unsigned integral types or char, cast to
> bool/_Bool is special, it is actually a comparison != 0.
> So, I'd say if we want to support arith overflow to bool/_Bool, it should be
> above with:
> signed_intNNN_t t = (signed_intNNN_t) x + (signed_intNNN_t) y;
> z = t != 0;
> ovf = (signed_intNNN_t) z != t;

And, I really don't see this as useful.  Rejecting boolean types there 
makes more sense to me.

On Tue, Jun 14, 2016 at 02:05:12PM +0000, Joseph Myers wrote:
> On Tue, 14 Jun 2016, Jakub Jelinek wrote:
> 
> > But, unlike normal signed or unsigned integral types or char, cast to
> > bool/_Bool is special, it is actually a comparison != 0.
> > So, I'd say if we want to support arith overflow to bool/_Bool, it should be
> > above with:
> > signed_intNNN_t t = (signed_intNNN_t) x + (signed_intNNN_t) y;
> > z = t != 0;
> > ovf = (signed_intNNN_t) z != t;
> 
> And, I really don't see this as useful.  Rejecting boolean types there 
> makes more sense to me.

I also don't think supporting booleans is worth all the hassle.

	Marek