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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@bugseng.com>
>>>> ---
>>>> 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.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)