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

From: "Jan Beulich" <JBeulich@suse.com>
To: Julien Grall <julien.grall@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH for-4.9] x86/mm: Placate DEADCODE Coverity warning
Date: Thu, 01 Jun 2017 05:21:32 -0600	[thread overview]
Message-ID: <593014DC020000780015EABB@prv-mh.provo.novell.com> (raw)
In-Reply-To: <a873f459-5672-5c67-e5b7-ac22d82f5b41@arm.com>

>>> On 01.06.17 at 13:18, <julien.grall@arm.com> wrote:
> Hi Jan,
> 
> On 01/06/17 12:15, Jan Beulich wrote:
>>>>> On 01.06.17 at 13:09, <julien.grall@arm.com> 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@citrix.com> 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.

Jan

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