Re: [RFC PATCH 2/4] xen/arm64: bitops: justify uninitialized variable inside a macro

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
> 
> The macro 'testop' expands to a function that declares the local
> variable 'oldbit', which is written before being set, but is such a
> way that is not amenable to automatic checking.
> 
> Therefore, a deviation comment, is introduced to document this situation.
> 
> A similar reasoning applies to macro 'guest_testop'.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
> docs/misra/safe.json                     | 16 ++++++++++++++++
> xen/arch/arm/arm64/lib/bitops.c          |  3 +++
> xen/arch/arm/include/asm/guest_atomics.h |  3 +++
> 3 files changed, 22 insertions(+)
> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index 244001f5be..4cf7cbf57b 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -20,6 +20,22 @@
>         },
>         {
>             "id": "SAF-2-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.R9.1"
> +            },
> +            "name": "Rule 9.1: initializer not needed",
> +            "text": "The following local variables are possibly subject to 
> being read before being written, but code inspection ensured that the control 
> flow in the construct where they appear ensures that no such event may 
> happen."
> +        },
> +        {
> +            "id": "SAF-3-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.R9.1"
> +            },
> +            "name": "Rule 9.1: initializer not needed",
> +            "text": "The following local variables are possibly subject to 
> being read before being written, but code inspection ensured that the control 
> flow in the construct where they appear ensures that no such event may 
> happen."
> +        },

Since the rule and the justification are the same, you can declare only once 
and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 
*/,
also, I remember some maintainers not happy about the misra rule being put 
after the tag, now I don’t recall who

> +        {
> +            "id": "SAF-4-safe",
>             "analyser": {},
>             "name": "Sentinel",
>             "text": "Next ID to be used"
> diff --git a/xen/arch/arm/arm64/lib/bitops.c b/xen/arch/arm/arm64/lib/bitops.c
> index 20e3f3d6ce..e0728bb29d 100644
> --- a/xen/arch/arm/arm64/lib/bitops.c
> +++ b/xen/arch/arm/arm64/lib/bitops.c
> @@ -114,8 +114,11 @@ bitop(change_bit, eor)
> bitop(clear_bit, bic)
> bitop(set_bit, orr)
> 
> +/* SAF-2-safe MC3R1.R9.1 */
> testop(test_and_change_bit, eor)
> +/* SAF-2-safe MC3R1.R9.1 */
> testop(test_and_clear_bit, bic)
> +/* SAF-2-safe MC3R1.R9.1 */
> testop(test_and_set_bit, orr)
> 
> static always_inline bool int_clear_mask16(uint16_t mask, volatile uint16_t 
> *p,
> diff --git a/xen/arch/arm/include/asm/guest_atomics.h 
> b/xen/arch/arm/include/asm/guest_atomics.h
> index a1745f8613..9d8f8ec3a3 100644
> --- a/xen/arch/arm/include/asm/guest_atomics.h
> +++ b/xen/arch/arm/include/asm/guest_atomics.h
> @@ -67,8 +67,11 @@ guest_bitop(change_bit)
> /* test_bit does not use load-store atomic operations */
> #define guest_test_bit(d, nr, p) ((void)(d), test_bit(nr, p))
> 
> +/* SAF-3-safe MC3R1.R9.1 */
> guest_testop(test_and_set_bit)
> +/* SAF-3-safe MC3R1.R9.1 */
> guest_testop(test_and_clear_bit)
> +/* SAF-3-safe MC3R1.R9.1 */
> guest_testop(test_and_change_bit)
> 
> #undef guest_testop
> -- 
> 2.34.1
> 
>