On 11/25/21 3:18 AM, Richard Biener wrote:
On Mon, Aug 30, 2021 at 10:06 PM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
The predicate analysis subset of the tree-ssa-uninit pass isn't
necessarily specific to the detection of uninitialized reads.
Suitably parameterized, the same core logic could be used in
other warning passes to improve their S/N ratio, or issue more
nuanced diagnostics (e.g., when an invalid access cannot be
ruled out but also need not in reality be unavoidable, issue
a "may be invalid" type of warning rather than "is invalid").
Separating the predicate analysis logic from the uninitialized
pass and exposing a narrow API should also make it easier to
understand and evolve each part independently of the other,
or replace one with a better implementation without modifying
the other.(*)
As the first step in this direction, the attached patch extracts
the predicate analysis logic out of the pass, turns the interface
into public class members, and hides the internals in either
private members or static functions defined in a new source file.
(**)
The changes should have no externally observable effect (i.e.,
should cause no changes in warnings), except on the contents of
the uninitialized dump. While making the changes I enhanced
the dumps to help me follow the logic. Turning some previously
free-standing functions into members involved changing their
signatures and adjusting their callers. While making these
changes I also renamed some of them as well some variables for
improved clarity. Finally, I moved declarations of locals
closer to their point of initialization.
Tested on x86_64-linux. Besides the usual bootstrap/regtest
I also tentatively verified the generality of the new class
interfaces by making use of it in -Warray-bounds. Besides there,
I'd like to make use of it in the new gimple-ssa-warn-access pass
and, longer term, any other flow-sensitive warnings that might
benefit from it.
This changed can_chain_union_be_invalidated_p from
for (size_t i = 0; i < uninit_pred.length (); ++i)
{
pred_chain c = uninit_pred[i];
size_t j;
for (j = 0; j < c.length (); ++j)
if (can_one_predicate_be_invalidated_p (c[j], use_guard))
break;
/* If we were unable to invalidate any predicate in C, then there
is a viable path from entry to the PHI where the PHI takes
an uninitialized value and continues to a use of the PHI. */
if (j == c.length ())
return false;
}
return true;
to
for (unsigned i = 0; i < preds.length (); ++i)
{
const pred_chain &chain = preds[i];
for (unsigned j = 0; j < chain.length (); ++j)
if (can_be_invalidated_p (chain[j], guard))
return true;
/* If we were unable to invalidate any predicate in C, then there
is a viable path from entry to the PHI where the PHI takes
an interesting value and continues to a use of the PHI. */
return false;
}
return true;
which isn't semantically equivalent (it also uses overloading to
confuse me). In particular the old code checked whether an
invalidation can happen for _each_ predicate in 'preds' while
the new one just checks preds[0], so the loop is pointless.
Catched by -Wunreachable-code complaining about unreachable
++i
Martin, was that change intended?
I didn't mean to make semantic changes (or to confuse you) so
the removal of the test is almost certainly an accident. It's
interesting that the change hasn't caused any other trouble.
We should look into that.
Martin
Richard.
Martin
[*] A review of open -Wuninitialized bugs I did while working
on this project made me aware of a number of opportunities to
improve the analyzer to reduce the number of false positives
-Wmaybe-uninitiailzed suffers from.
[**] The class isn't fully general and, like the uninit pass,
only works with PHI nodes. I plan to generalize it to compute
the set of predicates between any two basic blocks.
