diff mbox series

PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization

Message ID MN2PR01MB5424AEF5B753FFAEB6FEB00E9CC70@MN2PR01MB5424.prod.exchangelabs.com
State New
Headers show
Series PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization | expand

Commit Message

JiangNing OS July 23, 2019, 4:26 a.m. UTC 
This patch is to fix PR91195. Is it OK for trunk?


Thanks,
-Jiangning

Comments

Martin Sebor July 23, 2019, 4:20 p.m. UTC | #1 
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.

Martin
Jeff Law July 24, 2019, 3:25 p.m. UTC | #2 
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.

jeff
Jeff Law July 24, 2019, 3:47 p.m. UTC | #3 
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.
I'll conditionally OK this for the trunk.  Give Richi 48hrs to chime in
if he doesn't like the TREE_NO_WARNING approach.

For anyone watching the thread, the setting of TREE_NO_WARNING here is
only done in those cases where we haven't already seen a dominating
memory reference, so it minimizes how often we set TREE_NO_WARNING.

jeff
Martin Sebor July 24, 2019, 4:09 p.m. UTC | #4 
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
Jeff Law July 24, 2019, 5:12 p.m. UTC | #5 
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
Martin Sebor July 24, 2019, 6:07 p.m. UTC | #6 
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  <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.

I don't know what Jakub had in mind but the mapping I envision is
one like hash_map<tree, bitmap> 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
JiangNing OS July 25, 2019, 5:08 a.m. UTC | #7 
> -----Original Message-----
> From: Martin Sebor <msebor@gmail.com>
> Sent: Thursday, July 25, 2019 2:08 AM
> To: Jeff Law <law@redhat.com>; JiangNing OS
> <jiangning@os.amperecomputing.com>; 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  <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.
> 
> I don't know what Jakub had in mind but the mapping I envision is one like
> hash_map<tree, bitmap> 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.

Thanks,
-Jiangning

> 
> Martin
Martin Sebor July 25, 2019, 6:59 p.m. UTC | #8 
On 7/24/19 11:08 PM, JiangNing OS wrote:
>> -----Original Message-----
>> From: Martin Sebor <msebor@gmail.com>
>> Sent: Thursday, July 25, 2019 2:08 AM
>> To: Jeff Law <law@redhat.com>; JiangNing OS
>> <jiangning@os.amperecomputing.com>; 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  <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.
>>
>> I don't know what Jakub had in mind but the mapping I envision is one like
>> hash_map<tree, bitmap> 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
Jeff Law July 26, 2019, 4:34 a.m. UTC | #9 
On 7/24/19 12:07 PM, Martin Sebor wrote:

> 
> I don't know what Jakub had in mind but the mapping I envision is
> one like hash_map<tree, bitmap> 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.
Ah, yes.  I like that.  I'm still worried about the linkage between the
map and the GC system, but a <tree, bitmap> has a lot of potential.

> 
>>
>> 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.
I suspect shared objects are just going to be painful.  A solution which
worked on EXPR nodes would still be a significant step forward.


> 
> 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.
Yea, but at least they're easy to find via the TREE_NO_WARNING flag.
Hopefully we've got tests for most of the issues we're working around
with that bit.

jeff
Jakub Jelinek July 29, 2019, 3:50 p.m. UTC | #10 
On Tue, Jul 23, 2019 at 04:26:24AM +0000, JiangNing OS wrote:
> --- 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;

I don't like this, but not for the reasons Martin stated, we use
TREE_NO_WARNING not just when we've emitted warnings, but in many places
when we've done something that might trigger false positives.
Yes, it would be nice to do it more selectively.

The problem I see with the above though is that lhs might very well be
a decl, and setting TREE_NO_WARNING on it then doesn't affect only the
hoisted load, but also all other code that refers to the decl.

