On 7/24/19 11:08 PM, JiangNing OS wrote:
-----Original Message-----
From: Martin Sebor <mse...@gmail.com>
Sent: Thursday, July 25, 2019 2:08 AM
To: Jeff Law <l...@redhat.com>; JiangNing OS
<jiangn...@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....@amperecomputing.com>
+
+ PR middle-end/91195
+ * tree-ssa-phiopt.c (cond_store_replacement): Work around
+ -Wmaybe-uninitialized warning.
+
2019-07-22 Stafford Horne <sho...@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
