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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2023 09:01, Jan Beulich wrote:
> On 11.08.2023 15:48, Anthony PERARD wrote:
>> 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.
> 
> Right, but that has been an issue already before.
> 
>> But isn't it doing doing pattern matching on an error message going to
>> lead sometime to false positive?
> 
> There's a certain risk, of course.
> 
>> 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
> 
> Ah, yes, this would likely be a better way to test things. Time to redo
> what was done 12 years ago. I guess for the purpose of this series I'll
> keep what I have, but take note to rework things afterwards (which now
> would likely mean post-4.18, as the new-submissions deadline has passed).

Hmm, or maybe I could simply call this v4 then, especially when directly
integrating -Wno-* handling right here (by suitably using $(patsubst ...).
The extra Clang aspects (if indeed needed; didn't check yet) may not
easily be coverable that way, though.

Jan