From patchwork Mon Dec 6 17:32:19 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 1564059 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=TCqRusWI; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4J79YQ1x8Dz9s0r for ; Tue, 7 Dec 2021 04:35:06 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 178AC3858D28 for ; Mon, 6 Dec 2021 17:35:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 178AC3858D28 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1638812104; bh=G4Ti/TmNGIbQ4m6VsxapX3vxRc8XbdPg62o68fc/qww=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=TCqRusWIHrTYAeipdsbs17Q1BKnT07U7WlfqDvqZ6Wgldz8cWUeJftA3l404I//yf 5aJ0OoSILJ5OLwrX4SYCs2p4jcF5pSKNQewgPGtO3UjbZZPs/sGhv3jh6+crCgDzZm 4jy/uFyKF34Fsxw6Wj1M3K3gxWJbxELMw5N5txGw= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-oi1-x236.google.com (mail-oi1-x236.google.com [IPv6:2607:f8b0:4864:20::236]) by sourceware.org (Postfix) with ESMTPS id DCB1D3858434 for ; Mon, 6 Dec 2021 17:32:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DCB1D3858434 Received: by mail-oi1-x236.google.com with SMTP id t23so22802999oiw.3 for ; Mon, 06 Dec 2021 09:32:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=G4Ti/TmNGIbQ4m6VsxapX3vxRc8XbdPg62o68fc/qww=; b=1k9n/ZhWEDqPvpMBTKI97ImVO1rqQH9lQnKH38YsLGZ6ApRvDrT4zTDJNQs52UhO5h XcimIwmXWNOC3+I6k8t/1ApnGFUkdv0pFTLbhaof0PcOmnwudgoKHlu9393bxWz3rvUc 4fHLj97odVxvtF+mmBpcjAxuY/w5AOy88AHefVTUslpbxXSTg0/QHrgonMhrrLvndIJA 0R0N4IELZf4wbOmQcg6xQMStxIXe9BP02c2FjYFlXbTLSylxQzO2mr3400/IRrvnEsnA iyyzmhmoQtN2ciBvFGvZZ7mZrIGbgUwfeaVEapgJG5Y+scH2wxeBuBxjapdjFrpgwm5Q ivJg== X-Gm-Message-State: AOAM530le1tXYSOcuLtkNGC9M0qD2qTDowgZgUwHhO4Wdhm6BAB/mpfN IpJAVhU7ZJ6fzJgzExCoSlfFDIK6e3E= X-Google-Smtp-Source: ABdhPJx7W0QzoLTjQiW+PNOuluxgWyzfyu3XIiY1Y/bI51s42u15CUcXnMZj4IanfE2xizDKvangzA== X-Received: by 2002:aca:1811:: with SMTP id h17mr24788140oih.178.1638811941270; Mon, 06 Dec 2021 09:32:21 -0800 (PST) Received: from [192.168.0.41] (184-96-227-137.hlrn.qwest.net. [184.96.227.137]) by smtp.gmail.com with ESMTPSA id h6sm2280758otb.60.2021.12.06.09.32.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Dec 2021 09:32:20 -0800 (PST) Subject: [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling To: Jeff Law , gcc-patches References: <65d1e530-a4cc-de27-1198-0dcaa08274bd@gmail.com> Message-ID: Date: Mon, 6 Dec 2021 10:32:19 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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-Patchwork-Original-From: Martin Sebor via Gcc-patches From: Martin Sebor Reply-To: Martin Sebor Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" Attached is subset of the patch in part (4) below: factor out PHI handling. It applies on top of patch 3/5. On 12/3/21 5:00 PM, Jeff Law wrote: > > > On 11/8/2021 7:34 PM, Martin Sebor via Gcc-patches wrote: >> The pointer-query code that implements compute_objsize() that's >> in turn used by most middle end access warnings now has a few >> warts in it and (at least) one bug.  With the exception of >> the bug the warts aren't behind any user-visible bugs that >> I know of but they do cause problems in new code I've been >> implementing on top of it.  Besides fixing the one bug (just >> a typo) the attached patch cleans up these latent issues: >> >> 1) It moves the bndrng member from the access_ref class to >>    access_data.  As a FIXME in the code notes, the member never >>    did belong in the former and only takes up space in the cache. >> >> 2) The compute_objsize_r() function is big, unwieldy, and tedious >>    to step through because of all the if statements that are better >>    coded as one switch statement.  This change factors out more >>    of its code into smaller handler functions as has been suggested >>    and done a few times before. >> >> 3) (2) exposed a few places where I fail to pass the current >>    GIMPLE statement down to ranger.  This leads to worse quality >>    range info, including possible false positives and negatives. >>    I just spotted these problems in code review but I haven't >>    taken the time to come up with test cases.  This change fixes >>    these oversights as well. >> >> 4) The handling of PHI statements is also in one big, hard-to- >>    follow function.  This change moves the handling of each PHI >>    argument into its own handler which merges it into the previous >>    argument.  This makes the code easier to work with and opens it >>    to reuse also for MIN_EXPR and MAX_EXPR.  (This is primarily >>    used to print informational notes after warnings.) >> >> 5) Finally, the patch factors code to dump each access_ref >>    cached by the pointer_query cache out of pointer_query::dump >>    and into access_ref::dump.  This helps with debugging. >> >> These changes should have no user-visible effect and other than >> a regression test for the typo (PR 103143) come with no tests. >> They've been tested on x86_64-linux. > Sigh.  You've identified 6 distinct changes above.  The 5 you've > enumerated plus a typo fix somewhere.  There's no reason why they need > to be a single patch and many reasons why they should be a series of > independent patches.    Combining them into a single patch isn't how we > do things and it hides the actual bugfix in here. > > Please send a fix for the typo first since that should be able to > trivially go forward.  Then  a patch for item #1.  That should be > trivial to review when it's pulled out from teh rest of the patch. > Beyond that, your choice on ordering, but you need to break this down. > > > > > Jeff > commit 6ac1d37947ad5cf07fe133faaf8414f00e0eed13 Author: Martin Sebor Date: Mon Dec 6 09:23:22 2021 -0700 Introduce access_ref::merge_ref. gcc/ChangeLog: * pointer-query.cc (access_ref::merge_ref): Define new function. (access_ref::get_ref): Move code into merge_ref and call it. * pointer-query.h (access_ref::merge_ref): Declare new function. diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc index c75c4da6b60..24fbac84ec4 100644 --- a/gcc/pointer-query.cc +++ b/gcc/pointer-query.cc @@ -624,6 +624,97 @@ access_ref::phi () const return as_a (def_stmt); } +/* Determine the size and offset for ARG, append it to ALL_REFS, and + merge the result with *THIS. Ignore ARG if SKIP_NULL is set and + ARG refers to the null pointer. Return true on success and false + on failure. */ + +bool +access_ref::merge_ref (vec *all_refs, tree arg, gimple *stmt, + int ostype, bool skip_null, + ssa_name_limit_t &snlim, pointer_query &qry) +{ + access_ref aref; + if (!compute_objsize_r (arg, stmt, ostype, &aref, snlim, &qry) + || aref.sizrng[0] < 0) + /* This may be a PHI with all null pointer arguments. */ + return false; + + if (all_refs) + { + access_ref dummy_ref; + aref.get_ref (all_refs, &dummy_ref, ostype, &snlim, &qry); + } + + if (TREE_CODE (arg) == SSA_NAME) + qry.put_ref (arg, aref, ostype); + + if (all_refs) + all_refs->safe_push (aref); + + aref.deref += deref; + + bool merged_parmarray = aref.parmarray; + + const bool nullp = skip_null && integer_zerop (arg); + const offset_int maxobjsize = wi::to_offset (max_object_size ()); + offset_int minsize = sizrng[0]; + + if (sizrng[0] < 0) + { + /* If *THIS doesn't contain a meaningful result yet set it to AREF + unless the argument is null and it's okay to ignore it. */ + if (!nullp) + *this = aref; + + /* Set if the current argument refers to one or more objects of + known size (or range of sizes), as opposed to referring to + one or more unknown object(s). */ + const bool arg_known_size = (aref.sizrng[0] != 0 + || aref.sizrng[1] != maxobjsize); + if (arg_known_size) + sizrng[0] = aref.sizrng[0]; + + return true; + } + + /* Disregard null pointers in PHIs with two or more arguments. + TODO: Handle this better! */ + if (nullp) + return true; + + const bool known_size = (sizrng[0] != 0 || sizrng[1] != maxobjsize); + + if (known_size && aref.sizrng[0] < minsize) + minsize = aref.sizrng[0]; + + /* Determine the amount of remaining space in the argument. */ + offset_int argrem[2]; + argrem[1] = aref.size_remaining (argrem); + + /* Determine the amount of remaining space computed so far and + if the remaining space in the argument is more use it instead. */ + offset_int merged_rem[2]; + merged_rem[1] = size_remaining (merged_rem); + + /* Reset the PHI's BASE0 flag if any of the nonnull arguments + refers to an object at an unknown offset. */ + if (!aref.base0) + base0 = false; + + if (merged_rem[1] < argrem[1] + || (merged_rem[1] == argrem[1] + && sizrng[1] < aref.sizrng[1])) + /* Use the argument with the most space remaining as the result, + or the larger one if the space is equal. */ + *this = aref; + + sizrng[0] = minsize; + parmarray = merged_parmarray; + + return true; +} + /* Determine and return the largest object to which *THIS refers. If *THIS refers to a PHI and PREF is nonnull, fill *PREF with the details of the object determined by compute_objsize(ARG, OSTYPE) for each PHI @@ -636,9 +727,8 @@ access_ref::get_ref (vec *all_refs, ssa_name_limit_t *psnlim /* = NULL */, pointer_query *qry /* = NULL */) const { - gphi *phi_stmt = this->phi (); - if (!phi_stmt) - return ref; + if (!ref || TREE_CODE (ref) != SSA_NAME) + return NULL; /* FIXME: Calling get_ref() with a null PSNLIM is dangerous and might cause unbounded recursion. */ @@ -646,13 +736,49 @@ access_ref::get_ref (vec *all_refs, if (!psnlim) psnlim = &snlim_buf; - if (!psnlim->visit_phi (ref)) - return NULL_TREE; - pointer_query empty_qry; if (!qry) qry = &empty_qry; + if (gimple *def_stmt = SSA_NAME_DEF_STMT (ref)) + { + if (is_gimple_assign (def_stmt)) + { + tree_code code = gimple_assign_rhs_code (def_stmt); + if (code != MIN_EXPR && code != MAX_EXPR) + return NULL_TREE; + + access_ref aref; + tree arg1 = gimple_assign_rhs1 (def_stmt); + if (!aref.merge_ref (all_refs, arg1, def_stmt, ostype, false, + *psnlim, *qry)) + return NULL_TREE; + + tree arg2 = gimple_assign_rhs2 (def_stmt); + if (!aref.merge_ref (all_refs, arg2, def_stmt, ostype, false, + *psnlim, *qry)) + return NULL_TREE; + + if (pref && pref != this) + { + tree ref = pref->ref; + *pref = aref; + pref->ref = ref; + } + + return aref.ref; + } + } + else + return NULL_TREE; + + gphi *phi_stmt = this->phi (); + if (!phi_stmt) + return ref; + + if (!psnlim->visit_phi (ref)) + return NULL_TREE; + /* The conservative result of the PHI reflecting the offset and size of the largest PHI argument, regardless of whether or not they all refer to the same object. */ @@ -670,91 +796,17 @@ access_ref::get_ref (vec *all_refs, phi_ref = *pref; } - /* Set if any argument is a function array (or VLA) parameter not - declared [static]. */ - bool parmarray = false; - /* The size of the smallest object referenced by the PHI arguments. */ - offset_int minsize = 0; - const offset_int maxobjsize = wi::to_offset (max_object_size ()); - const unsigned nargs = gimple_phi_num_args (phi_stmt); for (unsigned i = 0; i < nargs; ++i) { access_ref phi_arg_ref; + bool skip_null = i || i + 1 < nargs; tree arg = gimple_phi_arg_def (phi_stmt, i); - if (!compute_objsize_r (arg, phi_stmt, ostype, &phi_arg_ref, *psnlim, - qry) - || phi_arg_ref.sizrng[0] < 0) - /* A PHI with all null pointer arguments. */ + if (!phi_ref.merge_ref (all_refs, arg, phi_stmt, ostype, skip_null, + *psnlim, *qry)) return NULL_TREE; - - if (TREE_CODE (arg) == SSA_NAME) - qry->put_ref (arg, phi_arg_ref); - - if (all_refs) - all_refs->safe_push (phi_arg_ref); - - parmarray |= phi_arg_ref.parmarray; - - const bool nullp = integer_zerop (arg) && (i || i + 1 < nargs); - - if (phi_ref.sizrng[0] < 0) - { - /* If PHI_REF doesn't contain a meaningful result yet set it - to the result for the first argument. */ - if (!nullp) - phi_ref = phi_arg_ref; - - /* Set if the current argument refers to one or more objects of - known size (or range of sizes), as opposed to referring to - one or more unknown object(s). */ - const bool arg_known_size = (phi_arg_ref.sizrng[0] != 0 - || phi_arg_ref.sizrng[1] != maxobjsize); - if (arg_known_size) - minsize = phi_arg_ref.sizrng[0]; - - continue; - } - - const bool phi_known_size = (phi_ref.sizrng[0] != 0 - || phi_ref.sizrng[1] != maxobjsize); - - if (phi_known_size && phi_arg_ref.sizrng[0] < minsize) - minsize = phi_arg_ref.sizrng[0]; - - /* Disregard null pointers in PHIs with two or more arguments. - TODO: Handle this better! */ - if (nullp) - continue; - - /* Determine the amount of remaining space in the argument. */ - offset_int argrem[2]; - argrem[1] = phi_arg_ref.size_remaining (argrem); - - /* Determine the amount of remaining space computed so far and - if the remaining space in the argument is more use it instead. */ - offset_int phirem[2]; - phirem[1] = phi_ref.size_remaining (phirem); - - /* Reset the PHI's BASE0 flag if any of the nonnull arguments - refers to an object at an unknown offset. */ - if (!phi_arg_ref.base0) - phi_ref.base0 = false; - - if (phirem[1] < argrem[1] - || (phirem[1] == argrem[1] - && phi_ref.sizrng[1] < phi_arg_ref.sizrng[1])) - /* Use the argument with the most space remaining as the result, - or the larger one if the space is equal. */ - phi_ref = phi_arg_ref; } - /* Replace the lower bound of the largest argument with the size - of the smallest argument, and set PARMARRAY if any argument - was one. */ - phi_ref.sizrng[0] = minsize; - phi_ref.parmarray = parmarray; - if (phi_ref.sizrng[0] < 0) { /* Fail if none of the PHI's arguments resulted in updating PHI_REF @@ -766,7 +818,14 @@ access_ref::get_ref (vec *all_refs, /* Avoid changing *THIS. */ if (pref && pref != this) - *pref = phi_ref; + { + /* Keep the SSA_NAME of the PHI unchanged so that all PHI arguments + can be referred to later if necessary. This is useful even if + they all refer to the same object. */ + tree ref = pref->ref; + *pref = phi_ref; + pref->ref = ref; + } psnlim->leave_phi (ref); diff --git a/gcc/pointer-query.h b/gcc/pointer-query.h index cbc87c86ed3..fe46f711e96 100644 --- a/gcc/pointer-query.h +++ b/gcc/pointer-query.h @@ -66,6 +66,10 @@ struct access_ref /* Return the PHI node REF refers to or null if it doesn't. */ gphi *phi () const; + /* Merge the result for a pointer with *THIS. */ + bool merge_ref (vec *all_refs, tree, gimple *, int, bool, + ssa_name_limit_t &, pointer_query &); + /* Return the object to which REF refers. */ tree get_ref (vec *, access_ref * = nullptr, int = 1, ssa_name_limit_t * = nullptr, pointer_query * = nullptr) const;