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