Message ID | b592eea8-9699-e706-196b-36f40d5d53b5@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3824E3858426 for <patchwork@sourceware.org>; Mon, 6 Dec 2021 17:31:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3824E3858426 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1638811915; bh=mGWhyToW8VPs8TAlWjoZB3bX46ZQol26aIEZ3vbBm7c=; 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=BqG4WWHu7yGi3jnZGSH67NynjygHjOXn9S54NuNyOK8iLdZl4UfvOkY3JHNSQY895 hsPfJBgHNCMsA8QR8ijs82ciI3kkmVCEaLAUaUH233pKnFOU+dAqufpAr7nLezf4HL hq5rsDrtFZu9RH9AlqcIqzztawrbKTBeGgIR8tdQ= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-ot1-x331.google.com (mail-ot1-x331.google.com [IPv6:2607:f8b0:4864:20::331]) by sourceware.org (Postfix) with ESMTPS id 390A93858D28 for <gcc-patches@gcc.gnu.org>; Mon, 6 Dec 2021 17:31:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 390A93858D28 Received: by mail-ot1-x331.google.com with SMTP id i5-20020a05683033e500b0057a369ac614so14529525otu.10 for <gcc-patches@gcc.gnu.org>; Mon, 06 Dec 2021 09:31:25 -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=mGWhyToW8VPs8TAlWjoZB3bX46ZQol26aIEZ3vbBm7c=; b=jtEK1gQS2mbRP6J4HelkMDww+Etd+9MGYg0NMSVgKFhDnOG8GOihY6bAkGXNuBs1EJ OFnQwJBMNwCt728cZBSjEQCIEBNE0nZ8dBTtUAqUXn4aHM6QzSP+pgedGkAZ30V+9ctR KC7Fovypn8d/R6BBtNo7zrBGN48lxB8JWWMw8ZabNuaGgcW36+Xa7/VPVKTu3BI1xBvd Z6fUpeorLM//cu0Re816fJB2UhaYE/lCNV6gIWbXaap7uIhrlDTPlyzfY4bFqmsre/tr 3Ci6+F89W0rludvznqfJ1deCbazSQ9lsyBppSP94y3GmbkDvZxwFyj3EhbldR5NwEKU/ Q3Eg== X-Gm-Message-State: AOAM532PmT9Cf5T9OwiIgoDz8XXn4BkMGKGc+X0U3Gn1glLwActrW7ub MzbeKOiLbONglBHxwmsEm8UDaETqDEI= X-Google-Smtp-Source: ABdhPJy219gg1SyJu3YLUng5VshibgKCWbQY/kRhvW7JXx5yJe11kTA+Ujz7PREL84l5WZxsgdgyXQ== X-Received: by 2002:a9d:69ce:: with SMTP id v14mr30537539oto.312.1638811884449; Mon, 06 Dec 2021 09:31:24 -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 k14sm2823923oil.38.2021.12.06.09.31.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Dec 2021 09:31:24 -0800 (PST) Subject: [PATCH v2] fix PR 103143 To: Jeff Law <jeffreyalaw@gmail.com>, gcc-patches <gcc-patches@gcc.gnu.org> References: <65d1e530-a4cc-de27-1198-0dcaa08274bd@gmail.com> <d61e151f-1e97-9a1d-18ae-8a7d2371778f@gmail.com> Message-ID: <b592eea8-9699-e706-196b-36f40d5d53b5@gmail.com> Date: Mon, 6 Dec 2021 10:31:23 -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: <d61e151f-1e97-9a1d-18ae-8a7d2371778f@gmail.com> Content-Type: multipart/mixed; boundary="------------53948E597D80AB81B7A3C06D" Content-Language: en-US X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_NUMSUBJECT, 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Martin Sebor <msebor@gmail.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series | [v2] fix PR 103143 | |
Commit Message
Martin Sebor
Dec. 6, 2021, 5:31 p.m. UTC
I have broken up the patch into a series of six. Attached is part (1), the fix for the typo that causes PR 103143. 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 >
Comments
On 12/6/2021 10:31 AM, Martin Sebor wrote: > I have broken up the patch into a series of six. Attached is > part (1), the fix for the typo that causes PR 103143. > > 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-103413.diff > > commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9 > Author: Martin Sebor<msebor@redhat.com> > Date: Sat Dec 4 16:22:07 2021 -0700 > > Use the recursive form of compute_objsize [PR 103143]. > > gcc/ChangeLog: > > PR middle-end/103143 > * pointer-query.cc (gimple_call_return_array): Call compute_objsize_r. > > gcc/testsuite/ChangeLog: > PR middle-end/103143 > * gcc.dg/Wstringop-overflow-83.c: New test. Thanks. Presumably by going through the public API, qry->depth got reset to 0 and/or we'd get a re-initialized snlim at each call, leading to the infinite recursion? OK for the trunk. Thanks for breaking this out. jeff
On 12/6/21 1:14 PM, Jeff Law wrote: > > > On 12/6/2021 10:31 AM, Martin Sebor wrote: >> I have broken up the patch into a series of six. Attached is >> part (1), the fix for the typo that causes PR 103143. >> >> 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-103413.diff >> >> commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9 >> Author: Martin Sebor<msebor@redhat.com> >> Date: Sat Dec 4 16:22:07 2021 -0700 >> >> Use the recursive form of compute_objsize [PR 103143]. >> >> gcc/ChangeLog: >> >> PR middle-end/103143 >> * pointer-query.cc (gimple_call_return_array): Call compute_objsize_r. >> >> gcc/testsuite/ChangeLog: >> PR middle-end/103143 >> * gcc.dg/Wstringop-overflow-83.c: New test. > Thanks. Presumably by going through the public API, qry->depth got > reset to 0 and/or we'd get a re-initialized snlim at each call, leading > to the infinite recursion? Yes, each call creates a new ssa_name_limit_t::visited bitmap. It might be better to make compute_objsize_r() a member of the same class as the bitmap and also rename it, to make it harder to make this mistake again. Martin > > OK for the trunk. Thanks for breaking this out. > > jeff >
commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9 Author: Martin Sebor <msebor@redhat.com> Date: Sat Dec 4 16:22:07 2021 -0700 Use the recursive form of compute_objsize [PR 103143]. gcc/ChangeLog: PR middle-end/103143 * pointer-query.cc (gimple_call_return_array): Call compute_objsize_r. gcc/testsuite/ChangeLog: PR middle-end/103143 * gcc.dg/Wstringop-overflow-83.c: New test. diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc index 2ead0271617..25ce4303849 100644 --- a/gcc/pointer-query.cc +++ b/gcc/pointer-query.cc @@ -199,7 +199,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end, of the source object. */ access_ref aref; tree src = gimple_call_arg (stmt, 1); - if (compute_objsize (src, stmt, 1, &aref, qry) + if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry) && aref.sizrng[1] < offrng[1]) offrng[1] = aref.sizrng[1]; } diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c new file mode 100644 index 00000000000..6928ee4d559 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c @@ -0,0 +1,19 @@ +/* PR middle-end/103143 - ICE due to infinite recursion in pointer-query.cc + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +typedef __SIZE_TYPE__ size_t; + +void foo (size_t x) +{ + struct T { char buf[64]; char buf2[64]; } t; + char *p = &t.buf[8]; + char *r = t.buf2; + size_t i; + + for (i = 0; i < x; i++) + { + r = __builtin_mempcpy (r, p, i); + p = r + 1; + } +}