From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x229.google.com (mail-oi1-x229.google.com [IPv6:2607:f8b0:4864:20::229]) by sourceware.org (Postfix) with ESMTPS id 4E56E3858434 for ; Mon, 6 Dec 2021 17:31:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4E56E3858434 Received: by mail-oi1-x229.google.com with SMTP id bf8so22755396oib.6 for ; Mon, 06 Dec 2021 09:31:36 -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=h9lBNbyx2LFctUXztTUEAyElTmSW0i2dZCM49hlp7oQ=; b=SFVhIesPOwyynAbFrjbeNLUMXpqFcSg2kUvVa+UH4B8eAkReJxo3Vl5OU4g4/oanDu iiJsA69s3NvgbXBInu1OUahl+8tGaRWtmOXx1RoDMxF3g2mLGjCmp0tdJkA+Id8OEB7e 2thkmQVjZGicWg+U2xgLkZdcPxeAb8vJnhGD8AnSjNr9RTIoZuKBSfGFrEFIXeZCAcEU GHseYTB54WsnrQEMxiuC8kUcsqk2A+nUhmrG+Yo55bFZ00aOu6D1Gcm6wxZI8zw5987+ XVR+sYelTDb5R/yE0chk2Z6MXBCdz0FarahHOwpHjht/yYMpzF2arsTPSEJOZuqNv1kI rFVQ== X-Gm-Message-State: AOAM531qVDR2eiScww2jOUXGJrlNrZNSseG+NXkz2lMWIEzmHNMyOk5r zrxcjpL9CyTlLxxs3ehyGuS7zbwSyPQ= X-Google-Smtp-Source: ABdhPJzeyZIP4TXeOqbdGGU3hv7d/gqyix/wi9YYbXKb5uwv7Ak90bE1X85G+pduaYS0dFdVJSd/ZA== X-Received: by 2002:aca:eb02:: with SMTP id j2mr25374693oih.3.1638811894958; Mon, 06 Dec 2021 09:31:34 -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 j5sm2314846ots.68.2021.12.06.09.31.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Dec 2021 09:31:34 -0800 (PST) Subject: [PATCH v2 1/5] fix up compute_objsize: move bndrng into access_data To: Jeff Law , gcc-patches References: <65d1e530-a4cc-de27-1198-0dcaa08274bd@gmail.com> From: Martin Sebor Message-ID: Date: Mon, 6 Dec 2021 10:31:33 -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-Type: multipart/mixed; boundary="------------AB881938C139A8E212532A4D" 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-List-Received-Date: Mon, 06 Dec 2021 17:31:39 -0000 This is a multi-part message in MIME format. --------------AB881938C139A8E212532A4D Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Attached is the subset of the patch in part (1) below: Move bndrng from access_ref to access_data. 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 > --------------AB881938C139A8E212532A4D Content-Type: text/x-patch; charset=UTF-8; name="gcc-pointer_query-refactor-1.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-pointer_query-refactor-1.diff" commit 1062c1154beeeae26e86f053946ea733ffb5d136 Author: Martin Sebor Date: Sat Dec 4 16:46:17 2021 -0700 Move bndrng from access_ref to access_data. gcc/ChangeLog: * gimple-ssa-warn-access.cc (check_access): Adjust to member name change. (pass_waccess::check_strncmp): Same. * pointer-query.cc (access_ref::access_ref): Remove arguments. Simpilfy. (access_data::access_data): Define new ctors. (access_data::set_bound): Define new member function. (compute_objsize_r): Remove unnecessary code. * pointer-query.h (struct access_ref): Remove ctor arguments. (struct access_data): Declare ctor overloads. (access_data::dst_bndrng): New member. (access_data::src_bndrng): New member. diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index 48bf8aaff50..05b8d91b71d 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -1337,10 +1337,10 @@ check_access (GimpleOrTree exp, tree dstwrite, if (!dstsize) dstsize = maxobjsize; - /* Set RANGE to that of DSTWRITE if non-null, bounded by PAD->DST.BNDRNG + /* Set RANGE to that of DSTWRITE if non-null, bounded by PAD->DST_BNDRNG if valid. */ gimple *stmt = pad ? pad->stmt : nullptr; - get_size_range (rvals, dstwrite, stmt, range, pad ? pad->dst.bndrng : NULL); + get_size_range (rvals, dstwrite, stmt, range, pad ? pad->dst_bndrng : NULL); tree func = get_callee_fndecl (exp); /* Read vs write access by built-ins can be determined from the const @@ -1432,9 +1432,9 @@ check_access (GimpleOrTree exp, tree dstwrite, of an object. */ if (maxread) { - /* Set RANGE to that of MAXREAD, bounded by PAD->SRC.BNDRNG if + /* Set RANGE to that of MAXREAD, bounded by PAD->SRC_BNDRNG if PAD is nonnull and BNDRNG is valid. */ - get_size_range (rvals, maxread, stmt, range, pad ? pad->src.bndrng : NULL); + get_size_range (rvals, maxread, stmt, range, pad ? pad->src_bndrng : NULL); location_t loc = get_location (exp); tree size = dstsize; @@ -1479,12 +1479,12 @@ check_access (GimpleOrTree exp, tree dstwrite, && (pad->src.offrng[1] < 0 || pad->src.offrng[0] <= pad->src.offrng[1])) { - /* Set RANGE to that of MAXREAD, bounded by PAD->SRC.BNDRNG if + /* Set RANGE to that of MAXREAD, bounded by PAD->SRC_BNDRNG if PAD is nonnull and BNDRNG is valid. */ - get_size_range (rvals, maxread, stmt, range, pad ? pad->src.bndrng : NULL); + get_size_range (rvals, maxread, stmt, range, pad ? pad->src_bndrng : NULL); /* Set OVERREAD for reads starting just past the end of an object. */ - overread = pad->src.sizrng[1] - pad->src.offrng[0] < pad->src.bndrng[0]; - range[0] = wide_int_to_tree (sizetype, pad->src.bndrng[0]); + overread = pad->src.sizrng[1] - pad->src.offrng[0] < pad->src_bndrng[0]; + range[0] = wide_int_to_tree (sizetype, pad->src_bndrng[0]); slen = size_zero_node; } @@ -2592,7 +2592,7 @@ pass_waccess::check_strncmp (gcall *stmt) /* Determine the range of the bound first and bail if it fails; it's cheaper than computing the size of the objects. */ tree bndrng[2] = { NULL_TREE, NULL_TREE }; - get_size_range (m_ptr_qry.rvals, bound, stmt, bndrng, adata1.src.bndrng); + get_size_range (m_ptr_qry.rvals, bound, stmt, bndrng, adata1.src_bndrng); if (!bndrng[0] || integer_zerop (bndrng[0])) return; diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc index 25ce4303849..a8c9671e3ba 100644 --- a/gcc/pointer-query.cc +++ b/gcc/pointer-query.cc @@ -596,15 +596,9 @@ gimple_parm_array_size (tree ptr, wide_int rng[2], return var; } -/* Given a statement STMT, set the bounds of the reference to at most - as many bytes as BOUND or unknown when null, and at least one when - the MINACCESS is true unless BOUND is a constant zero. STMT is - used for context to get accurate range info. */ - -access_ref::access_ref (range_query *qry /* = nullptr */, - tree bound /* = NULL_TREE */, - gimple *stmt /* = nullptr */, - bool minaccess /* = false */) +/* Initialize the object. */ + +access_ref::access_ref () : ref (), eval ([](tree x){ return x; }), deref (), trail1special (true), base0 (true), parmarray () { @@ -613,21 +607,6 @@ access_ref::access_ref (range_query *qry /* = nullptr */, offmax[0] = offmax[1] = 0; /* Invalidate. */ sizrng[0] = sizrng[1] = -1; - - /* Set the default bounds of the access and adjust below. */ - bndrng[0] = minaccess ? 1 : 0; - bndrng[1] = HOST_WIDE_INT_M1U; - - /* When BOUND is nonnull and a range can be extracted from it, - set the bounds of the access to reflect both it and MINACCESS. - BNDRNG[0] is the size of the minimum access. */ - tree rng[2]; - if (bound && get_size_range (qry, bound, stmt, rng, SR_ALLOW_ZERO)) - { - bndrng[0] = wi::to_offset (rng[0]); - bndrng[1] = wi::to_offset (rng[1]); - bndrng[0] = bndrng[0] > 0 && minaccess ? 1 : 0; - } } /* Return the PHI node REF refers to or null if it doesn't. */ @@ -1199,6 +1178,54 @@ access_ref::inform_access (access_mode mode) const sizestr, allocfn); } +/* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1 + when MINWRITE or MINREAD, respectively, is set. */ +access_data::access_data (range_query *query, gimple *stmt, access_mode mode, + tree maxwrite /* = NULL_TREE */, + bool minwrite /* = false */, + tree maxread /* = NULL_TREE */, + bool minread /* = false */) + : stmt (stmt), call (), dst (), src (), mode (mode) +{ + set_bound (dst_bndrng, maxwrite, minwrite, query, stmt); + set_bound (src_bndrng, maxread, minread, query, stmt); +} + +/* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1 + when MINWRITE or MINREAD, respectively, is set. */ +access_data::access_data (range_query *query, tree expr, access_mode mode, + tree maxwrite /* = NULL_TREE */, + bool minwrite /* = false */, + tree maxread /* = NULL_TREE */, + bool minread /* = false */) + : stmt (), call (expr), dst (), src (), mode (mode) +{ + set_bound (dst_bndrng, maxwrite, minwrite, query, stmt); + set_bound (src_bndrng, maxread, minread, query, stmt); +} + +/* Set BNDRNG to the range of BOUND for the statement STMT. */ + +void +access_data::set_bound (offset_int bndrng[2], tree bound, bool minaccess, + range_query *query, gimple *stmt) +{ + /* Set the default bounds of the access and adjust below. */ + bndrng[0] = minaccess ? 1 : 0; + bndrng[1] = HOST_WIDE_INT_M1U; + + /* When BOUND is nonnull and a range can be extracted from it, + set the bounds of the access to reflect both it and MINACCESS. + BNDRNG[0] is the size of the minimum access. */ + tree rng[2]; + if (bound && get_size_range (query, bound, stmt, rng, SR_ALLOW_ZERO)) + { + bndrng[0] = wi::to_offset (rng[0]); + bndrng[1] = wi::to_offset (rng[1]); + bndrng[0] = bndrng[0] > 0 && minaccess ? 1 : 0; + } +} + /* Set a bit for the PHI in VISITED and return true if it wasn't already set. */ @@ -1948,14 +1975,8 @@ compute_objsize_r (tree ptr, gimple *stmt, int ostype, access_ref *pref, if (const access_ref *cache_ref = qry->get_ref (ptr)) { /* If the pointer is in the cache set *PREF to what it refers - to and return success. - FIXME: BNDRNG is determined by each access and so it doesn't - belong in access_ref. Until the design is changed, keep it - unchanged here. */ - const offset_int bndrng[2] = { pref->bndrng[0], pref->bndrng[1] }; + to and return success. */ *pref = *cache_ref; - pref->bndrng[0] = bndrng[0]; - pref->bndrng[1] = bndrng[1]; return true; } } diff --git a/gcc/pointer-query.h b/gcc/pointer-query.h index fbea3316f14..82cb81b3987 100644 --- a/gcc/pointer-query.h +++ b/gcc/pointer-query.h @@ -61,8 +61,7 @@ class pointer_query; struct access_ref { /* Set the bounds of the reference. */ - access_ref (range_query *query = nullptr, tree = NULL_TREE, - gimple * = nullptr, bool = false); + access_ref (); /* Return the PHI node REF refers to or null if it doesn't. */ gphi *phi () const; @@ -129,11 +128,6 @@ struct access_ref offset_int sizrng[2]; /* The minimum and maximum offset computed. */ offset_int offmax[2]; - /* Range of the bound of the access: denotes that the access - is at least BNDRNG[0] bytes but no more than BNDRNG[1]. - For string functions the size of the actual access is - further constrained by the length of the string. */ - offset_int bndrng[2]; /* Used to fold integer expressions when called from front ends. */ tree (*eval)(tree); @@ -206,23 +200,18 @@ struct access_data { /* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1 when MINWRITE or MINREAD, respectively, is set. */ - access_data (range_query *query, gimple *stmt, access_mode mode, - tree maxwrite = NULL_TREE, bool minwrite = false, - tree maxread = NULL_TREE, bool minread = false) - : stmt (stmt), call (), - dst (query, maxwrite, stmt, minwrite), - src (query, maxread, stmt, minread), - mode (mode) { } + access_data (range_query *, gimple *, access_mode, + tree = NULL_TREE, bool = false, + tree = NULL_TREE, bool = false); /* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1 when MINWRITE or MINREAD, respectively, is set. */ - access_data (range_query *query, tree expr, access_mode mode, - tree maxwrite = NULL_TREE, bool minwrite = false, - tree maxread = NULL_TREE, bool minread = false) - : stmt (), call (expr), - dst (query, maxwrite, nullptr, minwrite), - src (query, maxread, nullptr, minread), - mode (mode) { } + access_data (range_query *, tree, access_mode, + tree = NULL_TREE, bool = false, + tree = NULL_TREE, bool = false); + + /* Constructor helper. */ + static void set_bound (offset_int[2], tree, bool, range_query *, gimple *); /* Access statement. */ gimple *stmt; @@ -230,6 +219,14 @@ struct access_data tree call; /* Destination and source of the access. */ access_ref dst, src; + + /* Range of the bound of the access: denotes that the access is at + least XXX_BNDRNG[0] bytes but no more than XXX_BNDRNG[1]. For + string functions the size of the actual access is further + constrained by the length of the string. */ + offset_int dst_bndrng[2]; + offset_int src_bndrng[2]; + /* Read-only for functions like memcmp or strlen, write-only for memset, read-write for memcpy or strcat. */ access_mode mode; --------------AB881938C139A8E212532A4D--