From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68947 invoked by alias); 2 Jun 2016 07:23:27 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 68929 invoked by uid 89); 2 Jun 2016 07:23:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 02 Jun 2016 07:23:24 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 15FFB71095; Thu, 2 Jun 2016 07:23:23 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-116-51.ams2.redhat.com [10.36.116.51]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u527NLBI028308 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 2 Jun 2016 03:23:22 -0400 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id u527NJYI003040; Thu, 2 Jun 2016 09:23:19 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id u527NGmj003039; Thu, 2 Jun 2016 09:23:16 +0200 Date: Thu, 02 Jun 2016 07:23:00 -0000 From: Jakub Jelinek To: Martin Sebor Cc: Gcc Patch List , Jason Merrill , "Joseph S. Myers" Subject: Re: [PATCH] integer overflow checking builtins in constant expressions Message-ID: <20160602072316.GY28550@tucnak.redhat.com> Reply-To: Jakub Jelinek References: <57263154.5080401@gmail.com> <20160531215025.GK28550@tucnak.redhat.com> <574E13DD.9040401@gmail.com> <20160601075212.GL28550@tucnak.redhat.com> <574EFC8F.2040400@gmail.com> <574FA3BC.8090603@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <574FA3BC.8090603@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes X-SW-Source: 2016-06/txt/msg00114.txt.bz2 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