Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 06/10/2023 16:35, Julien Grall wrote:
> Hi Nicola,
> 
> On 06/10/2023 11:34, Nicola Vetrini wrote:
>> On 06/10/2023 12:22, Julien Grall wrote:
>>> On 06/10/2023 11:02, Nicola Vetrini wrote:
>>>> On 06/10/2023 11:29, Julien Grall wrote:
>>>>> Hi,
>>>>> 
>>>>> On 06/10/2023 09:26, Nicola Vetrini wrote:
>>>>>> The purpose of this macro is to encapsulate the well-known 
>>>>>> expression
>>>>>> 'x & -x', that in 2's complement architectures on unsigned 
>>>>>> integers will
>>>>>> give 2^ffs(x), where ffs(x) is the position of the lowest set bit 
>>>>>> in x.
>>>>>> 
>>>>>> A deviation for ECLAIR is also introduced.
>>>>> 
>>>>> Can you explain why this is a deviation in ECLAIR rather than one 
>>>>> with
>>>>> /* SAF-* */ (or whichever name we decide to rename to)? Is this
>>>>> because the code is correct from MISRA perspective but the tool is
>>>>> confused?
>>>>> 
>>>> 
>>>> The code does violate Rule 10.1 (a unary minus applied to an 
>>>> unsigned value is
>>>> deemed inappropriate by MISRA), but rather than changing a whole lot 
>>>> of places where this
>>>> construct is used (mainly in x86 code), the reasoning is that it 
>>>> makes more sense to isolate
>>>> it and justify its presence by the fact that on 2's complement 
>>>> architectures the result is
>>>> indeed correct.
>>> 
>>> This is explaining to me why you are adding LOWEST_POW2(). But this
>>> doesn't explain why you are not using /* SAF-* */ on top of
>>> LOWEST_POW2().
>>> 
>>> To me, we should only use ECLAIR specific deviation when this is a
>>> shortcoming with the tool.
>>> 
>>> Cheers,
>> 
>> Because of the way ECLAIR deviation comments work implies that in most 
>> cases the real
>> place where to put the deviation is the usage site
>> (the so-called top expansion location of the macro). Now, for 
>> widely-used macros this is
>> cumbersome and would clutter the code unnecessarily. It's way cleaner 
>> imo to have a single
>> line in the configuration with a clear justification that is present 
>> in the textual output
>> of the tool.
> 
> Just to clarify, you are saying that the following would not work for 
> Eclair:
> 
> /* SAF-XXX */
> #define LOWEST_POW2()
> 
> Instead you would need the following:
> 
> void foo()
> {
> 	/* SAF-XXX */
> 	LOWEST()
> }
> 
> Am I correct? If so, would something like below (untested) work?
> 
> #define LOWEST_POW2(...) ({ \
>    /* SAFE-XXX */           \
>    ...
>    })
> 

Option (2) would work. I'm not sure about (3), I'll test it.

>> But then there are tool interoperability considerations, that would 
>> call for standardized
>> deviation mechanisms, if they do detect this as a violation (which I 
>> don't know).
> 
> I don't think we need to know whether a tool detects it. We only need
> to know whether this is  violation to MISRA. If this is one, then this
> is a call to have a marker in the code.
> 
> If this is a false positive, then adding the deviation in the tool
> configuration is best (unless there are multiple tools affected).
> 

This is definitely a MISRA violation.

>> 
>> In the end, it could be done with a textual deviation, if that's 
>> preferred, but keep in mind
>> that those are more fragile w.r.t. code movement.
> 
> If the comment is around the macro there are limited chance that this
> will be missed. But if you are worried about code movement, you should
> be worried about macro renaming with your approach (one may not know
> Eclair has a deviation) and/or function with the same name.
> 

True, but if you introduce a violation on a guideline that is supposed 
to be clean then
the analysis will fail and show what's wrong (not by making the pipeline 
fail right now, but
ideally that's the plan). Reused identifiers are addressed by separate 
rules
(mainly Series 5).

> I am curious to know what the other thinks.
> 
> Cheers,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)