Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization

[Martin Sebor <msebor@gmail.com>]

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  <jiangning.liu@amperecomputing.com>
>>> +
>>> +    PR middle-end/91195
>>> +    * tree-ssa-phiopt.c (cond_store_replacement): Work around
>>> +    -Wmaybe-uninitialized warning.
>>> +
>>>    2019-07-22  Stafford Horne  <shorne@gmail.com>
>>>          * 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 = <someval>
> 
> And turns it into
> 
>    t1 = *p;
>    t2 = x ? <someval> : 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.

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.

Martin