On 4/8/19 3:00 AM, Richard Biener wrote:
> On Fri, Apr 5, 2019 at 2:26 PM Vladislav Ivanishin <v...@ispras.ru> wrote:
>>
>> Richard Biener <richard.guent...@gmail.com> writes:
>>
>>> On Thu, Apr 4, 2019 at 4:05 PM Vladislav Ivanishin <v...@ispras.ru> wrote:
>>>>
>>>> Richard Biener <richard.guent...@gmail.com> writes:
>>>>
>>>>> On Mon, Apr 1, 2019 at 5:36 PM Vladislav Ivanishin <v...@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?
>
> Ah, I looked for but missed this check...
>
>> if (code1 == code2)
>> gcc_checking_assert (!operand_equal_p (expr1.pred_rhs,
>> expr2.pred_rhs, 0))
>
> No, I don't think that's needed.
>
>>>>> 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.
Given your two comments earlier, I think this is ready to go onto the
trunk. I'll install it momentarily.
jeff
>
