Re: [PATCH] automation: Add container and build jobs to run cppcheck analysis

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 14/02/2023 00:56, Stefano Stabellini wrote:
> 
> 
> On Mon, 13 Feb 2023, Michal Orzel wrote:
>> Add a debian container with cppcheck installation routine inside,
>> capable of performing cppcheck analysis on Xen-only build including
>> cross-builds for arm32 and arm64.
>>
>> Populate build jobs making use of that container to run cppcheck
>> analysis to produce a text report (xen-cppcheck.txt) containing the list
>> of all the findings.
>>
>> This patch does not aim at performing any sort of bisection. Cppcheck is
>> imperfect and for now, our goal is to at least be aware of its reports,
>> so that we can compare them with the ones produced by better tools and
>> to be able to see how these reports change as a result of further
>> infrastructure improvements (e.g. exception list, rules exclusion).
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> Thanks for the patch, very nice!
> 
> 
>> ---
>> For those interested in, here is a sample pipeline:
>> https://gitlab.com/xen-project/people/morzel/xen-orzelmichal/-/pipelines/775769167
>> ---
>>  .../build/debian/unstable-cppcheck.dockerfile | 37 +++++++++++++++++
>>  automation/gitlab-ci/build.yaml               | 40 +++++++++++++++++++
>>  automation/scripts/build                      | 11 ++++-
>>  3 files changed, 87 insertions(+), 1 deletion(-)
>>  create mode 100644 automation/build/debian/unstable-cppcheck.dockerfile
>>
>> diff --git a/automation/build/debian/unstable-cppcheck.dockerfile 
>> b/automation/build/debian/unstable-cppcheck.dockerfile
>> new file mode 100644
>> index 000000000000..39bcc50673c8
>> --- /dev/null
>> +++ b/automation/build/debian/unstable-cppcheck.dockerfile
>> @@ -0,0 +1,37 @@
>> +FROM debian:unstable
>> +LABEL maintainer.name="The Xen Project" \
>> +      maintainer.email="xen-devel@xxxxxxxxxxxxxxxxxxxx"
>> +
>> +ENV DEBIAN_FRONTEND=noninteractive
>> +ENV CPPCHECK_VERSION=2.7
>> +ENV USER root
>> +
>> +RUN mkdir /build
>> +WORKDIR /build
>> +
>> +# dependencies for cppcheck and Xen-only build/cross-build
>> +RUN apt-get update && \
>> +    apt-get --quiet --yes install \
>> +        build-essential \
>> +        curl \
>> +        python-is-python3 \
>> +        libpcre3-dev \
>> +        flex \
>> +        bison \
>> +        gcc-arm-linux-gnueabihf \
>> +        gcc-aarch64-linux-gnu
>> +
>> +# cppcheck release build (see cppcheck readme.md)
>> +RUN curl -fsSLO 
>> https://github.com/danmar/cppcheck/archive/"$CPPCHECK_VERSION".tar.gz && \
>> +    tar xvzf "$CPPCHECK_VERSION".tar.gz && \
>> +    cd cppcheck-"$CPPCHECK_VERSION" && \
>> +    make install -j$(nproc) \
>> +        MATCHCOMPILER=yes \
>> +        FILESDIR=/usr/share/cppcheck \
>> +        HAVE_RULES=yes CXXFLAGS="-O2 -DNDEBUG -Wall -Wno-sign-compare 
>> -Wno-unused-function"
>> +
>> +# clean
>> +RUN apt-get autoremove -y && \
>> +    apt-get clean && \
>> +    rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/* && \
>> +    rm -rf cppcheck-"$CPPCHECK_VERSION"* "$CPPCHECK_VERSION".tar.gz
>> diff --git a/automation/gitlab-ci/build.yaml 
>> b/automation/gitlab-ci/build.yaml
>> index a053c5c7325d..c8831ccbec7a 100644
>> --- a/automation/gitlab-ci/build.yaml
>> +++ b/automation/gitlab-ci/build.yaml
>> @@ -7,6 +7,7 @@
>>      paths:
>>        - binaries/
>>        - xen-config
>> +      - xen-cppcheck.txt
>>        - '*.log'
>>        - '*/*.log'
>>      when: always
>> @@ -145,6 +146,23 @@
>>    variables:
>>      <<: *gcc
>>
>> +.arm64-cross-build-tmpl:
>> +  <<: *build
>> +  variables:
>> +    XEN_TARGET_ARCH: arm64
>> +  tags:
>> +    - x86_64
>> +
>> +.arm64-cross-build:
>> +  extends: .arm64-cross-build-tmpl
>> +  variables:
>> +    debug: n
>> +
>> +.gcc-arm64-cross-build:
>> +  extends: .arm64-cross-build
>> +  variables:
>> +    <<: *gcc
>> +
>>  .arm64-build-tmpl:
>>    <<: *build
>>    variables:
>> @@ -679,6 +697,28 @@ archlinux-current-gcc-riscv64-debug-randconfig:
>>      EXTRA_FIXED_RANDCONFIG:
>>        CONFIG_COVERAGE=n
>>
>> +# Cppcheck analysis jobs
>> +
>> +debian-unstable-gcc-cppcheck:
>> +  extends: .gcc-x86-64-build
>> +  variables:
>> +    CONTAINER: debian:unstable-cppcheck
>> +    CPPCHECK: y
>> +
>> +debian-unstable-gcc-arm32-cppcheck:
>> +  extends: .gcc-arm32-cross-build
>> +  variables:
>> +    CONTAINER: debian:unstable-cppcheck
>> +    CROSS_COMPILE: /usr/bin/arm-linux-gnueabihf-
>> +    CPPCHECK: y
>> +
>> +debian-unstable-gcc-arm64-cppcheck:
>> +  extends: .gcc-arm64-cross-build
>> +  variables:
>> +    CONTAINER: debian:unstable-cppcheck
>> +    CROSS_COMPILE: /usr/bin/aarch64-linux-gnu-
>> +    CPPCHECK: y
>> +
>>  ## Test artifacts common
>>
>>  .test-jobs-artifact-common:
>> diff --git a/automation/scripts/build b/automation/scripts/build
>> index f2f5e55bc04f..c219752d553e 100755
>> --- a/automation/scripts/build
>> +++ b/automation/scripts/build
>> @@ -38,7 +38,16 @@ cp xen/.config xen-config
>>  # Directory for the artefacts to be dumped into
>>  mkdir binaries
>>
>> -if [[ "${HYPERVISOR_ONLY}" == "y" ]]; then
>> +if [[ "${CPPCHECK}" == "y" ]]; then
>> +    # Cppcheck analysis invokes Xen-only build.
> 
> Given that when $CPPCHECK == y we are doing a hypervisor-only build,
> what do you think of also specifying $HYPERVISOR_ONLY == y in these
> cases?
> 
> We could set both CPPCHECK=y and HYPERVISOR_ONLY=y in build.yaml and
> then here also check for both.
Well, these are just cosmetic changes not impacting anything important, so
if you want, I am ok to explicitly set HYPERVISOR_ONLY=y for cppcheck jobs.

> 
> 
>> +    # Known limitation: cppcheck generates inconsistent reports when running
>> +    # in parallel mode, therefore do not specify -j<n>.
> 
> I take you tried -j$(nproc) on gitlab-ci and didn't work well? I tested
> -j$(nproc) in my native arm64 environment and seemed to work well.
Both me and Luca agreed on the fact that the reports are inconsistent when
running cppcheck analysis with -j$(nproc). It is not that it would fail to 
create
a report. Instead, there will be some internal cppcheck errors present in a 
report.
Therefore, to minimize the risk of producing incorrect list of findings it is 
best
not to specify -j<n>.

> 
> 
>> +    xen/scripts/xen-analysis.py --run-cppcheck --cppcheck-misra
>> +
>> +    # Preserve artefacts
>> +    cp xen/xen binaries/xen
>> +    cp xen/cppcheck-report/xen-cppcheck.txt xen-cppcheck.txt
>> +elif [[ "${HYPERVISOR_ONLY}" == "y" ]]; then
>>      # Xen-only build
>>      make -j$(nproc) xen
>>
>> --
>> 2.25.1
>>

~Michal