Re: [XEN PATCH v4 1/2] automation/eclair: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 17 Nov 2023, Julien Grall wrote:
> Hi,
> 
> On 16/11/2023 08:45, Nicola Vetrini wrote:
> > On 2023-11-15 12:22, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 15/11/2023 11:02, Nicola Vetrini wrote:
> > > > On 2023-11-14 23:12, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 14/11/2023 15:36, Nicola Vetrini wrote:
> > > > > > To be able to check for the existence of the necessary subsections
> > > > > > in
> > > > > > the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a
> > > > > > source
> > > > > > file that is built.
> > > > > > 
> > > > > > This file is generated from 'C-runtime-failures.rst' in docs/misra
> > > > > > and the configuration is updated accordingly.
> > > > > > 
> > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > > > > > ---
> > > > > > Changes from RFC:
> > > > > > - Dropped unused/useless code
> > > > > > - Revised the sed command
> > > > > > - Revised the clean target
> > > > > > 
> > > > > > Changes in v2:
> > > > > > - Added explanative comment to the makefile
> > > > > > - printf instead of echo
> > > > > > 
> > > > > > Changes in v3:
> > > > > > - Terminate the generated file with a newline
> > > > > > - Build it with -std=c99, so that the documentation
> > > > > >    for D1.1 applies.
> > > > > > Changes in v5:
> > > > > > - Transform and build the file directly in the eclair-specific
> > > > > > directory
> > > > > > ---
> > > > > >   automation/eclair_analysis/build.sh   | 21 +++++++++++++++++++--
> > > > > >   automation/eclair_analysis/prepare.sh |  7 ++++---
> > > > > >   2 files changed, 23 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/automation/eclair_analysis/build.sh
> > > > > > b/automation/eclair_analysis/build.sh
> > > > > > index ec087dd822fa..f24292ed0643 100755
> > > > > > --- a/automation/eclair_analysis/build.sh
> > > > > > +++ b/automation/eclair_analysis/build.sh
> > > > > > @@ -33,12 +33,29 @@ else
> > > > > >     PROCESSORS=6
> > > > > >   fi
> > > > > >   +runtime_failures_docs() {
> > > > > > +  doc="C-runtime-failures.rst"
> > > > > > +  builddir="automation/eclair_analysis"
> > > > > > +
> > > > > > +  cp "docs/misra/${doc}" "${builddir}"
> > > > > 
> > > > > Is it necessary to copy the .rst? IOW, would it be sufficient to
> > > > > use...
> > > > > 
> > > > > > +  cd "${builddir}"
> > > > > > +  printf "/*\n\n" >"${doc}.tmp"
> > > > > > +  sed -e 's|\*/|*//*|g' "${doc}" >>"${doc}.tmp"
> > > > > 
> > > > > ... docs/misc/${doc} here?
> > > > > 
> > > > 
> > > > I didn't want to leave a stray file under docs/misra, but it's not
> > > > essential.
> > > 
> > > I am confused. I am suggesting to use:
> > > 
> > > sed -e 's|\*/|*//*|g' "../../docs/misc/${doc}" >> "${doc}.tmp"
> > > 
> > > So *.tmp is still created at the same place.
> > > 
> > 
> > Ok, makes sense.
> > 
> > > > 
> > > > > > +  printf "\n\n*/\n" >>"${doc}.tmp"
> > > > > > +  mv "${doc}.tmp" "${doc}.c"
> > > > > 
> > > > > NIT: I am not sure why you need to first create .tmp and then
> > > > > create.c.
> > > > > 
> > > > 
> > > > Wasn't this a pattern to defend against interruptions of the build, just
> > > > as I did in v3?
> > > > 
> > > > +$(TARGETS:.o=.c): %.c: %.rst
> > > > +    printf "/*\n\n" > $@.tmp
> > > > +    sed -e 's|\*/|*//*|g' $< >> $@.tmp
> > > > +    printf "\n\n*/\n" >> $@.tmp
> > > > +    mv $@.tmp $@
> > > 
> > > Yes but it makes sense for the Makefile because the target would not be
> > > re-executed if *.c exists.
> > > 
> > > But I don't think this is the case for you because you are using a bash
> > > script. So your function should always be re-executed regardless on
> > > whether it was interrupted or not.
> > > 
> > 
> > Ok.
> > 
> > > > 
> > > > > > +
> > > > > > +  # Cannot redirect to /dev/null because it would be excluded from
> > > > > > the analysis
> > > > > > +  "${CROSS_COMPILE}gcc-12" -std=c99 -c "${doc}.c" -o "${doc}.o"
> > > > > 
> > > > > NIT: It would be helpful to specify why -std=c99 is used. Above, you
> > > > > suggest this is to enable D1.1.
> > > > > 
> > > > 
> > > > Yeah, the comment in the changelog should be pasted here
> > > > 
> > > > > NIT: Can we define CC and use here and ...
> > > > > 
> > > > > > +  cd -
> > > > > > +}
> > > > > > +
> > > > > >   (
> > > > > > -  cd xen
> > > > > > +  runtime_failures_docs
> > > > > >       make "-j${PROCESSORS}" "-l${PROCESSORS}.0"    \
> > > > > >          "CROSS_COMPILE=${CROSS_COMPILE}"         \
> > > > > >          "CC=${CROSS_COMPILE}gcc-12"              \
> > > > > 
> > > > > ...? This would make easier to re-use the code.
> > > > > 
> > > > 
> > > > I don't expect this build script to be changed much to be honest, but if
> > > > you think
> > > > this is beneficial then it's ok.
> > > 
> > > This is not only about code evolving. It makes easier to spot your are
> > > using the same compiler. I would not have made the remark if you were
> > > using 'gcc'.
> > > 
> > > But I noticed you were using gcc-12 and originally thought it was a
> > > mistake until I saw the second use.
> > > 
> > > The advantage of a variable CC (and CXX) is you can add a comment on top
> > > why you are specifically requestion gcc-12? IOW, why is gcc not fine?
> > > 
> > 
> > The assumptions in C-language-toolchain.rst (which are reflected in the
> > analysis config) are using gcc-12 explicitly; that's just easier from a
> > certification perspective to have a fixed version.
> 
> I am not against fixed version. It just needs to be documented. At least
> reading C-language-toolchain.rst, it is not obvious to me that this is only
> applying to GCC-12.

I did a commit sweep for the old MISRA patches. I didn't commit this
series as I imagine it requires a respin.