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

Vladislav Ivanishin vlad@ispras.ru
Fri Apr 5 12:26:00 GMT 2019


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

> On Thu, Apr 4, 2019 at 4:05 PM Vladislav Ivanishin <vlad@ispras.ru> wrote:
>>
>> 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.
>
> OK.  Your patch does
>
> +  if (code2 == NE_EXPR && code1 == NE_EXPR)
> +    return false;
>
> but it should instead return operand_equal_p (expr1.pred_rhs,
> expr2.pred_rhs, 0)?

This doesn't change the semantics, because the case with equal rhs's is
already considered at the beginning of this function:

static bool
is_pred_expr_subset_of (pred_info expr1, pred_info expr2)
{
  enum tree_code code1, code2;

  if (pred_equal_p (expr1, expr2))
    return true;

So I think, leaving this part of the patch as is results in less
localized/explicit code, but better matches the overall style of this
function.  Or perhaps add a checking assert?

  if (code1 == code2)
    gcc_checking_assert (!operand_equal_p (expr1.pred_rhs,
                                           expr2.pred_rhs, 0))

>> > 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
>> >    X CMPC BOUNDARY.  */
>>
>> 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.
>
> The new patch is OK with the change suggested above and the new
> comment for is_value_included_in spelling out how BOUNDARY and CMPC
> form the domain, all x so that x CMPC BOUNDARY is true vs. the also
> possible all x so that BOUNDARY CMPC x is true.

I updated this to say /* Returns whether VAL CMPC BOUNDARY is true.  */
As actually there is no need to define any "domain" entity here.

If this looks good, I'll ping the patch when stage1 opens, or ask
someone to apply.

> Thanks for explaining and the extra testcases.
> Richard.

Thank you for reviewing.
Vlad

-------------- 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: 6100 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190405/64ba7fe1/attachment.bin>


More information about the Gcc-patches mailing list