Re: [XEN PATCH] xen: Add SAF deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 4 Oct 2023, at 12:17, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 
> On 04/10/2023 11:52 am, Luca Fancellu wrote:
>> 
>> From the documentation:
>> 
>> In the Xen codebase, these tags will be used to document and suppress 
>> findings:
>> 
>> - SAF-X-safe: This tag means that the next line of code contains a finding, 
>> but
>> the non compliance to the checker is analysed and demonstrated to be safe.
>> 
>> I understand that Eclair is capable of suppressing also the line in which 
>> the in-code suppression
>> comment resides, but these generic Xen in-code suppression comment are meant 
>> to be used
>> by multiple static analysis tools and many of them suppress only the line 
>> next to the comment
>> (Coverity, cppcheck).
>> 
>> So I’m in favour of your approach below, clearly it depends on what the 
>> maintainers feedback is:
>> 
>> 
>>> /* SAF-2-safe */
>>> if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) &&
>>> /* SAF-2-safe */
>>> (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>>> /* SAF-2-safe */
>>> (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) )
> 
> To be clear, this is illegible and a non-starter from a code maintenance 
> point of view.
> 
> It is bad enough needing annotations to start with, but the annotations 
> *must* not interfere with the prior legibility.

I agree with that, the code as above is not very nice, however as the current 
status it is the only way it can work,
maybe rewriting it in another way could solve the issue?

For example:

/* SAF-2-safe */
#define SENSIBLE_NAME_HERE(instr)   MASK_EXTR(instr, 0300)
/* SAF-2-safe */
#define SENSIBLE_NAME_HERE2(instr)   MASK_EXTR(instr, 0300)
/* SAF-2-safe */
#define SENSIBLE_NAME_HERE3(instr)   MASK_EXTR(instr, 0300)

And use these macro in the conditions above, however will it move the violation 
at the macro definition?

Having macro with a sensible name explaining the meaning of the value could 
also improve the readability of the code.

But this choice is up on you x86 maintainers.

> 
> The form with comments on the end, that do not break up the tabulation of the 
> code, is tolerable, providing the SAF turns into something meaningful.
> 
> ~Andrew
> 
> P.S. to be clear, I'm not saying that an ahead-of-line comments are 
> unacceptable generally.  Something like
> 
>     /* $FOO-$N-safe */
>     if ( blah )
> 
> might be fine in context, but that is a decision that needs to be made based 
> on how the code reads with the comment in place.