If the TREE_NO_WARNING bit is set on something that isn't shareable, that is
fine with me, like a MEM_REF, TARGET_MEM_REF or handled component.  If lhs
is a decl, can we force a MEM_REF around it (and won't we fold it back to
the decl?)?  Or perhaps better, can we gimple_set_no_warning on the load
stmt instead?

	Jakub
Jakub Jelinek July 29, 2019, 4:03 p.m. UTC | #11 
On Wed, Jul 24, 2019 at 12:07:36PM -0600, Martin Sebor wrote:
> > > 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<tree, bitmap> 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.

I had in mind something like a hash_map<tree, const_bitmap> / hash_map<gimple *,
const_bitmap> (or just make it a <void *, const_bitmap>) where possibly the
bitmaps would be shared, so have a hash table in which one would look up existing
bitmaps containing particular sets of warnings and if not create a new one (and
where the bitmaps would be const after creation).
The TREE_NO_WARNING or gimple_no_warning bits would be a flag that one
should look up the hash_map if needed, those bits clear would imply empty
bitmap.  Routines that copy trees or gimple stmts, like copy_node or
gimple_copy would take care of adding the new tree/gimple * into the
hash_map if needed too.
And, because we have a lot of warnings that are only emitted in the FEs, or
say before IPA and not afterwards, we could also have spots where we'd
replace the bitmaps with ones that don't have any of the no longer
interesting warning bits.
Say during gimplification we could drop TREE_NO_WARNING bit or not set
gimple_no_warning if the bitmap only contains FE warning bits, or (perhaps
more questionable whether worth it) replace with a const_bitmap that doesn't
have those).

	Jakub
Richard Biener July 30, 2019, 8:21 a.m. UTC | #12 
On Mon, Jul 29, 2019 at 6:03 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Jul 24, 2019 at 12:07:36PM -0600, Martin Sebor wrote:
> > > > 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<tree, bitmap> 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.
>
> I had in mind something like a hash_map<tree, const_bitmap> / hash_map<gimple *,
> const_bitmap> (or just make it a <void *, const_bitmap>) where possibly the
> bitmaps would be shared, so have a hash table in which one would look up existing
> bitmaps containing particular sets of warnings and if not create a new one (and
> where the bitmaps would be const after creation).
> The TREE_NO_WARNING or gimple_no_warning bits would be a flag that one
> should look up the hash_map if needed, those bits clear would imply empty
> bitmap.  Routines that copy trees or gimple stmts, like copy_node or
> gimple_copy would take care of adding the new tree/gimple * into the
> hash_map if needed too.
> And, because we have a lot of warnings that are only emitted in the FEs, or
> say before IPA and not afterwards, we could also have spots where we'd
> replace the bitmaps with ones that don't have any of the no longer
> interesting warning bits.
> Say during gimplification we could drop TREE_NO_WARNING bit or not set
> gimple_no_warning if the bitmap only contains FE warning bits, or (perhaps
> more questionable whether worth it) replace with a const_bitmap that doesn't
> have those).

Would you need to LTO stream & merge the bitmaps / maps somehow?
(maybe "unmap" to sth else when streaming individual stmts)  Not sure if
a bitmap is really necessary, we could simply have a tree/gimple * -> vec<>
mapping with records about emitted (or queued) diagnostics, like
OPT_, message pairs or so.

Richard.

>
>         Jakub
Jakub Jelinek July 30, 2019, 8:34 a.m. UTC | #13 
On Tue, Jul 30, 2019 at 10:21:15AM +0200, Richard Biener wrote:
> Would you need to LTO stream & merge the bitmaps / maps somehow?

Yes.  And if we do not throw unneeded warnings from the sets normally, LTO
streaming might be a good time to do that, so that we merge in only warnings
that will be tested during/post IPA.

> (maybe "unmap" to sth else when streaming individual stmts)  Not sure if
> a bitmap is really necessary, we could simply have a tree/gimple * -> vec<>
> mapping with records about emitted (or queued) diagnostics, like
> OPT_, message pairs or so.

