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

[Jeff Law <law at redhat dot com>]

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  <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.
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