From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) by sourceware.org (Postfix) with ESMTPS id E72A5385841B for ; Wed, 8 Dec 2021 19:15:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E72A5385841B Received: by mail-pj1-x1033.google.com with SMTP id y14-20020a17090a2b4e00b001a5824f4918so5044764pjc.4 for ; Wed, 08 Dec 2021 11:15:26 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=Fwbsu560lpOAHjF4oAb4X0tVidHAToMANg8DVV0HsEY=; b=HLAPmyX5cZIah/Jh5LbA83BM/iTOHEvYT2P1gCb5pKBkE0tmNZi8XR7a9ed2z+6dyP 7By+SvYHPWw/pTu1OlHrl6SalKFbDE6DhrvZfqgrtGeX++u4LhQxW195vpsEtCHaHZsB b8iUEw+rKzJ1gg+XBs1rPoJuDEHXHJesynstDc4d7ToDBZ46InftXPk51NwsIMOzLiiu I9LdRUzqgbgn3STubJwNMmYtVcNa8/IGAqIuACYuSj5EdvgfLPtvFZLCJ36fwJZPwp36 APBLffGA+NxxrBuLEWzJ3QIrXX6obB3CSQHKEVkyHsx/fBQBia2E2vm87hGj0c7cmpj5 pwOQ== X-Gm-Message-State: AOAM530Sr2AsV/4sQSDsNvdHbasDtncT/P2StD1gcZs5il09X1L8Gzye fuuRMxh2h/BpMR/bIYNLfNA= X-Google-Smtp-Source: ABdhPJytJR23LUuDQOTPfxrP7/xS8BRfSz1fE7bBWVemhQ898X/oPTcsR+UhGzoOZrcqw+bj/RFfnQ== X-Received: by 2002:a17:902:e0d4:b0:142:8897:94e2 with SMTP id e20-20020a170902e0d400b00142889794e2mr61498589pla.58.1638990925942; Wed, 08 Dec 2021 11:15:25 -0800 (PST) Received: from [192.168.1.15] (65-130-80-28.slkc.qwest.net. [65.130.80.28]) by smtp.gmail.com with ESMTPSA id l6sm4756891pfc.30.2021.12.08.11.15.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 08 Dec 2021 11:15:25 -0800 (PST) Message-ID: <75d58067-795e-e539-7247-4a737d96aefe@gmail.com> Date: Wed, 8 Dec 2021 12:15:24 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.3.2 Subject: Re: [PATCH v2 5/5] fix up compute_objsize: add a dump function Content-Language: en-US To: Martin Sebor , gcc-patches References: <65d1e530-a4cc-de27-1198-0dcaa08274bd@gmail.com> From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, NICE_REPLY_A, 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: Wed, 08 Dec 2021 19:15:28 -0000 On 12/6/2021 10:32 AM, Martin Sebor wrote: > Attached is the subset of the patch in part (5) below: Add > a new dump function.  It applies on top of patch 4/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 >> > > > gcc-pointer_query-refactor-5.diff > > commit 2054b01fb383560b96d51fabfe9dee6dbd611f4a > Author: Martin Sebor > Date: Mon Dec 6 09:52:32 2021 -0700 > > Add a new dump function. > > gcc/ChangeLog: > > * pointer-query.cc (access_ref::dump): Define new function > (pointer_query::dump): Call it. > * pointer-query.h (access_ref::dump): Declare new function. OK.  I think it's worth also noting in the ChangeLog the additional dumping you added with the "pointer_query cache contents (again)" hunk. Jeff