On Mon, Nov 29, 2021 at 4:40 PM Martin Sebor <mse...@gmail.com> wrote:
>
> 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.
Jeff meanwhile approved restoring the original. But yes, it's
odd that this did not have any visible effect (maybe fixing
the mistake resolved some uninit regressions - but IIRC none
were bisected to that rev.)
Richard.
>
> 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.
>
