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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/10/2023 6:14 pm, Luca Fancellu wrote:
>
>> On 3 Oct 2023, at 17:17, andrew.cooper3@xxxxxxxxxx wrote:
>>
>> On 03/10/2023 4:37 pm, Nicola Vetrini wrote:
>>> As specified in rules.rst, these constants can be used
>>> in the code.
>>> Their deviation is now accomplished by using a SAF comment,
>>> rather than an ECLAIR configuration.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>> ---
>>> automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ------
>>> docs/misra/safe.json                             | 8 ++++++++
>>> xen/arch/x86/hvm/svm/emulate.c                   | 6 +++---
>>> xen/arch/x86/hvm/svm/svm.h                       | 9 +++++++++
>>> xen/common/inflate.c                             | 4 ++--
>>> 5 files changed, 22 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> index d8170106b449..fbb806a75d73 100644
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -132,12 +132,6 @@ safe."
>>> # Series 7.
>>> #
>>>
>>> --doc_begin="Usage of the following constants is safe, since they are given 
>>> as-is
>>> -in the inflate algorithm specification and there is therefore no risk of 
>>> them
>>> -being interpreted as decimal constants."
>>> --config=MC3R1.R7.1,literals={safe, 
>>> "^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$"}
>>> --doc_end
>>> -
>>> -doc_begin="Violations in files that maintainers have asked to not modify 
>>> in the
>>> context of R7.2."
>>> -file_tag+={adopted_r7_2,"^xen/include/xen/libfdt/.*$"}
>>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
>>> index 39c5c056c7d4..7ea47344ffcc 100644
>>> --- a/docs/misra/safe.json
>>> +++ b/docs/misra/safe.json
>>> @@ -20,6 +20,14 @@
>>>         },
>>>         {
>>>             "id": "SAF-2-safe",
>>> +            "analyser": {
>>> +                "eclair": "MC3R1.R7.1"
>>> +            },
>>> +            "name": "Rule 7.1: constants defined in specifications, 
>>> manuals, and algorithm descriptions",
>>> +            "text": "It is safe to use certain octal constants the way 
>>> they are defined in specifications, manuals, and algorithm descriptions."
>>> +        },
>>> +        {
>>> +            "id": "SAF-3-safe",
>>>             "analyser": {},
>>>             "name": "Sentinel",
>>>             "text": "Next ID to be used"
>>> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
>>> index aa2c61c433b3..c5e3341c6316 100644
>>> --- a/xen/arch/x86/hvm/svm/emulate.c
>>> +++ b/xen/arch/x86/hvm/svm/emulate.c
>>> @@ -90,9 +90,9 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned 
>>> int instr_enc)
>>>         if ( !instr_modrm )
>>>             return emul_len;
>>>
>>> -        if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) &&
>>> -             (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>>> -             (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )
>>> +        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) )  /* 
>>> SAF-2-safe */
>>>             return emul_len;
>>>     }
> Hi Andrew,
>
>> This is line noise, and later examples are even worse.
>>
>> What does SAF mean?  It's presumably not the Scalable Agile Framework.
> Please have a look on docs/misra/documenting-violations.rst, you will find 
> all the
> info about it.

Thankyou for proving my point perfectly.

The comment in the source code needs to be *far* clearer than it
currently is.

Even s/SAF/ANALYSIS/ would be an improvement, because it makes the
comment very clear that it's about code analysis.  An unknown initialism
like SAF does not convey enough meaning to be useful.

~Andrew