From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cross.elm.relay.mailchannels.net (cross.elm.relay.mailchannels.net [23.83.212.46]) by sourceware.org (Postfix) with ESMTPS id 8A7143857C4A for ; Tue, 23 Nov 2021 13:53:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8A7143857C4A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id E4974621F82; Tue, 23 Nov 2021 13:53:08 +0000 (UTC) Received: from pdx1-sub0-mail-a306.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 6DE44621D98; Tue, 23 Nov 2021 13:53:08 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a306.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.109.250.31 (trex/6.4.3); Tue, 23 Nov 2021 13:53:08 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Bottle-Shoe: 79575e573f1f13cb_1637675588740_744145059 X-MC-Loop-Signature: 1637675588740:2556482140 X-MC-Ingress-Time: 1637675588740 Received: from [192.168.1.174] (unknown [1.186.224.140]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a306.dreamhost.com (Postfix) with ESMTPSA id 4Hz5FG5zscz2x; Tue, 23 Nov 2021 05:53:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gotplt.org; s=gotplt.org; t=1637675588; bh=7UBUeRN8o3oYUUeIVndbXTuYnsQ=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=ew1up4mPlUTwq2LmT+myzcWjvdvGJBBk97gvZMd8hRCpSl5mDbFvD69LjpLTANZix 7zpAAQu3qcx5VciUnoyZuDt9tvEZo/pmFU9G9z4FuTegOgddfNX3AR9Ljs/cN+tnt1 pl08y9UF7LNutWXzAeZDAHIAezHOhXT3fXVPhaZc= Message-ID: <8034fc39-9efc-30f8-6abe-f61f2bc30fbb@gotplt.org> Date: Tue, 23 Nov 2021 19:23:01 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 Subject: Re: [PATCH 05/10] __builtin_dynamic_object_size: Recognize builtin Content-Language: en-US To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org References: <20211109190137.1107736-1-siddhesh@gotplt.org> <20211109190137.1107736-6-siddhesh@gotplt.org> <20211123124149.GW2646553@tucnak> From: Siddhesh Poyarekar In-Reply-To: <20211123124149.GW2646553@tucnak> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3029.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_BL_SPAMCOP_NET, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Nov 2021 13:53:15 -0000 On 11/23/21 18:11, Jakub Jelinek wrote: > On Wed, Nov 10, 2021 at 12:31:31AM +0530, Siddhesh Poyarekar wrote: >> Recognize the __builtin_dynamic_object_size builtin and add paths in the >> object size path to deal with it, but treat it like >> __builtin_object_size for now. Also add tests to provide the same >> testing coverage for the new builtin name. >> >> gcc/ChangeLog: >> >> * builtins.def (BUILT_IN_DYNAMIC_OBJECT_SIZE): New builtin. >> * tree-object-size.h (compute_builtin_object_size): Add new >> argument dynamic. >> * builtins.c (expand_builtin, fold_builtin_2): Handle it. >> (fold_builtin_object_size): Handle new builtin and adjust for >> change to compute_builtin_object_size. >> * tree-object-size.c: Include builtins.h. >> (OST_DYNAMIC): New enum value. >> (compute_builtin_object_size): Add new argument dynamic. >> (addr_object_size): Adjust. >> (early_object_sizes_execute_one, >> dynamic_object_sizes_execute_one): New functions. >> (object_sizes_execute): Rename insert_min_max_p argument to >> early. Handle BUILT_IN_DYNAMIC_OBJECT_SIZE and call the new > > Two spaces after . instead of just one. > >> --- a/gcc/builtins.def >> +++ b/gcc/builtins.def >> @@ -972,6 +972,7 @@ DEF_BUILTIN_STUB (BUILT_IN_STRNCMP_EQ, "__builtin_strncmp_eq") >> >> /* Object size checking builtins. */ >> DEF_GCC_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_CONST_NOTHROW_LEAF_LIST) >> +DEF_GCC_BUILTIN (BUILT_IN_DYNAMIC_OBJECT_SIZE, "dynamic_object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_NOTHROW_LEAF_LIST) > > Are you sure about the omission of CONST_ in there? > If I do: > size_t a = __builtin_dynamic_object_size (x, 0); > size_t b = __builtin_dynamic_object_size (x, 0); > I'd expect the compiler to perform it just once. While it might actually do > it eventually after objsz2 pass lowers it, with the above it won't really do > it. Perhaps const attribute isn't really safe, the function might need to > read some memory in order to compute the return value, but certainly it will > not store to any memory, so perhaps > ATTR_PURE_NOTHROW_LEAF_LIST ? Thanks, I'll fix this. > >> +#define DYNAMIC_OBJECT_SIZE > > Why this extra macro? > >> +#define __builtin_object_size __builtin_dynamic_object_size > >> extern char ax[]; >> +#ifndef DYNAMIC_OBJECT_SIZE > > You can #ifndef __builtin_object_size > instead... I'll fix this too. > >> @@ -371,7 +373,8 @@ addr_object_size (struct object_size_info *osi, const_tree ptr, >> || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME) >> { >> compute_builtin_object_size (TREE_OPERAND (pt_var, 0), >> - object_size_type & ~OST_SUBOBJECT, &sz); >> + object_size_type & OST_MINIMUM, &sz, >> + object_size_type & OST_DYNAMIC); >> } >> else >> { >> @@ -835,9 +838,10 @@ resolve_dependency_loops (struct object_size_info *osi) >> >> bool >> compute_builtin_object_size (tree ptr, int object_size_type, >> - tree *psize) >> + tree *psize, bool dynamic) >> { >> gcc_assert (object_size_type >= 0 && object_size_type < OST_END); >> + object_size_type |= dynamic ? OST_DYNAMIC : 0; > > What's the advantage of another argument and then merging it with > object_size_type over just passing object_size_type which will have > all the bits in? I kept the size bits as an internal detail, I can define them in tree-object-size.h and hae builtins.c (and others) use them. >> +static void >> +early_object_sizes_execute_one (gimple_stmt_iterator *i, gimple *call) >> +{ >> + tree ost = gimple_call_arg (call, 1); >> + tree lhs = gimple_call_lhs (call); >> + gcc_assert (lhs != NULL_TREE); >> + >> + if (tree_fits_uhwi_p (ost)) >> + { >> + unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost); >> + tree ptr = gimple_call_arg (call, 0); >> + if ((object_size_type == 1 || object_size_type == 3) >> + && (TREE_CODE (ptr) == ADDR_EXPR || TREE_CODE (ptr) == SSA_NAME)) > > I think it would be better to have early exits there to avoid > indenting most of the function too much, because the function doesn't > do anything otherwise. So: > if (!tree_fits_uhwi_p (ost)) > return; OK, I'll fix this. > > unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost); > tree ptr = gimple_call_arg (call, 0); > if (object_size_type != 1 && object_size_type != 3) > return; > if (TREE_CODE (ptr) != ADDR_EXPR && TREE_CODE (ptr) != SSA_NAME) > return; > > tree type = ... > >> + { >> + tree type = TREE_TYPE (lhs); >> + tree bytes; >> + if (compute_builtin_object_size (ptr, object_size_type, &bytes)) >> + { >> + tree tem = make_ssa_name (type); >> + gimple_call_set_lhs (call, tem); >> + enum tree_code code >> + = object_size_type & OST_MINIMUM ? MAX_EXPR : MIN_EXPR; >> + tree cst = fold_convert (type, bytes); >> + gimple *g = gimple_build_assign (lhs, code, tem, cst); >> + gsi_insert_after (i, g, GSI_NEW_STMT); >> + update_stmt (call); >> + } >> + } >> + } > >> +/* Attempt to fold one __builtin_dynamic_object_size call in CALL into an >> + expression and insert it at I. Return true if it succeeds. */ >> + >> +static bool >> +dynamic_object_sizes_execute_one (gimple_stmt_iterator *i, gimple *call) >> +{ >> + unsigned numargs = gimple_call_num_args (call); >> + tree *args = XALLOCAVEC (tree, numargs); >> + args[0] = gimple_call_arg (call, 0); >> + args[1] = gimple_call_arg (call, 1); > > Why it would have numargs different from 2? It is a builtin, and we reject > ((__SIZE_TYPE__ (*) (const void *, int, int)) &__builtin_object_size) (x, 0, 3) > etc. with error that the builtin must be directly called. > And after all, you rely on it having at least 2 arguments anyway. > So, just tree args[2]; instead of XALLOCAVEC and if you want, > gcc_assert (numargs == 2); > ? Thanks, I'll fix this too. Siddhesh