[cfe-dev] [RFC] Introduce overflow builtins

[Dave Zarzycki]

On Mar 31, 2012, at 1:02 PM, Xi Wang <xi.wang at gmail.com> wrote:

> On Mar 30, 2012, at 4:32 PM, Dave Zarzycki wrote:
> 
>> Have you tried inferring the sign of T* in addition to the size of T* in the API? That would let you simplify this:
>> 
>> 	__builtin_sadd_with_overflow()
>> 	__builtin_uadd_with_overflow()
>> 	__builtin_ssub_with_overflow()
>> 	__builtin_usub_with_overflow()
>> 	__builtin_smul_with_overflow()
>> 	__builtin_umul_with_overflow()
>> 
>> To this:
>> 
>> 	__builtin_add_with_overflow()
>> 	__builtin_sub_with_overflow()
>> 	__builtin_mul_with_overflow()
> 
> Sounds like a good idea.

Great!

> I am just a little worried that this is more error-prone than explicitly specifying s/u (e.g., if the programmer accidentally mixes signed and unsigned integers).  -Wconversion would help (see below), but I guess the option is turned off in general.

I wouldn't worry about that. Making the above overflow intrinsics explicitly signed or unsigned will not magically make a developer care about sign conversion or size conversion problems.

However, making the intrinsics explicitly signed will make the intrinsics more inconvenient to use for developers that know what they're doing. Also, making the intrinsics explicitly signed will force developers to change all of their uses of these intrinsics every time the change the sign of a variable. Both of these problems are avoidable by having the intrinsics do the right thing and infer the sign of the operation based on T*.


>> Also, have you verified that -Wconversion does the right thing and warns when the sign of the parameters are inconsistent with each other (or the API if you do not make the above simplification)?
> 
> I tried a few examples and -Wconversion seemed to work.  Below is an example.

The patch does not validate that the correct intrinsic is used. For example:

/tmp/llvm/b $ cat of.c
int example(int x, int y, int z);
int example(int x, int y, int z) {
	if (__builtin_uadd_with_overflow(&x, y, z)) __builtin_trap();
	return x;
}

/tmp/llvm/b $ ./Debug+Asserts/bin/clang -Weverything -Os -c of.c
/tmp/llvm/b $ echo $?
0
/tmp/llvm/b $

Thanks for working on improving this patch!

davez