Right now we use it both for the case we've already emitted some diagnostics
and don't want to emit it again later, or for the case where we insert
something in the IL that a warning could be diagnosed on but we know we
don't want that.  The latter is the case e.g. for when we add
TREE_NO_WARNING on the last value in a statement expression so that we don't
diagnose it as unused, or the case we are discussing here.
If we need queued diagnostics, sure, we could add it in, but I don't see a
need for that right now.  vecs compared to bitmap might be larger and harder
to avoid duplicates.  I guess we'd need to do some analysis though, and e.g.
if 99% of cases we need to disable just one warning and not multiple,
perhaps optimize for that case.

	Jakub
Richard Biener July 30, 2019, 8:44 a.m. UTC | #14 
On Tue, Jul 30, 2019 at 10:35 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Jul 30, 2019 at 10:21:15AM +0200, Richard Biener wrote:
> > Would you need to LTO stream & merge the bitmaps / maps somehow?
>
> Yes.  And if we do not throw unneeded warnings from the sets normally, LTO
> streaming might be a good time to do that, so that we merge in only warnings
> that will be tested during/post IPA.
>
> > (maybe "unmap" to sth else when streaming individual stmts)  Not sure if
> > a bitmap is really necessary, we could simply have a tree/gimple * -> vec<>
> > mapping with records about emitted (or queued) diagnostics, like
> > OPT_, message pairs or so.
>
> Right now we use it both for the case we've already emitted some diagnostics
> and don't want to emit it again later, or for the case where we insert
> something in the IL that a warning could be diagnosed on but we know we
> don't want that.  The latter is the case e.g. for when we add
> TREE_NO_WARNING on the last value in a statement expression so that we don't
> diagnose it as unused, or the case we are discussing here.
> If we need queued diagnostics, sure, we could add it in, but I don't see a
> need for that right now.  vecs compared to bitmap might be larger and harder
> to avoid duplicates.  I guess we'd need to do some analysis though, and e.g.
> if 99% of cases we need to disable just one warning and not multiple,
> perhaps optimize for that case.

I was thinking we may want to use the same facility to record warnings we
want to not emit if we later figure a stmt was in an unreachable region.  For
this we need to record the actual warning, not only the warning kind.

Richard.

>
>         Jakub
Martin Sebor July 30, 2019, 2:42 p.m. UTC | #15 
On 7/30/19 2:44 AM, Richard Biener wrote:
> On Tue, Jul 30, 2019 at 10:35 AM Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Tue, Jul 30, 2019 at 10:21:15AM +0200, Richard Biener wrote:
>>> Would you need to LTO stream & merge the bitmaps / maps somehow?
>>
>> Yes.  And if we do not throw unneeded warnings from the sets normally, LTO
>> streaming might be a good time to do that, so that we merge in only warnings
>> that will be tested during/post IPA.
>>
>>> (maybe "unmap" to sth else when streaming individual stmts)  Not sure if
>>> a bitmap is really necessary, we could simply have a tree/gimple * -> vec<>
>>> mapping with records about emitted (or queued) diagnostics, like
>>> OPT_, message pairs or so.
>>
>> Right now we use it both for the case we've already emitted some diagnostics
>> and don't want to emit it again later, or for the case where we insert
>> something in the IL that a warning could be diagnosed on but we know we
>> don't want that.  The latter is the case e.g. for when we add
>> TREE_NO_WARNING on the last value in a statement expression so that we don't
>> diagnose it as unused, or the case we are discussing here.
>> If we need queued diagnostics, sure, we could add it in, but I don't see a
>> need for that right now.  vecs compared to bitmap might be larger and harder
>> to avoid duplicates.  I guess we'd need to do some analysis though, and e.g.
>> if 99% of cases we need to disable just one warning and not multiple,
>> perhaps optimize for that case.
> 
> I was thinking we may want to use the same facility to record warnings we
> want to not emit if we later figure a stmt was in an unreachable region.  For
> this we need to record the actual warning, not only the warning kind.

