From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98041 invoked by alias); 24 Jul 2019 17:12:42 -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 97913 invoked by uid 89); 24 Jul 2019 17:12:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Jul 2019 17:12:41 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A822D307D934; Wed, 24 Jul 2019 17:12:39 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-8.rdu2.redhat.com [10.10.112.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8DABB5C22D; Wed, 24 Jul 2019 17:12:38 +0000 (UTC) Subject: Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization To: Martin Sebor , JiangNing OS , "gcc-patches@gcc.gnu.org" References: <187485ec-9c48-735e-54da-8b0820372fe2@redhat.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: Date: Wed, 24 Jul 2019 17:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-07/txt/msg01595.txt.bz2 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. 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. jeff > > Martin