From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89677 invoked by alias); 25 Jul 2019 18:59: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 89666 invoked by uid 89); 25 Jul 2019 18:59:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.1 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-qt1-f193.google.com Received: from mail-qt1-f193.google.com (HELO mail-qt1-f193.google.com) (209.85.160.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 Jul 2019 18:59:41 +0000 Received: by mail-qt1-f193.google.com with SMTP id y26so50143359qto.4 for ; Thu, 25 Jul 2019 11:59:41 -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=OI3qhy9z91luqBXVZ5RGqOop+B8+JIPiPAk4CxVPuHs=; b=fNzUExKjy7yn4NnpNUD227yMgqwMhiNoS2Dtw5iWD/Shz0jyGgRWFJpBJyoB/QcY+p LuSEk2Ip8wZPZDNGHUhdwLWZFoxZ6V8bwpYwDqtpI6JfBei5gMbzn6/yKXZ9ehpH4vLk y/pZdNxWrLVRZQgS6Q3RVWKAIj0gs5gp4KrkOvE5MHeY0baTWNrf4xv6QKzcyXvrjLUG xkS53UrNu5f3/2kYYHlj47EpJ78BTY0CnlnoYmG5XwqI0qs4UR3OkzHkhI1ZIWojIaaM L2zlC6EM67KLiJJSnl+iPP6kYEDJgk2ieqG0mHlANBMOQg/fapz7guXOTZKJXHhK3iIa yUuw== 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 o5sm22273298qkf.10.2019.07.25.11.59.37 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Thu, 25 Jul 2019 11:59:38 -0700 (PDT) Subject: Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization To: JiangNing OS , Jeff Law , "gcc-patches@gcc.gnu.org" , "jakub@redhat.com" References: <187485ec-9c48-735e-54da-8b0820372fe2@redhat.com> <0f07b57e-9def-2758-c58b-ec9200fa4432@gmail.com> From: Martin Sebor Message-ID: <2cd13860-94c0-3797-2f9f-326cf85b6c66@gmail.com> Date: Thu, 25 Jul 2019 19: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/msg01656.txt.bz2 On 7/24/19 11:08 PM, JiangNing OS wrote: >> -----Original Message----- >> From: Martin Sebor >> Sent: Thursday, July 25, 2019 2:08 AM >> To: Jeff Law ; JiangNing OS >> ; gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for >> conditional store optimization >> >> 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. > > Hi Martin, > > I see "TREE_NO_WARNING (repl) = 1;" is still in generate_subtree_copies, and it > seems PR89697 is unresolved, so we don't have the new hash_map mechanism to > make difference for -Wunintialized and all others yet, correct? It sounds we should > avoid using "TREE_NO_WARNING (xxx) = 1" if only xxx is not a newly build expr, but I > can see there are still a lot of code in trunk using it this way. > > Would it be OK to fix the issue this way first and then handle all cases together later? > After all, as Jeff pointed out false positive of raising uninitialization warning > is worse than missing the warning. The bugzilla examples you give are all for missing > warnings. It's okay with me. I don't share the view that false positives are necessarily worse than false negatives but I also don't have any say in approving patches so my input is only informative (and in this instance was meant to be). I appreciate your asking tough. Martin