Re: [PATCH v3 1/5] build: make cc-option properly deal with unrecognized sub-options

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jul 26, 2023 at 12:33:07PM +0200, Jan Beulich wrote:
> In options like -march=, it may be only the sub-option which is
> unrecognized by the compiler. In such an event the error message often
> splits option and argument, typically saying something like "bad value
> '<argument>' for '<option>'. Extend the grep invocation accordingly,
> also accounting for Clang to not mention e.g. -march at all when an
> incorrect argument was given for it.
> 
> To keep things halfway readable, re-wrap and re-indent the entire
> construct.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> In principle -e "$$pat" could now be omitted from the grep invocation,
> since if that matches, both $$opt and $$arg will, too. But I thought I'd
> leave it for completeness.
> ---
> v3: Fix build with make 4.3 and newer, where the treatment of \# has
>     changed.
> v2: Further relax grep patterns for clang, which doesn't mention -march
>     when complaining about an invalid argument to it.
> 
> --- a/Config.mk
> +++ b/Config.mk
> @@ -8,6 +8,7 @@ endif
>  comma   := ,
>  open    := (
>  close   := )
> +sharp   := \#
>  squote  := '
>  #' Balancing squote, to help syntax highlighting
>  empty   :=
> @@ -90,9 +91,14 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)
>  # of which would indicate an "unrecognized command-line option" 
> warning/error.
>  #
>  # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
> -cc-option = $(shell if test -z "`echo 'void*p=1;' | \
> -              $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- 
> $(2:-Wa$(comma)%=%) -`"; \
> -              then echo "$(2)"; else echo "$(3)"; fi ;)
> +cc-option = $(shell pat='$(2:-Wa$(comma)%=%)'; \
> +                    opt="$${pat%%=*}" arg="$${pat$(sharp)*=}"; \
> +                    if test -z "`echo 'void*p=1;' | \
> +                                 $(1) $(2) -c -o /dev/null -x c - 2>&1 | \
> +                                 grep -e "$$pat" -e "$$opt" -e "$$arg" -`"; \
> +                    then echo "$(2)"; \
> +                    else echo "$(3)"; \
> +                    fi;)

This patch looks fine. Shouldn't the comment been updated as well? At
the moment, it only discuss about -Wno-*, which it seems is why `grep`
was introduced in the first place.

But isn't it doing doing pattern matching on an error message going to
lead sometime to false positive? Linux's build system seems to works
fine by just using the exit value. They've got a few trick to deal with
-Wno-* and with clang.

For -Wno-$(warning), they test -W$(warning) instead. For clang, they've
enable additional warnings:
    -Werror=unknown-warning-option
    -Werror=ignored-optimization-argument
    -Werror=option-ignored
    -Werror=unused-command-line-argument


In any case, the patch is fine.
Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD