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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 1 Dec 2022, at 20:23, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Thu, 1 Dec 2022, Luca Fancellu wrote:
>> Hi Stefano,
>> 
>>>>> 
>>>>> 
>>>>>> +            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.
>>> 
>>> That seems a bit unnecessary: if the user wants to pass arguments to
>>> cppcheck, the user would do --cppcheck-cmd="cppcheck arg1 arg2" with ""
>>> quotes. Doing that should make --cppcheck-cmd="cppcheck arg1 arg2" be
>>> seen as a single argument from this script point of view. CPPCHECK_TOOL
>>> would end up being set to "cppcheck arg1 arg2" which is what we want
>>> anyway? And if we need to distinguish between the cppcheck binary and
>>> its argument we could use "cut" to extract "cppcheck", "arg1", and
>>> "arg2" from CPPCHECK_TOOL.  Would that work?
>>> 
>> 
>> I gave a try for the quotes, the problem is that we need to have quotes in 
>> CC=“...”, so adding
>> quotes also to --cppcheck-cmd= which is inside CC=“...” is preventing the 
>> Makefile to work,
>> I tried escaping etc but I didn’t manage to have it working, so would you 
>> agree on keeping it
>> like that?
> 
> Is the problem coming from the following?
> 
>    cppcheck_cc_flags = """--compiler={} --cppcheck-cmd={} {}
> --cppcheck-plat={}/cppcheck-plat --ignore-path=tools/
> """.format(xen_cc, settings.cppcheck_binpath, cppcheck_flags,
>           settings.tools_dir)
> 
>    if settings.cppcheck_html:
>        cppcheck_cc_flags = cppcheck_cc_flags + " --cppcheck-html"
> 
>    # Generate the extra make argument to pass the cppcheck-cc.sh wrapper as CC
>    cppcheck_extra_make_args = "CC=\"{}/cppcheck-cc.sh {} --\"".format(
>                                        settings.tools_dir,
>                                        cppcheck_cc_flags
>                                    ).replace("\n", "")
> 
> 
> Wouldn't something like the following solve the issue?
> 
>    settings.cppcheck_binpath = settings.cppcheck_binpath + " " + 
> cppcheck_cc_flags
> 
>    cppcheck_cc_flags = """--compiler={} --cppcheck-cmd=\"{}\"
> --cppcheck-plat={}/cppcheck-plat --ignore-path=tools/
> """.format(xen_cc, settings.cppcheck_binpath, settings.tools_dir)
> 
>    if settings.cppcheck_html:
>        cppcheck_cc_flags = cppcheck_cc_flags + " --cppcheck-html"
> 
>    # Generate the extra make argument to pass the cppcheck-cc.sh wrapper as CC
>    cppcheck_extra_make_args = "CC=\"{}/cppcheck-cc.sh {} --\"".format(
>                                        settings.tools_dir,
>                                        cppcheck_cc_flags
>                                    ).replace("\n", "")

No unfortunately not, Makefile is very sensitive to quotes, I’ve tried with 
many combination of single/double quotes but nothing worked