From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61959 invoked by alias); 24 Jul 2019 18:07:43 -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 61950 invoked by uid 89); 24 Jul 2019 18:07:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-qk1-f194.google.com Received: from mail-qk1-f194.google.com (HELO mail-qk1-f194.google.com) (209.85.222.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Jul 2019 18:07:40 +0000 Received: by mail-qk1-f194.google.com with SMTP id d15so34420133qkl.4 for ; Wed, 24 Jul 2019 11:07:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=kfohCEDR+rODIgGQM0ZmFXJw+oGbLZ6kYlwwZj1ZSCU=; b=p9I5A8WGjvxf/aSuHHzRletibRVWQ2U8xbnMqHbdLP9TvTYAiFkuK42lR7s9nsiqQT dtM32WEpSTr02CwIefsTE5joq9TeTrndJ3gT8PODTU6E73ljwyN9/pk0ldesdImxzpPr 4vsmUt7KieyxYdwdsc2KY0zL2XHebsxcHKnGdlxeWqppPYIvHmRoo5o7xO/D3oiiKWUR 1imbfiAuVyrbgWmBCqn/5UKHP1VN9dvSmgB7bQB12g6vCjDxCbMX17OmvUf7qBbf+lsq xBve/wNpmBshnk80a3BkKXEFKpttFgfpULP8JxcWt8rkwAQzgh1ceBGmeEqB8JaNrH96 qABw== Return-Path: Received: from [192.168.0.41] (184-96-248-45.hlrn.qwest.net. [184.96.248.45]) by smtp.gmail.com with ESMTPSA id i17sm19804683qkl.71.2019.07.24.11.07.37 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Wed, 24 Jul 2019 11:07:38 -0700 (PDT) Subject: Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization To: Jeff Law , JiangNing OS , "gcc-patches@gcc.gnu.org" References: <187485ec-9c48-735e-54da-8b0820372fe2@redhat.com> From: Martin Sebor Message-ID: <0f07b57e-9def-2758-c58b-ec9200fa4432@gmail.com> Date: Wed, 24 Jul 2019 18:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-07/txt/msg01598.txt.bz2 On 7/24/19 11:12 AM, Jeff Law wrote: > On 7/24/19 10:09 AM, Martin Sebor wrote: >> On 7/24/19 9:25 AM, Jeff Law wrote: >>> On 7/23/19 10:20 AM, Martin Sebor wrote: >>>> On 7/22/19 10:26 PM, JiangNing OS wrote: >>>>> This patch is to fix PR91195. Is it OK for trunk? >>>>> >>>>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog >>>>> index 711a31ea597..4db36644160 100644 >>>>> --- a/gcc/ChangeLog >>>>> +++ b/gcc/ChangeLog >>>>> @@ -1,3 +1,9 @@ >>>>> +2019-07-22  Jiangning Liu  >>>>> + >>>>> +    PR middle-end/91195 >>>>> +    * tree-ssa-phiopt.c (cond_store_replacement): Work around >>>>> +    -Wmaybe-uninitialized warning. >>>>> + >>>>>    2019-07-22  Stafford Horne  >>>>>          * config/or1k/or1k.c (or1k_expand_compare): Check for int >>>>> before >>>>> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c >>>>> index b64bde695f4..7a86007d087 100644 >>>>> --- a/gcc/tree-ssa-phiopt.c >>>>> +++ b/gcc/tree-ssa-phiopt.c >>>>> @@ -2240,6 +2240,14 @@ cond_store_replacement (basic_block middle_bb, >>>>> basic_block join_bb, >>>>>          tree base = get_base_address (lhs); >>>>>          if (!auto_var_p (base) || TREE_ADDRESSABLE (base)) >>>>>        return false; >>>>> + >>>>> +      /* The transformation below will inherently introduce a memory >>>>> load, >>>>> +     for which LHS may not be initialized yet if it is not in NOTRAP, >>>>> +     so a -Wmaybe-uninitialized warning message could be triggered. >>>>> +     Since it's a bit hard to have a very accurate uninitialization >>>>> +     analysis to memory reference, we disable the warning here to >>>>> avoid >>>>> +     confusion.  */ >>>>> +      TREE_NO_WARNING (lhs) = 1; >>>> >>>> The no-warning bit is sometimes (typically?) set by the middle-end >>>> after a warning has been issued, to avoid triggering other warnings >>>> down the line for an already diagnosed expression.  Unless it's >>>> done just for the purposes of a single pass and the bit is cleared >>>> afterwards, using it to avoid potential false positives doesn't >>>> seem like a robust solution.  It will mask warnings for constructs >>>> that have been determined to be invalid. >>>> >>>> FWIW, the middle-end is susceptible to quite a few false positives >>>> that would nice to avoid.  We have discussed various approaches to >>>> the problem but setting the no-warning bit seems like too blunt of >>>> an instrument. >>> All true. >>> >>> But in the case JiangNing is working with the transformation inherently >>> can introduce an uninitialized read.  It seems reasonable to mark those >>> loads he generates that don't have a dominating read. >>> >>> His code takes something like >>> >>>    if (x) >>>      *p = >>> >>> And turns it into >>> >>>    t1 = *p; >>>    t2 = x ? : t1; >>>    *p = t2; >>> >>> In the past we required there be a dominating read from *p (among other >>> restrictions).  That requirement was meant to ensure *p isn't going to >>> fault.  Jiang's work relaxes that requirement somewhat for objects that >>> we can prove aren't going to fault via other means. >>> >>> Can setting TREE_NO_WARNING on the inserted loads inhibit warnings? >>> Certainly.  However, I believe we use it in other places where we know >>> the code we're emitting is safe, but can cause a warning.  I think >>> Jiang's work falls into that category. >>> >>> I do think the bit should only be set if we don't have a dominating load >>> to minimize cases where we might inhibit a valid warning. >> >> I was thinking of a few cases where setting the no-warning bit might >> interfere with detecting bugs unrelated to uninitialized reads: >> >>   1) -Warray-bounds in gimple-ssa-warn-restrict and tree-vrp >>   2) -Wstringop-overflow in tree-ssa-strlen (other than for calls >>      to built-ins) >> >> I couldn't come up with a test case that shows how it might happen >> with this patch but I didn't spend too much time on it. > I bet we could find one and it's more likely to show up on aarch64 than > x86 due to costing issues. Either way we're making a bit of a judgment > call -- an extra false positive here due to a load the compiler inserted > on a path that didn't have one before, or potentially missing a warning > on that load because of the TREE_NO_WARNING. > > I believe the false positive here is worse than the potential missed > warning. > > >> >> There are a number of existing instances of setting TREE_NO_WARNING >> to suppress -Wuninitialized, and some are the cause of known problems. >> Bugs 51545, 58950, 74762, 74765 or 89697 are examples.  They all boil >> down to the fact that there's just one bit for all warnings.  Jakub >> mentioned adding a hash-map for this.  That seems like a simple and >> good solution. > I'm not sure how that really helps here. We marking the MEM on the LHS > and that's not a shared object. I don't see how it's going to be > significantly different using a hash map vs the bit in this circumstance. I don't know what Jakub had in mind but the mapping I envision is one like hash_map that would make it possible to set a bit for each distinct warning for every tree node. It would let us set a bit for -Wuninitialized while leaving the bit for -Warray-bounds (and all other warnings) clear. > > If the bit were on an SSA_NAME, or a _DECL node, then the flag bit is > shared and would be a much larger concern. For shared objects the mapping would have to be more involved but I haven't thought about it in any detail to have an idea what it might look like. Anyway, if/when someone does come up with a solution for this we will have to go through all the places where the no-warning bit is set and replace them with whatever replacement we come up with. One instance more or less won't make a difference. I just wanted to point out that setting the bit is not a robust solution. Martin