On Wed, Jun 01, 2016 at 09:10:52PM -0600, Martin Sebor wrote:
> @@ -7957,8 +7957,8 @@ fold_builtin_arith_overflow (location_t loc, enum
> built_in_function fcode,
> tree arg0, tree arg1, tree arg2)
> {
> enum internal_fn ifn = IFN_LAST;
> - tree type = TREE_TYPE (TREE_TYPE (arg2));
> - tree mem_arg2 = build_fold_indirect_ref_loc (loc, arg2);
> + enum tree_code opcode;
> +
> switch (fcode)
> {
> case BUILT_IN_ADD_OVERFLOW:
> @@ -7969,6 +7969,7 @@ fold_builtin_arith_overflow (location_t loc, enum
> built_in_function fcode,
> case BUILT_IN_UADDL_OVERFLOW:
> case BUILT_IN_UADDLL_OVERFLOW:
> ifn = IFN_ADD_OVERFLOW;
> + opcode = PLUS_EXPR;
> break;
> case BUILT_IN_SUB_OVERFLOW:
> case BUILT_IN_SSUB_OVERFLOW:
> @@ -7978,6 +7979,7 @@ fold_builtin_arith_overflow (location_t loc, enum
> built_in_function fcode,
> case BUILT_IN_USUBL_OVERFLOW:
> case BUILT_IN_USUBLL_OVERFLOW:
> ifn = IFN_SUB_OVERFLOW;
> + opcode = MINUS_EXPR;
> break;
> case BUILT_IN_MUL_OVERFLOW:
> case BUILT_IN_SMUL_OVERFLOW:
> @@ -7987,10 +7989,35 @@ fold_builtin_arith_overflow (location_t loc, enum
> built_in_function fcode,
> case BUILT_IN_UMULL_OVERFLOW:
> case BUILT_IN_UMULLL_OVERFLOW:
> ifn = IFN_MUL_OVERFLOW;
> + opcode = MULT_EXPR;
> break;
> default:
> gcc_unreachable ();
> }
> +
> + /* For the "generic" overloads, the first two arguments can have different
> + types and the last argument determines the target type to use to check
> + for overflow. The arguments of the other overloads all have the same
> + type. */
> + tree type = TREE_TYPE (TREE_TYPE (arg2));
> + bool isnullp = integer_zerop (arg2);
> +
> + /* 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 (isnullp
> + && TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
Doesn't this mean you allow NULL arg2 even for BUILT_IN_ADDL_OVERFLOW and
similar clang builtins? I'd set opcode to ERROR_MARK first, and then do:
switch (fcode)
{
case BUILT_IN_ADD_OVERFLOW:
opcode = PLUS_EXPR;
/* FALLTHROUGH */
case BUILT_IN_SADD_OVERFLOW:
case BUILT_IN_SADDL_OVERFLOW:
case BUILT_IN_SADDLL_OVERFLOW:
case BUILT_IN_UADD_OVERFLOW:
case BUILT_IN_UADDL_OVERFLOW:
case BUILT_IN_UADDLL_OVERFLOW:
ifn = IFN_ADD_OVERFLOW;
break;
and similarly, then
bool isnullp = opcode != ERROR_MARK ? integer_zerop (arg2) : false;
or so.
> + {
> + /* Perform the computation in the target type and check for overflow.
> */
> + arg0 = fold_convert (type, arg0);
> + arg1 = fold_convert (type, arg1);
> +
> + if (tree result = size_binop_loc (loc, opcode, arg0, arg1))
> + return TREE_OVERFLOW (result) ? build_one_cst (boolean_type_node)
> + : build_zero_cst (boolean_type_node);
This is wrong, it is not the computation of overflow that is intended.
The documentation says that we compute (infinite_precision_signed_type) arg0
op (infinite_precision_signed_type) arg1 and then cast it to type, extend
again to infinite_precision_signed_type and compare. And we have a helper
function for that already.
Furthermore, we have boolean_true_node and boolean_false_node.
Thus, I'd expect here:
return (arith_overflowed_p (opcode, type, arg0, arg1)
? boolean_true_node : boolean_false_node);
> + tree arg0 = cxx_eval_constant_expression (ctx, CALL_EXPR_ARG (t, 0), lval,
> + non_constant_p, overflow_p);
> + tree arg1 = cxx_eval_constant_expression (ctx, CALL_EXPR_ARG (t, 1), lval,
> + non_constant_p, overflow_p);
> +
> + if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
> + {
> + if (tree result = size_binop_loc (EXPR_LOC_OR_LOC (t, input_location),
> + opcode, arg0, arg1))
> + {
> + if (TREE_OVERFLOW (result))
> + {
> + /* Reset TREE_OVERFLOW to avoid warnings for the overflow. */
> + TREE_OVERFLOW (result) = 0;
Again, this is wrong, it should have used arith_overflowed_p.
> @@ -1270,18 +1332,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx,
> tree t,
> bool depth_ok;
>
> if (fun == NULL_TREE)
> - switch (CALL_EXPR_IFN (t))
> - {
> - case IFN_UBSAN_NULL:
> - case IFN_UBSAN_BOUNDS:
> - case IFN_UBSAN_VPTR:
> - return void_node;
> - default:
> - if (!ctx->quiet)
> - error_at (loc, "call to internal function");
> - *non_constant_p = true;
> - return t;
> - }
> + return cxx_eval_internal_function (ctx, t, lval,
> + non_constant_p, overflow_p);
>
> if (TREE_CODE (fun) != FUNCTION_DECL)
> {
> diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
> index 04702ee..f9db199 100644
> --- a/gcc/cp/tree.c
> +++ b/gcc/cp/tree.c
> @@ -352,6 +352,32 @@ builtin_valid_in_constant_expr_p (const_tree decl)
> case BUILT_IN_FUNCTION:
> case BUILT_IN_LINE:
>
> + /* The following built-ins are valid in constant expressions
> + when their arguments are. */
> + case BUILT_IN_ADD_OVERFLOW:
> + case BUILT_IN_SADD_OVERFLOW:
> + case BUILT_IN_SADDL_OVERFLOW:
> + case BUILT_IN_SADDLL_OVERFLOW:
> + case BUILT_IN_UADD_OVERFLOW:
> + case BUILT_IN_UADDL_OVERFLOW:
> + case BUILT_IN_UADDLL_OVERFLOW:
> +
> + case BUILT_IN_SUB_OVERFLOW:
> + case BUILT_IN_SSUB_OVERFLOW:
> + case BUILT_IN_SSUBL_OVERFLOW:
> + case BUILT_IN_SSUBLL_OVERFLOW:
> + case BUILT_IN_USUB_OVERFLOW:
> + case BUILT_IN_USUBL_OVERFLOW:
> + case BUILT_IN_USUBLL_OVERFLOW:
> +
> + case BUILT_IN_MUL_OVERFLOW:
> + case BUILT_IN_SMUL_OVERFLOW:
> + case BUILT_IN_SMULL_OVERFLOW:
> + case BUILT_IN_SMULLL_OVERFLOW:
> + case BUILT_IN_UMUL_OVERFLOW:
> + case BUILT_IN_UMULL_OVERFLOW:
> + case BUILT_IN_UMULLL_OVERFLOW:
> +
> /* These have constant results even if their operands are
> non-constant. */
> case BUILT_IN_CONSTANT_P:
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 2d4f028..00435f7 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -9737,7 +9737,10 @@ compiler may also ignore this parameter.
> @section Built-in Functions to Perform Arithmetic with Overflow Checking
>
> The following built-in functions allow performing simple arithmetic
> operations
> -together with checking whether the operations overflowed.
> +together with checking whether the operations overflowed. The first of the
> +functions accepts either a pointer to an integer object or a null pointer to
> +integer as the last argument. The rest require a pointer to an object of
> +the specified type as the last argument.
>
> @deftypefn {Built-in Function} bool __builtin_add_overflow (@var{type1} a,
> @var{type2} b, @var{type3} *res)
> @deftypefnx {Built-in Function} bool __builtin_sadd_overflow (int a, int b,
> int *res)
> @@ -9747,21 +9750,50 @@ together with checking whether the operations
> overflowed.
> @deftypefnx {Built-in Function} bool __builtin_uaddl_overflow (unsigned long
> int a, unsigned long int b, unsigned long int *res)
> @deftypefnx {Built-in Function} bool __builtin_uaddll_overflow (unsigned
> long long int a, unsigned long long int b, unsigned long int *res)
>
> -These built-in functions promote the first two operands into infinite
> precision signed
> -type and perform addition on those promoted operands. The result is then
> -cast to the type the third pointer argument points to and stored there.
> -If the stored result is equal to the infinite precision result, the built-in
> -functions return false, otherwise they return true. As the addition is
> -performed in infinite signed precision, these built-in functions have fully
> defined
> -behavior for all argument values.
> -
> -The first built-in function allows arbitrary integral types for operands and
> -the result type must be pointer to some integer type, the rest of the
> built-in
> -functions have explicit integer types.
> +These built-in functions promote the first two operands into infinite
> precision
> +signed type and perform addition on those promoted operands. The result is
> then
> +converted to the type the third pointer argument points to, and for the first
> +function when the pointer is not null, stored there. If the converted result
> +is equal to the infinite precision result, the built-in functions return
> +@code{false}, otherwise they return @code{true} to indicate that an overflow
> +has been detected. Because the addition is performed in infinite precision,
> +these built-in functions have fully defined behavior for all argument values
> +and integer types.
> +
> +The first type-generic built-in function allows arbitrary integer types as
> +the first two arguments and requires that a pointer to some possibly distinct
> +integer type be passed to it as the third argument. The pointed-to type is
> +then used to determine the overflow. As a result, this built-in function
> +can be used to detect overflow in any arbitrary integer type, including
> +@code{char} and @code{short}. The remaining built-in functions take
> +arguments of explicit integer types and make it possible to determine
> +overflow only in the ranges of those types. Since the only provided forms
> +of these latter built-in functions are for the signed and unsigned variants
> +of types @code{int}, @code{long}, and @code{long long}, they cannot be used
> +to determine overflow in other integer types.
> +
> +To enable the efficient integer overflow detection at translation-time,
> +in C and C++ 11 and later programs (but not in C++ 98), the first built-in
AFAIK the docs use either C++98, C++11 (without space), or
ISO/IEC 14882:2011 etc., but not C++ 98 or C++ 11.
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);
Jakub
