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

[Jakub Jelinek <jakub@redhat.com>]

On Tue, May 31, 2016 at 04:44:45PM -0600, Martin Sebor wrote:
> >I fail to understand this.  You set type to NULL_TREE, and then (not in any
> >kind of loop, nor any label you could goto to) do:
> >   type = type ? type : xxx;
> >That is necessarily equal to type = xxx;, isn't it?
> 
> Each label falls through to the next, so the test prevents
> the subsequent labels from overwriting the value assigned
> to type by the label that matched the switch expression.
> I'm not exactly enamored with these repetitive tests but
> the only alternative that occurred to me was to duplicate
> the whole switch statement, and this seemed like the lesser
> of the two evils.  If you have a suggestion for something
> better I'd be happy to change it.

Ugh, I've missed missing break; Anyway, IMHO we shouldn't change
the old style builtins and therefore you really don't need this.

> >>+  /* When the last argument is a NULL pointer and the first two arguments
> >>+     are constant, attempt to fold the built-in call into a constant
> >>+     expression indicating whether or not it detected an overflow but
> >>+     without storing the result.  */
> >>+  if ((!arg2 || zerop (arg2))
> >
> >As I said in another mail, IMNSHO you don't want to change the clang
> >compatibility builtins, therefore arg2 should never be NULL.
> 
> Allowing the last argument to be null is the subject of
> the enhancement request in c/68120 (one of the two PRs
> driving this enhancement).  The argument can only be null
> for these overloads and not the type-generic ones because
> the latter use the type to which the pointer points to
> determine the type in which to do the arithmetic.  Without
> this change, C or C++ 98 users wouldn't be able to use the
> built-ins in constant expressions (C++ 98 doesn't allow
> casting a null pointer in those contexts).

Sorry, I don't understand this argument.  What is wrong on using
const int x = __builtin_add_overflow (1234567, 123456, (int *) NULL);
?
I don't get any warning/error on
#include <stddef.h>
int *const p = (int *) NULL;
in C89 nor C++98 with -pedantic-errors, not to mention that the
builtins are extensions anyway, so what do you accept/reject in its
arguments doesn't need to honor any specific rules.  Even if (int *) NULL
is not allowed for whatever reason, is (int *) 0 disallowed too?  It is
just a matter of what we document.

The clang compatibility builtins are severely limited and too ugly to live
with, they don't allow different types of the arguments, have just a very
limited set of accepted types, don't allow different result
type, how do you even handle a type for which you don't know if it is
unsigned, unsigned long or unsigned long long?
  size_t a, b, c, d;
...
  if (sizeof (size_t) == sizeof (unsigned long long))
    a = __builtin_uaddll_overflow (b, c, &d);
  else if (sizeof (size_t) == sizeof (unsigned long))
    a = __builtin_uaddl_overflow (b, c, &d);
  else if (sizeof (size_t) == sizeof (unsigned int))
    a = __builtin_uadd_overflow (b, c, &d);
  else
    abort ();
?  We really shouldn't encourage these at all.

> I suspect I didn't use integer_zerop because the argument
> is a pointer and not integer, but I'll change it before
> I commit the patch.

Pointers are treated the same as integers, they are represented as
INTEGER_CSTs too.
> 
> >Regarding the varargs vs. named args only different diagnostics, if you
> >think it should change, then IMHO you should submit as independent patch
> >a change to the diagnostics wording, so that the wording is the same, rather
> >than changing functions from fixed arguments to varargs (/ typegeneric).
> 
> I'm not sure I know what part of the patch you're referring
> to.  Are you suggesting to submit a separate patch for the
> change from "not enough arguments" to "too few arguments"
> below?
> 
> -		"not enough arguments to function %qE", fndecl);
> +		"too few arguments to function %qE", fndecl);

Yeah.  IMO that is an independent change, you can/should add a testcase for
it, and it can go in before or after the patch.

	Jakub