Re: [Xen-devel] [PATCH for-4.9] x86/mm: Placate DEADCODE Coverity warning

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/06/17 18:25, Julien Grall wrote:
> CC Ian + Wei for the testing
>
> On 01/06/17 12:21, Jan Beulich wrote:
>>>>> On 01.06.17 at 13:18, <julien.grall@xxxxxxx> wrote:
>>> Hi Jan,
>>>
>>> On 01/06/17 12:15, Jan Beulich wrote:
>>>>>>> On 01.06.17 at 13:09, <julien.grall@xxxxxxx> wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> On 31/05/17 14:23, Andrew Cooper wrote:
>>>>>> On 31/05/17 09:52, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 05/22/2017 02:32 PM, Jan Beulich wrote:
>>>>>>>>>>> On 22.05.17 at 15:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>>>>> _PAGE_GNTTAB is only used in debug builds of Xen; in release
>>>>>>>>> builds,
>>>>>>>>> it has
>>>>>>>>> the value 0.  Coverity complains that "l1e_get_flags(l1e) & 0" is
>>>>>>>>> logically
>>>>>>>>> dead.
>>>>>>>>>
>>>>>>>>> Add an extra condition into the logic to skip the flag check if
>>>>>>>>> _PAGE_GNTTAB
>>>>>>>>> is 0.
>>>>>>>>
>>>>>>>> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) &&
>>>>>>>> ... )" look
>>>>>>>> logically the same (i.e. I'd expect the same warnings to be
>>>>>>>> triggered
>>>>>>>> [or not]).
>>>>>>>
>>>>>>> I haven't seen any answer on this question. Andrew, does this patch
>>>>>>> still hold for Xen 4.9?
>>>>>>
>>>>>> Sorry - it fell through the cracks, but yes, it does stand for 4.9.
>>>>>>
>>>>>> As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very
>>>>>> clearly a "short circuit every thing else if this value is zero",
>>>>>> while
>>>>>> the other looks like a programming mistake, which is also why I
>>>>>> expect
>>>>>> this to resolve Coverity's complaint.
>>>>>>
>>>>>> Unfortunately, I can't be certain that this will resolve the
>>>>>> issue until
>>>>>> it gets committed, as I don't have a useful way to run Coverity on
>>>>>> arbitrary non-debug builds.
>>>>>
>>>>> Are we running staging-4.* branch on Coverity? Looking at the git,
>>>>> I see
>>>>> *coverity* branch only on unstable.
>>>>
>>>> Yes, so my suggestion would be to commit the patch on master, see
>>>> if it helps, and if so consider backporting for 4.9. If it doesn't
>>>> help,
>>>> it should be reverted or replaced by something "better".
>>>
>>> Well master has now debug enabled and AFAIU the warning can only
>>> occurred on non-debug build. So I am not sure how this would help to be
>>> in master.
>>
>> Oh, good point. I think it would really be a good idea to have
>> both debug and non-debug builds tested, which at once would
>> avoid sudden bursts of issues when switching the default.

We cannot test both at the same time.

The free Coverity Scan only offers a single stream, which means we can
only submit a single logical stream of successive changes.

Ideally, this patch would have gotten in to staging while coverity was
still running the release build, so we would have gotten a positive
answer on the next scan.

Without taking this patch (assuming it fixes the issue), we will have
the same issue flap in our results every time we do a release.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel