Re: [PATCH 2/4] xen/scripts: add cppcheck tool to the xen-analysis.py script

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Stefano,

> I think the revert of the cppcheck integration in xen/Makefile and
> xen/tools/merge_cppcheck_reports.py could be a separate patch. There is
> no need to make sure cppcheck support in the xen Makefile is
> "bisectable". That patch could have my acked-by already.

Ok I will split these changes in a following patch

> 
> Also the document changes introduced in this patch have my reviewed-by:
> - docs/misra/cppcheck.txt
> - docs/misra/documenting-violations.rst
> - docs/misra/false-positive-cppcheck.json
> - docs/misra/xen-static-analysis.rst

Thank you, should I put those files in a separate patch with your rev-by before
this patch or this is just a comment for you to remember which file you already
reviewed?

>> 
>> +
>> +def generate_cppcheck_deps():
>> +    global cppcheck_extra_make_args
>> +
>> +    # Compile flags to pass to cppcheck:
>> +    # - include config.h as this is passed directly to the compiler.
>> +    # - define CPPCHECK as we use it to disable or enable some specific 
>> part of
>> +    #   the code to solve some cppcheck issues.
>> +    # - explicitely enable some cppcheck checks as we do not want to use 
>> "all"
>> +    #   which includes unusedFunction which gives wrong positives as we 
>> check
>> +    #   file per file.
>> +    # - Explicitly suppress warnings on compiler-def.h because cppcheck 
>> throws
>> +    #   an unmatchedSuppression due to the rule we put in 
>> suppression-list.txt
>> +    #   to skip every finding in the file.
>> +    #
>> +    # Compiler defines are in compiler-def.h which is included in config.h
>> +    #
>> +    cppcheck_flags="""
>> +--cppcheck-build-dir={}/{}
>> + --max-ctu-depth=10
>> + --enable=style,information,missingInclude
>> + 
>> --template=\'{{file}}({{line}},{{column}}):{{id}}:{{severity}}:{{message}}\'
>> + --relative-paths={}
>> + --inline-suppr
>> + --suppressions-list={}/suppression-list.txt
>> + --suppress='unmatchedSuppression:*generated/compiler-def.h'
>> + --include={}/include/xen/config.h
> 
> I noticed that some of the includes we used to have like
> xsm/flask/include are missing here. Is that intended?

Yes it is, now that cppcheck is using the JSON compilation database, it can 
understand
by the compilation argument “-I” what include path it needs to add, before we 
were
adding it to every file, resulting in some false positive from the tool.
Just --include={}/include/xen/config.h is needed because in the Xen makefile we 
are doing
the same, passing the option to the compiler, resulting in every compiled file 
to have that
header included.

>> 
>> +    case ${OPTION} in
>> +        -h|--help)
>> +            help
>> +            exit 0
>> +            ;;
>> +        --compiler=*)
>> +            COMPILER="$(eval echo "${OPTION#*=}")"
> 
> This can be:
> 
> COMPILER="${OPTION#*=}"
> 
> and same for all the other below

Ok I’ll fix that

> 
> 
>> +            sm_tool_args="n"
>> +            ;;
>> +        --cppcheck-cmd=*)
>> +            CPPCHECK_TOOL="$(eval echo "${OPTION#*=}")"
>> +            sm_tool_args="y"
>> +            ;;
>> +        --cppcheck-html)
>> +            CPPCHECK_HTML="y"
>> +            sm_tool_args="n"
>> +            ;;
>> +        --cppcheck-plat=*)
>> +            CPPCHECK_PLAT_PATH="$(eval echo "${OPTION#*=}")"
>> +            sm_tool_args="n"
>> +            ;;
>> +        --ignore-path=*)
>> +            IGNORE_PATH_LIST="${IGNORE_PATH_LIST} $(eval echo 
>> "${OPTION#*=}")"
>> +            sm_tool_args="n"
>> +            ;;
>> +        --)
>> +            forward_to_cc="y"
>> +            sm_tool_args="n"
>> +            ;;
>> +        *)
>> +            if [ "${sm_tool_args}" = "y" ]; then
>> +                CPPCHECK_TOOL_ARGS="${CPPCHECK_TOOL_ARGS} ${OPTION}"
>> +            else
>> +                echo "Invalid option ${OPTION}"
>> +                exit 1
> 
> It doesn't look like sm_tool_args is really needed? It is only set to
> 'y' in the case of --cppcheck-cmd, and in that case we also set
> CPPCHECK_TOOL. CPPCHECK_TOOL is the variable used below. Am I missing
> something?

We use sm_tool_args to fill CPPCHECK_TOOL_ARGS, basically it’s a state machine 
where
when we find --cppcheck-cmd=<xxx> we expect that every other space separated 
arguments
passed afterwards are the args for cppcheck, so we append to CPPCHECK_TOOL_ARGS
until we find an argument that is supposed to be only for this script.

> 
> 
>> +            fi
>> +            ;;
>> +    esac
>> +done
>> +
>> +if [ "${COMPILER}" = "" ]
>> +then
>> +    echo "--compiler arg is mandatory."
>> +    exit 1
>> +fi
>> +
>> +function print_file() {
>> +    local text="${1}"
>> +    local init_file="${2}"
>> +
>> +    if [ "${init_file}" = "y" ]
>> +    then
>> +        echo -e -n "${text}" > "${JDB_FILE}"
>> +    else
>> +        echo -e -n "${text}" >> "${JDB_FILE}"
>> +    fi
> 
> The >> can be used to create a file if the file is not already present.
> So why the need for this if? In fact, we don't need print_file at all
> and we can just 
> 
>  echo -e -n "something" >> "${JDB_FILE}"
> 
> directly from create_jcd. If you are concerned about a preexisting file,
> then at the beginning of create_jcd you can:
> 
>  rm "${JDB_FILE}"

Ok I’ll remove the file in the top of create_jcd and use echo -e -n "something" 
>> "${JDB_FILE}”

>> 
>> +
>> +        # Check wchar size
>> +        wchar_plat_suffix="t4"
>> +        # sed prints the last occurence of -f(no-)short-wchar which is the 
>> one
>> +        # applied to the file by the compiler
>> +        wchar_option=$(echo "${FORWARD_FLAGS}" | \
>> +            sed -nre 's,.*(-f(no-)?short-wchar).*,\1,p')
>> +        if [ "${wchar_option}" = "-fshort-wchar" ]
>> +        then
>> +            wchar_plat_suffix="t2"
>> +        fi
> 
> This seems a bit unnecessary: we should be able to find the right
> platform file from XEN_TARGET_ARCH alone. No need to reverse engineer
> the compiler command line?

The efi code is compiled with -fshort-wchar, but the rest of the file uses 
default length wchar,
now maybe it was a bit of overthinking because I guess we have only these cases:

arm64:   arm64-wchar_t2 (efi code uses -fshort-wchar)
arm32:   arm32-wchar_t4 (efi code is not in, but common-stub compiled with 
-f-no-short-wchar)
x86_64: x86_64-wchar_t2 (efi code uses -fshort-wchar)

Am I right? 

> 
> 
>> +
>> +        # Select the right target platform, ARCH is generated from Xen 
>> Makefile
>> +        
>> platform="${CPPCHECK_PLAT_PATH}/${ARCH}-wchar_${wchar_plat_suffix}.xml"
>> +        if [ ! -f "${platform}" ]
>> +        then
>> +            echo "${platform} not found!"
>> +            exit 1
>> +        fi
>> +
>> +        # Shellcheck complains about missing quotes on CPPCHECK_TOOL_ARGS, 
>> but
>> +        # they can't be used here
>> +        # shellcheck disable=SC2086
>> +        ${CPPCHECK_TOOL} ${CPPCHECK_TOOL_ARGS} \
>> +            --project="${JDB_FILE}" \
>> +            --output-file="${out_file}" \
>> +            --platform=${platform}
>> +
>> +        if [ "${CPPCHECK_HTML}" = "y" ]
>> +        then
>> +            # Shellcheck complains about missing quotes on 
>> CPPCHECK_TOOL_ARGS,
>> +            # but they can't be used here
>> +            # shellcheck disable=SC2086
>> +            ${CPPCHECK_TOOL} ${CPPCHECK_TOOL_ARGS} \
>> +                --project="${JDB_FILE}" \
>> +                --output-file="${out_file%.txt}.xml" \
>> +                --platform=${platform} \
>> +                -q \
>> +                --xml
> 
> This is showing my ignorance in cppcheck, but does it actually need to
> be called twice in the html generation case? Actually three times if we
> count the extra cppcheck-htmlreport call?

Cppcheck is not able to output a text report and an XML report at the same time,
hence we need to call it twice, but the second call will use the cppcheck build 
directory
As a “cache” to generate the results so it will be much more faster than the 
first one.

> 
> 
>> +        fi
>> +    fi
>> +fi
>> diff --git a/xen/tools/cppcheck-plat/arm32-wchar_t4.xml 
>> b/xen/tools/cppcheck-plat/arm32-wchar_t4.xml
>> new file mode 100644
>> index 000000000000..3aefa7ba5c98
>> --- /dev/null
>> +++ b/xen/tools/cppcheck-plat/arm32-wchar_t4.xml
>> @@ -0,0 +1,17 @@
>> +<?xml version="1.0"?>
>> +<platform>
>> +  <char_bit>8</char_bit>
>> +  <default-sign>unsigned</default-sign>
> 
> usually in C the default is actually "signed" not "unsigned". If you
> write:
> 
>  int i;
> 
> i is signed

It took me a bit to understand this field, as the documentation is not clear at 
all, the default-sign is referring
to the default char sign, which should be unsigned for arm, right?

Here the code to cppcheck that clarifies the field:

https://github.com/danmar/cppcheck/blob/2.7.5/lib/platform.cpp

At line 204, defaultSign is taking the value of <default-sign>, at line 64, 
when the platform is Native,
defaultSign = (std::numeric_limits<char>::is_signed) ? 's' : 'u';

I’ve done some tests with this code in arm/arm64/x86_64:

   #define is_type_signed(my_type) (((my_type)-1) < 0)
   if (is_type_signed(char))
        printf("signed\n");
    else
        printf("unsigned\n");

And I have unsigned for arm/arm64 and signed for x86_64 (which I will change as 
it is wrong in this patch)

Can you confirm my results are right?

>> 
>> +++ b/xen/tools/cppcheck-plat/arm64-wchar_t2.xml
>> @@ -0,0 +1,17 @@
>> +<?xml version="1.0"?>
>> +<platform>
>> +  <char_bit>8</char_bit>
>> +  <default-sign>unsigned</default-sign>
>> +  <sizeof>
>> +    <short>2</short>
>> +    <int>4</int>
>> +    <long>8</long>
>> +    <long-long>8</long-long>
>> +    <float>4</float>
>> +    <double>8</double>
>> +    <long-double>16</long-double>
>> +    <pointer>8</pointer>
>> +    <size_t>4</size_t>
> 
> Isn't size_t 8 bytes on arm64?

Yes you are right, I will fix it

> 
> 
>> +    <wchar_t>2</wchar_t>
>> +  </sizeof>
>> +</platform>