[PATCH] Fix a missed case in predicate analysis of the late uninit pass

Vladislav Ivanishin vlad@ispras.ru
Thu Apr 4 14:05:00 GMT 2019

Richard Biener <richard.guenther@gmail.com> writes:

> On Mon, Apr 1, 2019 at 5:36 PM Vladislav Ivanishin <vlad@ispras.ru> wrote:
>> Hi!
>> This is a fairly trivial change fixing a false negative in
>> -Wmaybe-uninitialized.  I am pretty sure this is simply an overlooked
>> case (is_value_included_in() is not meant to deal with the case where
>> both compare codes are NE_EXPRs, neither does it need to, since their
>> handling is trivial).
>> In a nutshell, what happens is values of v restricted by (v != 2) are
>> incorrectly considered a subset of values of v restricted by (v != 1).
>> As if "v != 2, therefore v != 1".
>> This is by no means a gcc-9 regression; I'll ping the patch once stage4
>> is over, if needed.
>> This came up when I was experimenting with moving the uninit passes
>> around.  On mainline, the late uninit pass runs very late, so reliably
>> triggering the affected path is a bit tricky.  So I created a GIMPLE
>> test (it reproduces the behavior precisely, but might be fragile
>> w.r.t. future versions of the textual representation) and then with a
>> hint from Alexander managed to produce a simple C test.  [By the way,
>> the first take was to insert an asm with a lot of newlines to prevent
>> the dom pass from rewriting the CFG due to high cost of duplicating
>> instructions.  This didn't work out; I think the dom pass does not
>> respect sizes of inline asms.  I plan to create a testcase and file a
>> bug later.]
>> I ran regression testing on x86_64-pc-linux-gnu and saw no new
>> regressions modulo a handful of flaky UNRESOLVED tests under
>> gcc.dg/tree-prof.  `BOOT_CFLAGS="-O -Wno-error=maybe-uninitialized
>> -Wmaybe-uninitialized" bootstrap` also succeeded producing no new
>> warnings.
>> OK for stage1?
> Hum.  While definitely two NE_EXPR do not work correctly I'd like
> to see a positive test since LT_EXPR doesn't work either?

Right, thanks.  The case that was not covered well is actually when
cond2 == NE_EXPR (arbitrary cond1).  I created 2 new tests: uninit-26.c
demonstrates a false negative, and uninit-27-gimple.c a false positive
with trunk.

> Specifically the code falls through to test is_value_included_in which
> seems to assume code1 == code2.

The function is_value_included_in itself only cares about one condition
code (I'll expound on this below).  I agree though that the second part
of the comment seems to assume that.

Please take a look at the updated patch.  The rationale is as follows.

When we have 2 potentially comparable predicates in
is_pred_expr_subset_of, there are basically two things we want to check.

1) Whether two ranges with identical condition codes are nested.  This
is done straightforwardly with is_value_included_in.

2) Whether X CMPC VAL1 is nested in X != VAL2.  Which is easy: this
happens iff the set (a.k.a "range") {X: X CMPC VAL1 is true} doesn't
contain ("cover") VAL2, in other words iff VAL2 CMPC VAL1 is false.

Only, the logic of 2) is faulty when X CMPC VAL1 is not a range bounded
on one end (this happens when, and only when CMPC is NE_EXPR; the range
is then unbounded on both ends and can only be nested in X != VAL2, if
VAL1 == VAL2).

3) Whether X != VAL1 is nested in X CMPC VAL2, but that is always false
unless CMPC is NE_EXPR, which is already considered.

> To me it looks like is_value_includeds comment should be clarified to
> say
> /* Returns true if all values X satisfying X CMPC VAL satisfy

This is indeed more clear than the current comment, and the meaning is
the same.  I suggest however to not discuss nestedness of ranges in this
comment at all.

> That is, "all values in the range" in the current comment is
> under-specified since VAL is just a single value.

The range is implied, since only one CMPC is passed.  (If CMPC is
NE_EXPR, however, then I guess the range would only be known in the
caller, but the function does not work correctly for this purpose --
hence the patch to not call it then.)  But is_value_included_in actually
only cares about one value and one set anyway, as the name suggests.

So I think the second part of the comment is supposed to help to think
about applying this function for its main use-case (this function is
used twice, actually: in the case we are discussing there is a range
whose nestedness the calling code is testing, in the other case there is
just a constant), whereas the first part simply states what the function
does.  I'd say, the second part of the comment should be rewritten or
discarded, while the first should be kept.  OTOH, it might have been
helpful to the person who wrote this code.

> So I wonder what testcases regress if we remove the && code2 != NE_EXPR
> case?  That way we see what the intention was.  A patch should then
> change that to
>   if ((code1 != code2)
>       || !(<condition on code1> && code2 == NE_EXPR))
>    return false;
> to explicitely spell out what case was meant here.

make check-gcc RUNTESTFLAGS='dg.exp=uninit*' gives one regression:

gcc.dg/uninit-pred-9_b.c bogus warning (test for bogus messages, line 24)

This test boils down to this:

     if (m != 100)
        v = sth;
     if (m < 99)
        use (v);

So with the code2 != NE_EXPR check in place, expr1 = {m, 98, LE_EXPR},
expr2 = {m, 100, NE_EXPR}, and code2 = NE_EXPR are passed to
is_value_included_in, which returns true: 98 is included in m != 100
and true means "no warning".  This does not clarify the intention for
me, since this only works by luck; the condition that needs to be tested
cannot be tested with passing NE_EXPR to is_value_included_in, as the
new uninit-26.c test shows.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-NE_EXPR-handling-in-predicate-analysis-of-the-la.patch
Type: text/x-patch
Size: 6366 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190404/9bea0b5f/attachment.bin>

More information about the Gcc-patches mailing list