I was thinking of introducing a __builtin_warning() for this and
issuing it if it's not eliminated just before RTL expansion.  That
would let programs like libstdc++ use it too.

The downside of these solutions is that warnings may be issued out
of order as code moves around.  To have the issued in the order of
increasing source lines they would need to be queued.

Martin
Jeff Law Aug. 7, 2019, 10:03 p.m. UTC | #16 
On 7/30/19 8:42 AM, Martin Sebor wrote:
> On 7/30/19 2:44 AM, Richard Biener wrote:
>> On Tue, Jul 30, 2019 at 10:35 AM Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> On Tue, Jul 30, 2019 at 10:21:15AM +0200, Richard Biener wrote:
>>>> Would you need to LTO stream & merge the bitmaps / maps somehow?
>>>
>>> Yes.  And if we do not throw unneeded warnings from the sets
>>> normally, LTO
>>> streaming might be a good time to do that, so that we merge in only
>>> warnings
>>> that will be tested during/post IPA.
>>>
>>>> (maybe "unmap" to sth else when streaming individual stmts)  Not
>>>> sure if
>>>> a bitmap is really necessary, we could simply have a tree/gimple *
>>>> -> vec<>
>>>> mapping with records about emitted (or queued) diagnostics, like
>>>> OPT_, message pairs or so.
>>>
>>> Right now we use it both for the case we've already emitted some
>>> diagnostics
>>> and don't want to emit it again later, or for the case where we insert
>>> something in the IL that a warning could be diagnosed on but we know we
>>> don't want that.  The latter is the case e.g. for when we add
>>> TREE_NO_WARNING on the last value in a statement expression so that
>>> we don't
>>> diagnose it as unused, or the case we are discussing here.
>>> If we need queued diagnostics, sure, we could add it in, but I don't
>>> see a
>>> need for that right now.  vecs compared to bitmap might be larger and
>>> harder
>>> to avoid duplicates.  I guess we'd need to do some analysis though,
>>> and e.g.
>>> if 99% of cases we need to disable just one warning and not multiple,
>>> perhaps optimize for that case.
>>
>> I was thinking we may want to use the same facility to record warnings we
>> want to not emit if we later figure a stmt was in an unreachable
>> region.  For
>> this we need to record the actual warning, not only the warning kind.
> 
> I was thinking of introducing a __builtin_warning() for this and
> issuing it if it's not eliminated just before RTL expansion.  That
> would let programs like libstdc++ use it too.
I've stated before, I think this is a *great* idea and would encourage
you to prototype it out to see if there's any significant gotchas.
There's several places I think we could use it.

> 
> The downside of these solutions is that warnings may be issued out
> of order as code moves around.  To have the issued in the order of
> increasing source lines they would need to be queued.
I think David is out right now, but I think he'd claim that emitting
diagnostics strictly in line number order isn't best anyway.  Building a
queuing mechanism with an eye towards a more complex comparison to
facilitate priority ordering of diagnostics would be a good thing.  He
may already have some prototype code, not sure on that.

jeff
Jeff Law Sept. 3, 2019, 8:22 p.m. UTC | #17 
On 7/24/19 12:07 PM, Martin Sebor wrote:
> 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  <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.
> 
> I don't know what Jakub had in mind but the mapping I envision is
> one like hash_map<tree, bitmap> 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.
Not a bad idea.  I like the ability to individually control which
warning we're inhibiting.

It'd have to play well with the GC system, but we could make that work.
 WOrst case would be we'd have nodes in the map which inhibit some GC,
but I doubt it'd matter in practice.

