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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 2 Dec 2022, Luca Fancellu wrote:
> > 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

I spent a couple of hours to try to get it to work. I also admit defeat.
Keep your original code, that's better.