This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization
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 <firstname.lastname@example.org>
>>>> + PR middle-end/91195
>>>> + * tree-ssa-phiopt.c (cond_store_replacement): Work around
>>>> + -Wmaybe-uninitialized warning.
>>>> 2019-07-22 Stafford Horne <email@example.com>
>>>> * config/or1k/or1k.c (or1k_expand_compare): Check for int
>>>> 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
>>>> + 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
>>>> + 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
> 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.