From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14593 invoked by alias); 8 Feb 2018 20:45:37 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 14032 invoked by uid 89); 8 Feb 2018 20:45:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=one's X-HELO: mail-oi0-f49.google.com Received: from mail-oi0-f49.google.com (HELO mail-oi0-f49.google.com) (209.85.218.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 08 Feb 2018 20:45:23 +0000 Received: by mail-oi0-f49.google.com with SMTP id b3so4479802oib.11 for ; Thu, 08 Feb 2018 12:45:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=wU58aL/V9iPVVLCmgqXG5sxltTKWnCG7eEWu0IfNDik=; b=TiI+v5DZeDyalm+Qo0UhbbYD7VfiNWzhtLMg/qGUrTeleDDdGZu5OId4aruYTxEmyC /7YrO0fvABSyqDTf/hJqXzwCmDUMQkzNPMCguUS1CZQdF1a9xgiZDl0xYm/8w9FvMGMs OYWEG1wl7o1WG2C3Ine9iQAYRa9nCZ8EuoDWwqlX5Crwb2xhEzNhW8ogJ8RNS2e9MFRd hAmRGbHc5g1WqizYV3KBYtVH78E9DQeSbNisYrb9RCOiDq+MUnh7Ma3GVuGH6CZ98o/j cnlbVzSnYoqvrJBwfwClJZqjcCCiFZ1/mShBMRw9GIBg3zaDPPjzqjW4n7ucmINifXoW P6EA== X-Gm-Message-State: APf1xPA8SMoaj5jswQU8cwTb+M2MV5SGdiZWAf5q7Hh4gLeXAULCx8W/ Q9X9BA3ZYGwQe3swJkofByj4V+0u X-Google-Smtp-Source: AH8x225KhACghKK4lSP10iker5l7eM0BeHT/IuImHmgIzgdjcYUKzyOUb/JDkbpVPrYfsna4nkGhKQ== X-Received: by 10.202.214.84 with SMTP id n81mr251204oig.294.1518122710832; Thu, 08 Feb 2018 12:45:10 -0800 (PST) Received: from localhost.localdomain (75-171-255-194.hlrn.qwest.net. [75.171.255.194]) by smtp.gmail.com with ESMTPSA id 38sm390687ota.55.2018.02.08.12.45.08 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 08 Feb 2018 12:45:09 -0800 (PST) Subject: Re: [PR tree-optimization/84047] missing -Warray-bounds on an out-of-bounds index To: Richard Biener , Aldy Hernandez References: <9a52975c-c720-f42f-3564-7b69e361487d@redhat.com> Cc: Jakub Jelinek , Martin Sebor , gcc-patches From: Martin Sebor Message-ID: Date: Thu, 08 Feb 2018 20:45:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00466.txt.bz2 On 02/08/2018 03:38 AM, Richard Biener wrote: > On Thu, Feb 1, 2018 at 6:42 PM, Aldy Hernandez wrote: >> Since my patch isn't the easy one liner I wanted it to be, perhaps we >> should concentrate on Martin's patch, which is more robust, and has >> testcases to boot! His patch from last week also fixes a couple other >> PRs. >> >> Richard, would this be acceptable? That is, could you or Jakub review >> Martin's all-encompassing patch? If so, I'll drop mine. > > Sorry, no - this one looks way too complicated. Presumably the complication is in he loop that follows SSA_NAMEs and the offsets in: const char *s0 = "12"; const char *s1 = s0 + 1; const char *s2 = s1 + 1; const char *s3 = s2 + 1; int i = *s0 + *s1 + *s2 + *s3; ? I don't know if this used to be diagnosed and is also part of the regression. If it isn't it could be removed for GCC 8 and then added for GCC 9. If this isn't your concern can you be more specific about what is? I should note (again) that this patch doesn't fix the whole regression. There are remaining cases (involving arrays) that used to be handled but no longer are. It's tedious (and hacky) to limit the fix to just the subset of the regression while at the same time preserving the pre-existing limitations (or bugs, depending on one's point of view). >> Also, could someone pontificate on whether we want to fix >> -Warray-bounds regressions for this release cycle? > > Remove bogus ones? Yes. Add "missing ones"? No. Can you please explain how to interpret the Target Milestone then? Why is it set it to 6.5 when the bug is not meant to be fixed? If it's meant to be fixed in 6.5 (and presumably also 7.4) but not in 8.1, do we expect to fix it in 8.2? More to the point, how can we tell which it is? Thanks Martin > > Richard. > >> Thanks. >> >> On Wed, Jan 31, 2018 at 6:05 AM, Richard Biener >> wrote: >>> On Tue, Jan 30, 2018 at 11:11 PM, Aldy Hernandez wrote: >>>> Hi! >>>> >>>> [Note: Jakub has mentioned that missing -Warray-bounds regressions should be >>>> punted to GCC 9. I think this particular one is easy pickings, but if this >>>> and/or the rest of the -Warray-bounds regressions should be marked as GCC 9 >>>> material, please let me know so we can adjust all relevant PRs.] >>>> >>>> This is a -Warray-bounds regression that happens because the IL now has an >>>> MEM_REF instead on ARRAY_REF. >>>> >>>> Previously we had an ARRAY_REF we could diagnose: >>>> >>>> D.2720_5 = "12345678"[1073741824]; >>>> >>>> But now this is represented as: >>>> >>>> _1 = MEM[(const char *)"12345678" + 1073741824B]; >>>> >>>> I think we can just allow check_array_bounds() to handle MEM_REF's and >>>> everything should just work. >>>> >>>> The attached patch fixes both regressions mentioned in the PR. >>>> >>>> Tested on x86-64 Linux. >>>> >>>> OK? >>> >>> This doesn't look correct. You lump MEM_REF handling together with >>> ADDR_EXPR handling but for the above case you want to diagnose >>> _dereferences_ not address-taking. >>> >>> For the dereference case you need to amend the ARRAY_REF case, for example >>> via >>> >>> Index: gcc/tree-vrp.c >>> =================================================================== >>> --- gcc/tree-vrp.c (revision 257181) >>> +++ gcc/tree-vrp.c (working copy) >>> @@ -5012,6 +5012,13 @@ check_array_bounds (tree *tp, int *walk_ >>> if (TREE_CODE (t) == ARRAY_REF) >>> vrp_prop->check_array_ref (location, t, false /*ignore_off_by_one*/); >>> >>> + else if (TREE_CODE (t) == MEM_REF >>> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR >>> + && TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) == STRING_CST) >>> + { >>> + call factored part of check_array_ref passing in STRING_CST and offset >>> + } >>> + >>> else if (TREE_CODE (t) == ADDR_EXPR) >>> { >>> vrp_prop->search_for_addr_array (t, location); >>> >>> note your patch will fail to warn for "1"[1] because taking that >>> address is valid but not >>> dereferencing it. >>> >>> Richard.