| Message ID | 60da37cf-abec-be58-d433-e98eec0c59bd@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: allow Kconfig control over psABI level | expand |
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@suse.com> > --- > 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@citrix.com> Thanks,
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@suse.com> >> --- >> 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). > In any case, the patch is fine. > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks. Jan
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@suse.com> >>> --- >>> 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
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@suse.com> >> --- >> 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 I think using just -Werror is going to be enough. The completely changed patch below appears to be doing fine, but of course requires me to drop ... > In any case, the patch is fine. > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> ... this. Jan --- a/Config.mk +++ b/Config.mk @@ -81,17 +81,17 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix) # cc-option: Check if compiler supports first option, else fall back to second. # -# This is complicated by the fact that unrecognised -Wno-* options: +# This is complicated by the fact that with most gcc versions unrecognised +# -Wno-* options: # (a) are ignored unless the compilation emits a warning; and # (b) even then produce a warning rather than an error -# To handle this we do a test compile, passing the option-under-test, on a code -# fragment that will always produce a warning (integer assigned to pointer). -# We then grep for the option-under-test in the compiler's output, the presence -# of which would indicate an "unrecognized command-line option" warning/error. +# Further Clang also only warns for unrecognised -W* options. To handle this +# we do a test compile, substituting -Wno-* by -W* and adding -Werror. This +# way all unrecognised options are diagnosed uniformly, allowing us to merely +# check exit status. # # 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)%=%) -`"; \ +cc-option = $(shell if $(1) $(2:-Wno-%=-W%) -Werror -c -o /dev/null -x c /dev/null >/dev/null 2>&1; \ then echo "$(2)"; else echo "$(3)"; fi ;) # cc-option-add: Add an option to compilation flags, but only if supported.
On Wed, Aug 16, 2023 at 08:06:52AM +0200, Jan Beulich wrote: > On 11.08.2023 15:48, Anthony PERARD wrote: > > 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 > > I think using just -Werror is going to be enough. The completely changed Yes, looks like -Werror is enough. I'm not sure why Linux has them because they tests flags with -Werror in most cases. > patch below appears to be doing fine, but of course requires me to drop > ... > > > In any case, the patch is fine. > > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> > > ... this. > > Jan > > --- a/Config.mk > +++ b/Config.mk > @@ -81,17 +81,17 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix) > > # cc-option: Check if compiler supports first option, else fall back to second. > # > -# This is complicated by the fact that unrecognised -Wno-* options: > +# This is complicated by the fact that with most gcc versions unrecognised > +# -Wno-* options: > # (a) are ignored unless the compilation emits a warning; and > # (b) even then produce a warning rather than an error > -# To handle this we do a test compile, passing the option-under-test, on a code > -# fragment that will always produce a warning (integer assigned to pointer). > -# We then grep for the option-under-test in the compiler's output, the presence > -# of which would indicate an "unrecognized command-line option" warning/error. > +# Further Clang also only warns for unrecognised -W* options. To handle this > +# we do a test compile, substituting -Wno-* by -W* and adding -Werror. This > +# way all unrecognised options are diagnosed uniformly, allowing us to merely > +# check exit status. > # > # 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)%=%) -`"; \ > +cc-option = $(shell if $(1) $(2:-Wno-%=-W%) -Werror -c -o /dev/null -x c /dev/null >/dev/null 2>&1; \ > then echo "$(2)"; else echo "$(3)"; fi ;) I've try to compare the result of cc-option with and without this change in the gitlab CI, and it seems that the result is the same for the flags we tests. So this change looks fine: Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
--- 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;) # cc-option-add: Add an option to compilation flags, but only if supported. # Usage: $(call cc-option-add CFLAGS,CC,-march=winchip-c6)
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@suse.com> --- 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.