[PATCH] introduce predicate analysis class

Martin Sebor msebor@gmail.com
Mon Nov 29 15:40:00 GMT 2021


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.



More information about the Gcc-patches mailing list