jeff
Jeff Law Sept. 3, 2019, 8:27 p.m. UTC | #18 
On 7/29/19 9:50 AM, Jakub Jelinek wrote:
> On Tue, Jul 23, 2019 at 04:26:24AM +0000, JiangNing OS wrote:
>> --- 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;
> 
> I don't like this, but not for the reasons Martin stated, we use
> TREE_NO_WARNING not just when we've emitted warnings, but in many places
> when we've done something that might trigger false positives.
> Yes, it would be nice to do it more selectively.
> 
> The problem I see with the above though is that lhs might very well be
> a decl, and setting TREE_NO_WARNING on it then doesn't affect only the
> hoisted load, but also all other code that refers to the decl.
LHS is restricted to just MEM_REF, ARRAY_REF and COMPONENT_REF.  We'd be
setting the NO_WARNING bits on the toplevel expression, but not on
anything shared like a _DECL node.

So what we're losing here would be things like out of bounds array
checks on the LHS, so it still sucks.


> 
> If the TREE_NO_WARNING bit is set on something that isn't shareable, that is
> fine with me, like a MEM_REF, TARGET_MEM_REF or handled component.  If lhs
> is a decl, can we force a MEM_REF around it (and won't we fold it back to
> the decl?)?  Or perhaps better, can we gimple_set_no_warning on the load
> stmt instead?
We have the toplevel statement, so that might be worth a try as well.

jeff
Jakub Jelinek Nov. 20, 2019, 12:03 a.m. UTC | #19 
On Tue, Sep 03, 2019 at 02:27:29PM -0600, Jeff Law wrote:
> >> +      /* 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;
> > 
> > I don't like this, but not for the reasons Martin stated, we use
> > TREE_NO_WARNING not just when we've emitted warnings, but in many places
> > when we've done something that might trigger false positives.
> > Yes, it would be nice to do it more selectively.
> > 
> > The problem I see with the above though is that lhs might very well be
> > a decl, and setting TREE_NO_WARNING on it then doesn't affect only the
> > hoisted load, but also all other code that refers to the decl.
> LHS is restricted to just MEM_REF, ARRAY_REF and COMPONENT_REF.  We'd be
> setting the NO_WARNING bits on the toplevel expression, but not on
> anything shared like a _DECL node.
> 
> So what we're losing here would be things like out of bounds array
> checks on the LHS, so it still sucks.

Sorry for dropping the ball on this.
You're right, LHS is a MEM_REF, ARRAY_REF or COMPONENT_REF, so what I was
worried about doesn't happen.
I've tried using gimple_set_no_warning, but tree-ssa-uninit.c in this case
actually doesn't look at that (it does only in a different spot).

> > If the TREE_NO_WARNING bit is set on something that isn't shareable, that is
> > fine with me, like a MEM_REF, TARGET_MEM_REF or handled component.  If lhs
> > is a decl, can we force a MEM_REF around it (and won't we fold it back to
> > the decl?)?  Or perhaps better, can we gimple_set_no_warning on the load
> > stmt instead?
> We have the toplevel statement, so that might be worth a try as well.

But, what the patch did was set it on the tree that is later unshared,
which means TREE_NO_WARNING wasn't set just on the rhs1 of the load, but
also on the lhs of the store.

The following version fixes that and I've also added the testcase to the
testsuite.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-11-19  Jiangning Liu  <jiangning.liu@amperecomputing.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/91195
	* tree-ssa-phiopt.c (cond_store_replacement): Move lhs unsharing
	earlier.  Set TREE_NO_WARNING on the rhs1 of the artificially added
	load.

	* gcc.dg/pr91195.c: New test.

--- gcc/tree-ssa-phiopt.c.jj	2019-11-13 10:54:53.882917365 +0100
+++ gcc/tree-ssa-phiopt.c	2019-11-19 20:51:33.345775363 +0100
@@ -2269,6 +2269,10 @@ cond_store_replacement (basic_block midd
   name = make_temp_ssa_name (TREE_TYPE (lhs), NULL, "cstore");
   new_stmt = gimple_build_assign (name, lhs);
   gimple_set_location (new_stmt, locus);
+  lhs = unshare_expr (lhs);
+  /* Set TREE_NO_WARNING on the rhs of the load to avoid uninit
+     warnings.  */
+  TREE_NO_WARNING (gimple_assign_rhs1 (new_stmt)) = 1;
   gsi_insert_on_edge (e1, new_stmt);
 
   /* 3) Create a PHI node at the join block, with one argument
@@ -2279,7 +2283,6 @@ cond_store_replacement (basic_block midd
   add_phi_arg (newphi, rhs, e0, locus);
   add_phi_arg (newphi, name, e1, locus);
 
-  lhs = unshare_expr (lhs);
   new_stmt = gimple_build_assign (lhs, PHI_RESULT (newphi));
 
   /* 4) Insert that PHI node.  */
