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