Re: [PATCH] integer overflow checking builtins in constant expressions

[Martin Sebor <msebor at gmail dot com>]

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