From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from butterfly.birch.relay.mailchannels.net (butterfly.birch.relay.mailchannels.net [23.83.209.27]) by sourceware.org (Postfix) with ESMTPS id 921023858D28 for ; Wed, 15 Dec 2021 17:57:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 921023858D28 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 56BA5828E29; Wed, 15 Dec 2021 17:56:57 +0000 (UTC) Received: from pdx1-sub0-mail-a304.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id C5756828DE5; Wed, 15 Dec 2021 17:56:56 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a304.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.114.196.210 (trex/6.4.3); Wed, 15 Dec 2021 17:56:57 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Attack-Turn: 74bfaf446b68dfe5_1639591017134_3259927543 X-MC-Loop-Signature: 1639591017134:3934422728 X-MC-Ingress-Time: 1639591017134 Received: from [192.168.52.116] (unknown [223.185.62.238]) (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-a304.dreamhost.com (Postfix) with ESMTPSA id 4JDjcP2LsTz1P7; Wed, 15 Dec 2021 09:56:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gotplt.org; s=gotplt.org; t=1639591015; bh=80ZOGsCdJbfU58bODCLc2GqdzjY=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=uy1dfSLWJmVkS1pHxIKrFyPhFlftKEBZGNcEvPpGsZ1aPFErjAza/cpSz0uOKDdqj V8oSWq84+pjPUcBt9e/9ltNtqq76NXMw/f0E+SkWrTf67T2Mnk9SGlvkyB05/pzy4U qf6BaInhexaD1Za2guqLhanReUPp+x9Xo9ASW0Y0= Message-ID: <1c596138-c500-bf62-0e1f-28063fb9edc3@gotplt.org> Date: Wed, 15 Dec 2021 23:26:48 +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 v4 3/6] tree-object-size: Support dynamic sizes in conditions Content-Language: en-US To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org References: <20211109190137.1107736-1-siddhesh@gotplt.org> <20211201142757.4086840-1-siddhesh@gotplt.org> <20211201142757.4086840-4-siddhesh@gotplt.org> <20211215162413.GL2646553@tucnak> From: Siddhesh Poyarekar In-Reply-To: <20211215162413.GL2646553@tucnak> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3031.0 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, 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: Wed, 15 Dec 2021 17:57:03 -0000 On 12/15/21 21:54, Jakub Jelinek wrote: > On Wed, Dec 01, 2021 at 07:57:54PM +0530, Siddhesh Poyarekar wrote: >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c >> @@ -0,0 +1,72 @@ >> +/* { dg-do run } */ >> +/* { dg-options "-O2" } */ >> + >> +typedef __SIZE_TYPE__ size_t; >> +#define abort __builtin_abort >> + >> +size_t >> +__attribute__ ((noinline)) >> +test_builtin_malloc_condphi (int cond) >> +{ >> + void *ret; >> + >> + if (cond) >> + ret = __builtin_malloc (32); >> + else >> + ret = __builtin_malloc (64); >> + >> + return __builtin_dynamic_object_size (ret, 0); >> +} >> + >> +size_t >> +__attribute__ ((noinline)) >> +test_builtin_calloc_condphi (size_t cnt, size_t sz, int cond) >> +{ >> + struct >> + { >> + int a; >> + char b; >> + } bin[cnt]; >> + >> + char *ch = __builtin_calloc (cnt, sz); >> + >> + return __builtin_dynamic_object_size (cond ? ch : (void *) &bin, 0); > > I think it would be nice if the testcases didn't leak memory, can > you replace return ... with size_t ret = > and add > __builtin_free (ch); > return ret; > in both cases (in the first perhaps rename ret to ch first. > OK, I'll fix up all patches for this. >> --- a/gcc/testsuite/gcc.dg/builtin-object-size-5.c >> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-5.c >> @@ -1,5 +1,7 @@ >> /* { dg-do compile { target i?86-*-linux* i?86-*-gnu* x86_64-*-linux* } } */ >> /* { dg-options "-O2" } */ >> +/* For dynamic object sizes we 'succeed' if the returned size is known for >> + maximum object size. */ >> >> typedef __SIZE_TYPE__ size_t; >> extern void abort (void); >> @@ -13,7 +15,11 @@ test1 (size_t x) >> >> for (i = 0; i < x; ++i) >> p = p + 4; >> +#ifdef __builtin_object_size >> + if (__builtin_object_size (p, 0) == -1) >> +#else >> if (__builtin_object_size (p, 0) != sizeof (buf) - 8) >> +#endif >> abort (); >> } >> >> @@ -25,10 +31,15 @@ test2 (size_t x) >> >> for (i = 0; i < x; ++i) >> p = p + 4; >> +#ifdef __builtin_object_size >> + if (__builtin_object_size (p, 1) == -1) >> +#else >> if (__builtin_object_size (p, 1) != sizeof (buf) - 8) >> +#endif >> abort (); >> } > > I'd say for __bdos it would be better to rewrite the testcase > as dg-do run, perhaps use somewhat smaller buffer (say 16 times smaller; > and dg-additional-sources for a file that actually defines that buffer > and main. Perhaps you can have those > #ifdef __builtin_object_size > if (__builtin_object_size (p, 0) != sizeof (buf) - 8 - 4 * x) > #else > in there, just in the wrapper that #define __builtin_object_size > make it dg-do run and have dg-additional-sources (and > #ifndef N > #define N 0x40000000 > #endif > and use that as size of buf. Got it, I'll do that. >> + gcc_checking_assert (is_gimple_variable (ret) > > This should be TREE_CODE (ret) == SSA_NAME > The reason why is_gimple_variable accepts VAR_DECLs/PARM_DECLs/RESULT_DECLs > is high vs. low gimple, but size_type_node sizes are gimple types and > both objsz passes are run when in ssa form, so it should always be either > a SSA_NAME or INTEGER_CST. OK. > >> + || TREE_CODE (ret) == INTEGER_CST); >> + } >> + >> + return ret; >> } >> >> /* Set size for VARNO corresponding to OSI to VAL. */ >> @@ -176,27 +218,113 @@ object_sizes_initialize (struct object_size_info *osi, unsigned varno, >> object_sizes[object_size_type][varno].wholesize = wholeval; >> } >> >> +/* Return a MODIFY_EXPR for cases where SSA and EXPR have the same type. The >> + TREE_VEC is returned only in case of PHI nodes. */ >> + >> +static tree >> +bundle_sizes (tree ssa, tree expr) >> +{ >> + gcc_checking_assert (TREE_TYPE (ssa) == sizetype); >> + >> + if (!TREE_TYPE (expr)) >> + { >> + gcc_checking_assert (TREE_CODE (expr) == TREE_VEC); > > I think I'd prefer to do it the other way, condition on TREE_CODE (expr) == TREE_VEC > and if needed assert it has NULL TREE_TYPE. OK. > >> + TREE_VEC_ELT (expr, TREE_VEC_LENGTH (expr) - 1) = ssa; >> + return expr; >> + } >> + >> + gcc_checking_assert (types_compatible_p (TREE_TYPE (expr), sizetype)); >> + return size_binop (MODIFY_EXPR, ssa, expr); > > This looks wrong. MODIFY_EXPR isn't a binary expression > (tcc_binary/tcc_comparison), size_binop shouldn't be called on it. > I think you even don't want to fold it, so > return build2 (MODIFY_EXPR, sizetype, ssa, expr); > ? Got it, I'll fix that. > Also, calling a parameter or var ssa is quite unusual, normally > one calls a SSA_NAME either name, or ssa_name etc. OK. >> + gcc_checking_assert (size_initval_p (oldval, object_size_type)); >> + gcc_checking_assert (size_initval_p (old_wholeval, >> + object_size_type)); >> + /* For dynamic object sizes, all object sizes that are not gimple >> + variables will need to be gimplified. */ >> + if (TREE_CODE (wholeval) != INTEGER_CST >> + && !is_gimple_variable (wholeval)) >> + { >> + bitmap_set_bit (osi->reexamine, varno); >> + wholeval = bundle_sizes (make_ssa_name (sizetype), wholeval); >> + } >> + if (TREE_CODE (val) != INTEGER_CST && !is_gimple_variable (val)) > > Again twice above. OK. >> +/* Set temporary SSA names for object size and whole size to resolve dependency >> + loops in dynamic size computation. */ >> + >> +static inline void >> +object_sizes_set_temp (struct object_size_info *osi, unsigned varno) >> +{ >> + tree val = object_sizes_get (osi, varno); >> + >> + if (size_initval_p (val, osi->object_size_type)) >> + object_sizes_set (osi, varno, >> + make_ssa_name (sizetype), >> + make_ssa_name (sizetype)); > > This makes me a little bit worried. Do you compute the wholesize SSA_NAME > at runtime always, or only when it is really needed and known not to always > be equal to the size? > I mean, e.g. for the cases where there is just const char *p = malloc (size); > and the pointer is never increased size == wholesize. For __bos it will > just be 2 different INTEGER_CSTs, but if it would at runtime mean we compute > something twice and hope we eventually find out during later passes > it is the same, it would be bad. I'm emitting both size and wholesize all the time; wholesize only really gets used in size_for_offset and otherwise should get DCE'd. Right now for __bos (and constant sizes) wholesize is unused if it is the same as size. FOR GIMPLE_CALL, GIMPLE_NOP, etc. I return the same tree for size and wholesize; maybe a trivial pointer comparison (sz != wholesize) ought to get rid of most of the uses in size_for_offset. >> + tree phires = TREE_VEC_ELT (wholesize, TREE_VEC_LENGTH (wholesize) - 1); >> + gphi *wholephi = create_phi_node (phires, gimple_bb (stmt)); >> + phires = TREE_VEC_ELT (size, TREE_VEC_LENGTH (size) - 1); >> + gphi *phi = create_phi_node (phires, gimple_bb (stmt)); >> + gphi *obj_phi = as_a (stmt); > > Just one space instead of 2 above. > And the above shows that you actually create 2 PHIs unconditionally, > rather than trying to do that only if 1) wholesize is ever actually > different from size 2) something needs wholesize. Hmm, so I only really need wholesize in ADDR_EXPR and POINTER_PLUS expressions. I suppose I could improve this bit too and reduce wholesize computations. >> + /* If we built an expression, we will need to build statements >> + and insert them on the edge right away. */ >> + if (!is_gimple_variable (wsz)) > > Again, above comments about is_gimple_variable. > >> + wsz = force_gimple_operand (wsz, &seq, true, NULL); >> + if (!is_gimple_variable (sz)) >> + { >> + gimple_seq s; >> + sz = force_gimple_operand (sz, &s, true, NULL); >> + gimple_seq_add_seq (&seq, s); >> + } >> + >> + if (seq) >> + { >> + edge e = gimple_phi_arg_edge (obj_phi, i); >> + >> + /* Put the size definition before the last statement of the source >> + block of the PHI edge. This ensures that any branches at the end >> + of the source block remain the last statement. We are OK even if >> + the last statement is the definition of the object since it will >> + succeed any definitions that contribute to its size and the size >> + expression will succeed them too. */ >> + gimple_stmt_iterator gsi = gsi_last_bb (e->src); >> + gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING); > > This looks problematic. The last stmt in the bb might not exist at all, Wouldn't the bb minimally have to contain the definition of the object whose size we computed? e.g. for PHI [a(2), b(3)], wouldn't bb 2 at least have a statement with the definition of a? Or wait, there could be situations where the definition is in a different block, e.g. bb 1, which has a single edge going on to bb 2? > or can be one that needs to terminate the bb (stmt_ends_bb_p), or can be > some other normal stmt that is just last in the bb, or it can be a debug > stmt. E.g. for -fcompare-debug sanity, inserting before the last stmt > in the block is wrong, because without -g it could be some random stmt and > with -g it can be a debug stmt, so the resulting stmts will then differ > between -g and -g0. Or the last stmt could actually be computing something > you use in the expressions? > I think generally you want gsi_insert_seq_on_edge, just be prepared that it > doesn't always work - one can't insert on EH or ABNORMAL edges. > For EH/ABNORMAL edges not really sure what can be done, punt, force just > __bos behavior for it, or perhaps insert before the last stmt in that case, > but beware that it would need to be SSA_NAME_OCCURS_IN_ABNORMAL_PHI > SSA_NAME which I think needs to have underlying SSA_NAME_VAR and needs to > follow rules such that out of SSA can handle it. I suppose __bos-like behaviour could be a good compromise, i.e. insert a MAX_EXPR (or MIN_EXPR) if we can't find a suitable location to insert on edge. Siddhesh