--- gcc/testsuite/gcc.dg/pr91195.c.jj	2019-11-19 20:57:57.841024685 +0100
+++ gcc/testsuite/gcc.dg/pr91195.c	2019-11-19 20:57:40.767280052 +0100
@@ -0,0 +1,25 @@
+/* PR middle-end/91195 */
+/* { dg-do compile } */
+/* { dg-options "-Wmaybe-uninitialized -O2" } */
+
+int bar (char*);
+
+void
+foo (char *x, char *y)
+{
+  char *a[2];
+  int b = 0;
+
+  if (x)
+    a[b++] = x;		/* { dg-bogus "may be used uninitialized in this function" } */
+  if (y)
+    a[b++] = y;
+
+  for (int j = 0; j < 4; j++) 
+    switch (j)
+      {
+      case 0:
+	if (b == 0 || bar (a[0]))
+	  break;
+      }
+}


	Jakub
Jeff Law Nov. 20, 2019, 2:27 a.m. UTC | #20 
On 11/19/19 5:03 PM, Jakub Jelinek wrote:
> On Tue, Sep 03, 2019 at 02:27:29PM -0600, Jeff Law wrote:
>>>> +      /* 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;
>>>
>>> I don't like this, but not for the reasons Martin stated, we use
>>> TREE_NO_WARNING not just when we've emitted warnings, but in many places
>>> when we've done something that might trigger false positives.
>>> Yes, it would be nice to do it more selectively.
>>>
>>> The problem I see with the above though is that lhs might very well be
>>> a decl, and setting TREE_NO_WARNING on it then doesn't affect only the
>>> hoisted load, but also all other code that refers to the decl.
>> LHS is restricted to just MEM_REF, ARRAY_REF and COMPONENT_REF.  We'd be
>> setting the NO_WARNING bits on the toplevel expression, but not on
>> anything shared like a _DECL node.
>>
>> So what we're losing here would be things like out of bounds array
>> checks on the LHS, so it still sucks.
> 
> Sorry for dropping the ball on this.
> You're right, LHS is a MEM_REF, ARRAY_REF or COMPONENT_REF, so what I was
> worried about doesn't happen.
> I've tried using gimple_set_no_warning, but tree-ssa-uninit.c in this case
> actually doesn't look at that (it does only in a different spot).
> 
>>> If the TREE_NO_WARNING bit is set on something that isn't shareable, that is
>>> fine with me, like a MEM_REF, TARGET_MEM_REF or handled component.  If lhs
>>> is a decl, can we force a MEM_REF around it (and won't we fold it back to
>>> the decl?)?  Or perhaps better, can we gimple_set_no_warning on the load
>>> stmt instead?
>> We have the toplevel statement, so that might be worth a try as well.
> 
> But, what the patch did was set it on the tree that is later unshared,
> which means TREE_NO_WARNING wasn't set just on the rhs1 of the load, but
> also on the lhs of the store.
> 
> The following version fixes that and I've also added the testcase to the
> testsuite.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2019-11-19  Jiangning Liu  <jiangning.liu@amperecomputing.com>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/91195
> 	* tree-ssa-phiopt.c (cond_store_replacement): Move lhs unsharing
> 	earlier.  Set TREE_NO_WARNING on the rhs1 of the artificially added
> 	load.
> 
> 	* gcc.dg/pr91195.c: New test.
OK
jeff
diff mbox series

Patch

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;
     }
 
   /* Now we've checked the constraints, so do the transformation: