Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

On 29/08/23 00:27, Stefano Stabellini wrote:
> +Nicola, Luca
>
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
> > xen/arch/x86/usercopy.c includes itself, so it is not supposed to
> > comply with Directive 4.10:
> > "Precautions shall be taken in order to prevent the contents of a
> > header file being included more than once"
> >
> > This patch adds a deviation for the file.
> >
> > Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> >
> > ---
> >   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
> >   docs/misra/rules.rst                             | 2 ++
> >   2 files changed, 6 insertions(+)
> >
> > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > index 2681a4cff5..a7d4f29b43 100644
> > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > @@ -96,6 +96,10 @@ conform to the directive."
> >   -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no 
> > inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
> >   -doc_end
> >   
> > +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive"
> > +-config=MC3R1.D4.10,reports+={deliberate, 
> > "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
> > +-doc_end
> > +
> >   #
> >   # Series 5.
> >   #
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > index 4b1a7b02b6..45e13d0302 100644
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
> >        - Files that are intended to be included more than once do not need to
> >          conform to the directive. Files that explicitly avoid inclusion guards
> >          under specific circumstances do not need to conform the directive.
> > +       xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
> > +       with the directive.
>
> We need to find a consistent way to document this kind of deviations in
> a non-ECLAIR specific way, without adding the complete list of
> deviations to rules.rst.
>
> Can we use safe.json and add an in-code comment at the top of
> usercopy.c? E.g.:
>
> diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> index b8c2d1cc0b..8bb591f472 100644
> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -1,3 +1,4 @@
> +/* SAF-1-safe */
>   /*
>    * User address space access functions.
>    *
>  > Otherwise, maybe we should extend safe.json to also have an extra field
> with a list of paths. For instance see "files" below >
> {
>      "version": "1.0",
>      "content": [
>          {
>              "id": "SAF-0-safe",
>              "analyser": {
>                  "eclair": "MC3R1.R8.6",
>                  "coverity": "misra_c_2012_rule_8_6_violation"
>              },
>              "name": "Rule 8.6: linker script defined symbols",
>              "text": "It is safe to declare this symbol because it is defined in the 
> linker script."
>          },
>          {
>              "id": "SAF-1-safe",
>              "analyser": {
>                  "eclair": "MC3R1.D4.10"
>              },
>              "name": "Dir 4.10: files that include themselves",
>              "text": "Files purposely written to include themselves are not supposed 
> to comply with D4.10.",
>              "files": ["xen/arch/x86/usercopy.c"]
>          },
>          {
>              "id": "SAF-2-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
>          }
>      ]
> }
In general, I prefer the first option for such ad hoc deviation (the 
comment at the beginning of the file): this way, anyone who touches the 
file will immediately see the comment and think as its changes will 
affect the deviation (is it still safe? is it still necessary?).
To help the developer more, I think it is better to also add the "name" 
in the comment, this is my proposal:
/* SAF-4-safe Dir 4.10: files that include themselves*/
/*
 * User address space access functions.
 *
 * Copyright 1997 Andi Kleen <ak@xxxxxx>
 * Copyright 1997 Linus Torvalds
 * Copyright 2002 Andi Kleen <ak@xxxxxxx>
 */

--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)