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