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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 30 Aug 2023, Simone Ballarin wrote:
> 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*/

Yes, this is fine, it was always intended to be possible to add the
name of the deviation or a short comment in the in-code comment


> /*
>  * 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)
>