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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

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.

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.

[-- Attachment #2: Type: text/html, Size: 2328 bytes --]