From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from quail.birch.relay.mailchannels.net (quail.birch.relay.mailchannels.net [23.83.209.151]) by sourceware.org (Postfix) with ESMTPS id 047053858D28 for ; Tue, 11 Jan 2022 00:32:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 047053858D28 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 9EBBF21515; Tue, 11 Jan 2022 00:32:14 +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 32C2221830; Tue, 11 Jan 2022 00:32:14 +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.114.70.197 (trex/6.4.3); Tue, 11 Jan 2022 00:32:14 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Coil-Illegal: 56a7cced7cbc00a7_1641861134452_4011209168 X-MC-Loop-Signature: 1641861134452:3392967542 X-MC-Ingress-Time: 1641861134452 Received: from [192.168.1.174] (unknown [1.186.121.232]) (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 4JXs8X5Pf6z1Ph; Mon, 10 Jan 2022 16:32:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gotplt.org; s=gotplt.org; t=1641861133; bh=uF0Si0K8VgaD/bWBiGDjbz7XvLo=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=aamZ7uIRMklFBGfUD5fu5y4P+Vy7f35xRKwgj84nJINNpraPlzhbxkncFUgGG7NDK P9hnFXfDVP9h2JDSqxqHD1i1n2Vv0hqKBOu7d7OjlUsWYfEUFBr17yDDGDtXedVL0D uaNgpoOD1IvFCLDVT/G3EtnydeZ1XkvjZi6HJD3I= Message-ID: <5bba7c35-0065-cc71-c4a4-7a529e7ca226@gotplt.org> Date: Tue, 11 Jan 2022 06:02:07 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH v5 2/4] tree-object-size: Handle function parameters Content-Language: en-US To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org References: <20211109190137.1107736-1-siddhesh@gotplt.org> <20211218123511.139456-1-siddhesh@gotplt.org> <20211218123511.139456-3-siddhesh@gotplt.org> <20220110105024.GE2646553@tucnak> From: Siddhesh Poyarekar In-Reply-To: <20220110105024.GE2646553@tucnak> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3030.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, RCVD_IN_SBL, 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, 11 Jan 2022 00:32:18 -0000 On 10/01/2022 16:20, Jakub Jelinek wrote: > On Sat, Dec 18, 2021 at 06:05:09PM +0530, Siddhesh Poyarekar wrote: >> @@ -1440,6 +1441,53 @@ cond_expr_object_size (struct object_size_info *osi, tree var, gimple *stmt) >> return reexamine; >> } >> >> +/* Find size of an object passed as a parameter to the function. */ >> + >> +static void >> +parm_object_size (struct object_size_info *osi, tree var) >> +{ >> + int object_size_type = osi->object_size_type; >> + tree parm = SSA_NAME_VAR (var); >> + >> + if (!(object_size_type & OST_DYNAMIC) || !POINTER_TYPE_P (TREE_TYPE (parm))) >> + expr_object_size (osi, var, parm); > > This looks very suspicious. Didn't you mean { expr_object_size (...); return; } here? > Because the code below e.g. certainly assumes OST_DYNAMIC and that TREE_TYPE (parm) > is a pointer type (otherwise TREE_TYPE (TREE_TYPE (...) wouldn't work. Indeed, fixed. > >> + >> + /* Look for access attribute. */ >> + rdwr_map rdwr_idx; >> + >> + tree fndecl = cfun->decl; >> + const attr_access *access = get_parm_access (rdwr_idx, parm, fndecl); >> + tree typesize = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm))); >> + tree sz = NULL_TREE; >> + >> + if (access && access->sizarg != UINT_MAX) > > Perhaps && typesize here? It makes no sense to e.g. create ssa default def > when you aren't going to use it in any way. The typesize is only for scaling; the result of get_or_create_ssa_default_def should get returned unscaled if it is non-NULL and typesize is NULL; the latter happens when the type is void *: sz = get_or_create_ssa_default_def (cfun, arg); if (sz != NULL_TREE) { sz = fold_convert (sizetype, sz); if (typesize) sz = size_binop (MULT_EXPR, sz, typesize); } } > >> + { >> + tree fnargs = DECL_ARGUMENTS (fndecl); >> + tree arg = NULL_TREE; >> + unsigned argpos = 0; >> + >> + /* Walk through the parameters to pick the size parameter and safely >> + scale it by the type size. */ >> + for (arg = fnargs; argpos != access->sizarg && arg; >> + arg = TREE_CHAIN (arg), ++argpos); > > Instead of a loop with empty body wouldn't it be better to > do the work in that for loop? > I.e. take argpos != access->sizarg && from the condition, > replace arg != NULL_TREE with that argpos == access->sizarg > and add a break;? Fixed. > >> + >> + if (arg != NULL_TREE && INTEGRAL_TYPE_P (TREE_TYPE (arg))) >> + { >> + sz = get_or_create_ssa_default_def (cfun, arg); > > Also, I must say I'm little bit worried about this > get_or_create_ssa_default_def call. If the SSA_NAME doesn't exist, > so you create it and then attempt to use it but in the end don't > because e.g. some PHI's another argument was unknown etc., will > that SSA_NAME be released through release_ssa_name? > I think GIMPLE is fairly unhappy if there are SSA_NAMEs created and not > released that don't appear in the IL anywhere. AFAICT, set_ss_default_def ends up creating a definition for the new SSA_NAME it creates, so it does end up in the IR and in case of object size computation failure, it just ends up being a dead store. I've added a test to verify this: size_t __attribute__ ((access (__read_write__, 1, 3))) __attribute__ ((noinline)) test_parmsz_unknown (void *obj, void *unknown, size_t sz, int cond) { return __builtin_dynamic_object_size (cond ? obj : unknown, 0); } which works as expected and returns -1. Thanks, Siddhesh