Re: [v2] Proposal for deviations in static analyser findings

[Julien Grall <julien@xxxxxxx>]

On 25/10/2022 11:13, Luca Fancellu wrote:
> Hi all,
Hi Luca,

Some comments below if we plan to merge the doc in the tree.

> This is the V2 of the proposal for deviations tagging in the Xen codebase, this 
> includes
> all the feedbacks from the FuSa session held at the Xen Summit 2022 and all the
> feedbacks received in the previous proposal sent on the mailing list.
>
> Here a link to the previous thread:
> https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00541.html >
> Documenting violations
> ======================
>
> Static analysers are used on the Xen codebase for both static analysis and MISRA
> compliance.
> There might be the need to suppress some findings instead of fixing them and
> many tools permit the usage of in-code comments that suppress findings so that
> they are not shown in the final report.
>
> Xen will include a tool capable of translating a specific comment used in its
> codebase to the right proprietary in-code comment understandable by the selected
> analyser that suppress its finding.
>
> 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.
> - SAF-X-false-positive-<tool>: This tag means that the next line of code 
> contains a
>   finding, but the finding is a bug of the tool.
>
> SAF stands for Static Analyser Finding, the X is a placeholder for a positive
> number, the number after SAF- shall be incremental and unique, base ten
> notation and without leading zeros.
>
> Entries in the database should never be removed, even if they are not used
> anymore in the code (if a patch is removing or modifying the faulty line).
> This is to make sure that numbers are not reused which could lead to conflicts
> with old branches or misleading justifications.
>
> An entry can be reused in multiple places in the code to suppress a finding if
> and only if the justification holds for the same non-compliance to the coding
> standard.
>
> An orphan entry, that is an entry who was justifying a finding in the code, but 
> later
> that code was removed and there is no other use of that entry in the code, can 
> be
> reused as long as the justification for the finding holds. This is done to 
> avoid the
> allocation of a new entry with exactly the same justification, that would lead 
> to waste
> of space and maintenance issues of the database.
>
> The files where to store all the justifications are in xen/docs/misra/ and are
> named as safe.json and false-positive-<tool>.json, they have JSON format, 
> entries
> of these files have independent ID numbering.
>
> Here is an example to add a new justification in safe.json::
>
> |{
> |    "version": "1.0",
> |    "content": [
> |        {
> |            "id":"SAF-0-safe",
> |            "analyser": {
> |                "cppcheck": "misra-c2012-20.7",
> |                "coverity": "misra_c_2012_rule_20_7_violation",
> |                "eclair": "MC3R1.R20.7"
> |            },
> |            "name": “R20.7 C macro parameters not used as expression",
> |            "text": "The macro parameters used in this […]"
> |        },
> |        {
> |            "id":”SAF-1-safe",
> |            "analyser": {
> |                "cppcheck": "unreadVariable",
> |                "coverity": "UNUSED_VALUE"
> |            },
> |            "name": “Variable set but not used",
> |            "text": “It is safe because […]"
> |        },
> |        {
> |            "id":”SAF-2-safe",
> |            "analyser": {},
> |            "name": "Sentinel",
> |            "text": ""
> |        }
> |    ]
> |}
>
> Here is an example to add a new justification in false-positive-cppcheck.json::
>
> |{
> |    "version": "1.0",
> |    "content": [
> |        {
> |            "id":"SAF-0-false-positive-cppcheck",
> |            "analyser": {
> |                "cppcheck": "misra-c2012-20.7"
> |            },
> |            “tool-version”: “2.7",
> |            "name": “R20.7 second operand of member-access operator",
> |            "text": "The second operand of a member access operator shall be a name 
> of a member of the type pointed to, so in this particular case it is wrong to use parentheses on 
> the macro parameter."
> |        },
> |        {
> |            "id":”SAF-1-false-positive-cppcheck",
> |            "analyser": {},
> |            “tool-version”: “",
> |            "name": "Sentinel",
> |            "text": ""
> |        }
> |    ]
> |}
>
> To document a finding, just add another block {[...]} before the sentinel block,
> using the id contained in the sentinel block and increment by one the number
> contained in the id of the sentinel block.
>
> Here a brief explanation of the field inside an object of the "content" array:
You don't seem to have a longer explanation afterwards. So I would drop 
"brief".
> - id: it is a unique string that is used to refer to the finding, many finding
>   can be tagged with the same id, if the justification holds for any applied
>   case.
>   It tells the tool to substitute a Xen in-code comment having this structure:
>   /* SAF-0-safe [...] \*/
> - analyser: it is an object containing pair of key-value strings, the key is
>   the analyser, so it can be cppcheck, coverity or eclair. The value is the
>   proprietary id corresponding on the finding, for example when coverity is
>   used as analyser, the tool will translate the Xen in-code coment in this way:
>   /* SAF-0-safe [...] \*/ -> /* coverity[coverity-id] \*/
>   if the object doesn't have a key-value, then the corresponding in-code
>   comment won't be translated.
> - name: a simple name for the finding
> - text: a proper justification to turn off the finding.
>
>
>
> Here an example of the usage of the in-code comment tags to suppress a finding 
> for the Rule 8.6:
>
> Eclair reports it here:
> https://eclairit.com:3787/fs/var/lib/jenkins/jobs/XEN/configurations/axis-Target/ARM64/axis-agent/public/builds/549/archive/ECLAIR/out/PROJECT.ecd;/sources/xen/include/xen/kernel.h.html#R50743_1
How stable is this link?

The rest of the document LGTM.

Cheers,

--
Julien Grall