| Message ID | 20160807034147.24890-1-andi@firstfloor.org |
|---|---|
| State | New |
| Headers | show |
On 08/06/2016 09:41 PM, Andi Kleen wrote: > From: Andi Kleen <ak@linux.intel.com> > > This patch fixes an bootstrap error with autoprofiledbootstrap > due to uninitiliazed variables, because the compiler cannot > figure out they don't need to be initialized in an error path. > Just always initialize them. > > gcc/: > > 2016-08-06 Andi Kleen <ak@linux.intel.com> > > * tree-vrp.c (get_single_symbol): Always initialize inv and neg. The patch is likely OK, but it'd be useful to know more about precisely the use that is causing problems. Ideally we'd have a test that we could more deeply analyze for paths through the CFG that can't be executed. Finding those paths usually both fixes the warning *and* results in better code. Even if we can't fix it now, we can file it away for future work jeff
> Ideally we'd have a test that we could more deeply analyze for paths > through the CFG that can't be executed. Finding those paths usually > both fixes the warning *and* results in better code. Even if we > can't fix it now, we can file it away for future work It's multiple variables who are depending on each other, with complex control flow inbetween. It's not surprising that it loses control of all the combinations. You can take a look yourself here: /home/andi/gcc/git/gcc/gcc/tree-vrp.c: In function 'int compare_values_warnv(tree, tree, bool*)': /home/andi/gcc/git/gcc/gcc/tree-vrp.c:1251:12: error: 'inv2' may be used uninitialized in this function [-Werror=maybe-uninitialized] tree inv = cst1 ? inv2 : inv1; ^~~ /home/andi/gcc/git/gcc/gcc/tree-vrp.c:1222:4: error: 'inv1' may be used uninitialized in this function [-Werror=maybe-uninitialized] if (strict_overflow_p != NULL ~~~~~~~~~~~~~~~~~~~~~~~~~ && (!inv1 || !TREE_NO_WARNING (val1)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Andi
On 08/08/2016 08:43 PM, Andi Kleen wrote: >> Ideally we'd have a test that we could more deeply analyze for paths >> through the CFG that can't be executed. Finding those paths usually >> both fixes the warning *and* results in better code. Even if we >> can't fix it now, we can file it away for future work > > It's multiple variables who are depending on each other, with complex > control flow inbetween. It's not surprising that it loses control > of all the combinations. > > You can take a look yourself here: > > /home/andi/gcc/git/gcc/gcc/tree-vrp.c: In function 'int compare_values_warnv(tree, tree, bool*)': > /home/andi/gcc/git/gcc/gcc/tree-vrp.c:1251:12: error: 'inv2' may be used uninitialized in this function [-Werror=maybe-uninitialized] > tree inv = cst1 ? inv2 : inv1; > ^~~ > /home/andi/gcc/git/gcc/gcc/tree-vrp.c:1222:4: error: 'inv1' may be used uninitialized in this function [-Werror=maybe-uninitialized] > if (strict_overflow_p != NULL > ~~~~~~~~~~~~~~~~~~~~~~~~~ > && (!inv1 || !TREE_NO_WARNING (val1)) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ From my reading it looks to me like we could use inv2 uninitialized. tree sym1 = get_single_symbol (val1, &neg1, &inv1); tree sym2 = get_single_symbol (val2, &neg2, &inv2); If sym1 results in a return value that is some useful tree and inv1 is true and cst1 is true via this call: const bool cst1 = is_gimple_min_invariant (val1); We then need sym2 to result in a return value from get_single_symbol to be NULL, but cst2 still be true via this statement: const bool cst2 = is_gimple_min_invariant (val2); If all those conditions can hold, then we'll use inv2 without initializing it AFAICT. The key here is can there be a value for val2 where val2 is an invariant not built from PLUS_EXPR, POINTER_PLUS_EXPR, MINUS_EXPR or NEGATE_EXPR. I think we can make that happen if val2 is an ADDR_EXPR with an invariant address. So it actually seems to me that unless ADDR_EXPR filtered out elsewhere that we've got a real bug here rather than a false positive. There may be others, that's just the first one that stood out. Or did I miss something? jeff
> > If sym1 results in a return value that is some useful tree and inv1 > is true and cst1 is true via this call: The only way for get_single_symbol to return a non NULL tree is to hit the return at the end -- and that always initializes inv and neg. And when the return is NULL the && prevents evaluating inv or neg. So I still think it's a false positive. -Andi
On 08/11/2016 09:28 PM, Andi Kleen wrote: >> >> If sym1 results in a return value that is some useful tree and inv1 >> is true and cst1 is true via this call: > > The only way for get_single_symbol to return a non NULL tree > is to hit the return at the end -- and that always initializes > inv and neg. Right. > > And when the return is NULL the && prevents evaluating > inv or neg. Consider the case where sym1 results in a non-null return value (and initializes neg1/inv1), but sym2 results in a null return value, leaving neg2/inv2 undefined, but cst2 is can still be true (ADDR_EXPR with an invariant address comes to mind). Thus we can get into these statements: tree cst = cst1 ? val1 : val2; tree inv = cst1 ? inv2 : inv1; Note carefully how they test cst1 and depending on its value, they may read val2 or inv2. Jeff
> Consider the case where sym1 results in a non-null return value (and > initializes neg1/inv1), but sym2 results in a null return value, leaving > neg2/inv2 undefined, but cst2 is can still be true (ADDR_EXPR with an > invariant address comes to mind). > > Thus we can get into these statements: > > > tree cst = cst1 ? val1 : val2; > tree inv = cst1 ? inv2 : inv1; > > > Note carefully how they test cst1 and depending on its value, they may > read val2 or inv2. The key here is that cst1 cannot be true if sym1 is non-null, same for cst2 and sym2. The code is guarded with: /* If one is of the form '[-]NAME + CST' and the other is constant, then it might be possible to say something depending on the constants. */ if ((sym1 && inv1 && cst2) || (sym2 && inv2 && cst1)) If this is the first case, then cst1 is false and val2 and inv1 are read. If this is the second case, then cst1 is true and val1 and inv2 are read. So inv2 is read only in the second case, and is initialized.
On 08/21/2016 02:30 PM, Eric Botcazou wrote: >> Consider the case where sym1 results in a non-null return value (and >> initializes neg1/inv1), but sym2 results in a null return value, leaving >> neg2/inv2 undefined, but cst2 is can still be true (ADDR_EXPR with an >> invariant address comes to mind). >> >> Thus we can get into these statements: >> >> >> tree cst = cst1 ? val1 : val2; >> tree inv = cst1 ? inv2 : inv1; >> >> >> Note carefully how they test cst1 and depending on its value, they may >> read val2 or inv2. > > The key here is that cst1 cannot be true if sym1 is non-null, same for cst2 > and sym2. > > The code is guarded with: > > /* If one is of the form '[-]NAME + CST' and the other is constant, then > it might be possible to say something depending on the constants. */ > if ((sym1 && inv1 && cst2) || (sym2 && inv2 && cst1)) > > If this is the first case, then cst1 is false and val2 and inv1 are read. > If this is the second case, then cst1 is true and val1 and inv2 are read. > > So inv2 is read only in the second case, and is initialized. THanks. Sometimes this stuff gets painful to follow :( It looks like Andi already checked in this change. Andi, please don't do that. THe patch wasn't approved at the time of your commit and there's no indication the change was regression tested. Jeff
On Fri, Sep 16, 2016 at 10:44:34AM -0600, Jeff Law wrote: > On 08/21/2016 02:30 PM, Eric Botcazou wrote: > > > Consider the case where sym1 results in a non-null return value (and > > > initializes neg1/inv1), but sym2 results in a null return value, leaving > > > neg2/inv2 undefined, but cst2 is can still be true (ADDR_EXPR with an > > > invariant address comes to mind). > > > > > > Thus we can get into these statements: > > > > > > > > > tree cst = cst1 ? val1 : val2; > > > tree inv = cst1 ? inv2 : inv1; > > > > > > > > > Note carefully how they test cst1 and depending on its value, they may > > > read val2 or inv2. > > > > The key here is that cst1 cannot be true if sym1 is non-null, same for cst2 > > and sym2. > > > > The code is guarded with: > > > > /* If one is of the form '[-]NAME + CST' and the other is constant, then > > it might be possible to say something depending on the constants. */ > > if ((sym1 && inv1 && cst2) || (sym2 && inv2 && cst1)) > > > > If this is the first case, then cst1 is false and val2 and inv1 are read. > > If this is the second case, then cst1 is true and val1 and inv2 are read. > > > > So inv2 is read only in the second case, and is initialized. > THanks. Sometimes this stuff gets painful to follow :( > > It looks like Andi already checked in this change. Andi, please don't do > that. THe patch wasn't approved at the time of your commit and there's no > indication the change was regression tested. I don't post any patches that are not regression tested. In fact the patch was needed to run proper regression tests with profiling on because it was a bootstrap fix. I had interpreted your earlier comments as approval, and imho the patch is obvious anyways. Also I would like to point out that such a long turn around time for bootstrap fixes (and only out of secondary considerations which had nothing to do with the actual bootstrap problem) is extremely disruptive for anyone who actually relies on working bootstraps. If there's no intention to keep --enable-werror working for all build options the default should probably change to --disable-werror. -Andi
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index 44dfc84..7f71f8a 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -893,6 +893,9 @@ get_single_symbol (tree t, bool *neg, tree *inv) bool neg_; tree inv_; + *inv = NULL_TREE; + *neg = false; + if (TREE_CODE (t) == PLUS_EXPR || TREE_CODE (t) == POINTER_PLUS_EXPR || TREE_CODE (t) == MINUS_EXPR)
From: Andi Kleen <ak@linux.intel.com> This patch fixes an bootstrap error with autoprofiledbootstrap due to uninitiliazed variables, because the compiler cannot figure out they don't need to be initialized in an error path. Just always initialize them. gcc/: 2016-08-06 Andi Kleen <ak@linux.intel.com> * tree-vrp.c (get_single_symbol): Always initialize inv and neg. --- gcc/tree-vrp.c | 3 +++ 1 file changed, 3 insertions(+)