[PATCH] integer overflow checking builtins in constant expressions

[Martin Sebor]

On 06/02/2016 01:34 AM, Jakub Jelinek wrote:
> On Thu, Jun 02, 2016 at 09:23:16AM +0200, Jakub Jelinek wrote:
>> Also, perhaps just a documentation thing, it would be good to clarify the
>> NULL last argument.  From the POV of generating efficient code, I think we
>> should say something that the last argument (for the GNU builtins) must be
>> either a pointer to a valid object, or NULL/nullptr constant cast expression
>> cast to a pointer type, but nothing else.  That is actually what your patch
>> implements.  But, I'd like to make sure that
>>    int *p = NULL;
>>    if (__builtin_add_overflow (a, b, p))
>>      ...
>> is actually not valid, otherwise we unnecessarily pessimize many of the GNU
>> style calls (those that don't pass &var), where instead of
>>    tem = ADD_OVERFLOW (a, b);
>>    *p = REALPART_EXPR (tem);
>>    ovf = IMAGPART_EXPR (tem);
>> we'd need to emit instead
>>    tem = ADD_OVERFLOW (a, b);
>>    ovf = IMAGPART_EXPR (tem);
>>    if (p != NULL)
>>      *p = REALPART_EXPR (tem);
>
> Though, perhaps that is too ugly, that it has different semantics for
>    __builtin_add_overflow (a, b, (int *) NULL)
> and for
>    int *p = NULL;
>    __builtin_add_overflow (a, b, p)
> Maybe the cleanest would be to just add 3 extra builtins, again,
> typegeneric,
>    __builtin_{add,sub,mul}_overflow_p
> where either the arguments would be instead of integertype1, integertype2,
> integertype3 * rather integertype1, integertype2, integertype3
> and we'd only care about the type, not value, of the last argument,
> so use it like __builtin_add_overflow_p (a, b, (__typeof ((a) + (b))) 0)
> or handle those 3 similarly to __builtin_va_arg, and use
> __builtin_add_overflow_p (a, b, int);
> I think I prefer the former though.

I'm not sure I understand your concern but at this point I would
prefer to keep things as they are.  I like that the functionality
that was requested in c/68120 could be provided under the existing
interface, and I'm not fond of the idea of adding yet another set
of built-ins just to get at a bit that's already available via
the existing ones (in C++ 11, even in constexpr contexts, provided
the built-ins were allowed there).  I've also spent far more time
on this patch than I had planned and I'm way behind on the tasks
I was asked to work on.

That said, in c/68120 the requester commented that he's considering
submitting a request for yet another enhancement in this area, so
I think letting him play with what we've got now for a bit will be
an opportunity to get his feedback and tweak the API further based
on it.

Martin