Messages in this thread | | | From | Nick Desaulniers <> | Date | Thu, 27 Sep 2018 15:17:41 -0700 | Subject | Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO |
| |
On Thu, Sep 27, 2018 at 2:51 PM Borislav Petkov <bp@alien8.de> wrote: > > On Thu, Sep 27, 2018 at 01:47:58PM -0700, ndesaulniers@google.com wrote: > > Early prototypes of Clang with asm goto support produce 6 instances of > > the following warning: > > > > In file included from arch/x86/boot/compressed/misc.h:20: > > In file included from ./include/linux/elf.h:5: > > In file included from ./arch/x86/include/asm/elf.h:8: > > In file included from ./include/linux/thread_info.h:38: > > In file included from ./arch/x86/include/asm/thread_info.h:53: > > ./arch/x86/include/asm/cpufeature.h:150:2: warning: "Compiler lacks > > ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments" > > [-W#warnings] > > your compiler arguments" > > ^ > > > > Since 6 files under arch/x86/boot/compressed/ include > > arch/x86/boot/compressed/misc.h AND > > arch/x86/boot/compressed/Makefile happens to redefine KBUILD_CFLAGS, > > which set these variables in the top level MAKEFILE. > > > > Suggested-by: Borislav Petkov <bp@suse.de> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > --- > > v1 -> v2: > > Updated commit message to provide more context as per Borislav. > > > > arch/x86/boot/compressed/Makefile | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > > index 28764dacf018..158c0b4e178a 100644 > > --- a/arch/x86/boot/compressed/Makefile > > +++ b/arch/x86/boot/compressed/Makefile > > @@ -56,6 +56,13 @@ KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \ > > endif > > LDFLAGS_vmlinux := -T > > > > +# check for 'asm goto' > > +ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y) > > + CC_HAVE_ASM_GOTO := 1 > > + KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO > > + KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO > > +endif > > I would still like to know why can't we do the -D_SETUP thing here: > https://lkml.kernel.org/r/20180926090841.GC5745@zn.tnic
That's another case that I look at and wonder "why does this exist?" The _SETUP guard exists in only one place: $ grep -rP 'ifdef\s+_SETUP' arch/x86/boot/cpucheck.c:#ifdef _SETUP
which is already under arch/x86/boot/. arch/x86/boot/Makefile unconditionally sets -D_SETUP, so what/who are we guarding against? Looks like a guard that's ALWAYS true (and thus could be removed).
> > instead of polluting this Makefile with defines which are not really > needed in the compressed kernel build, except to silence build warnings. > > I mean, we can perpetuate that ugly hack and do: > > #define __BPF_TRACING__ > > here in arch/x86/boot/compressed/misc.h which we could kill once clang > can do asm goto...
Or, or... we don't redefine KBUILD_CFLAGS in arch/x86/boot/Makefile (or any Makefile other than the top level one), and simply filter out the flags we DONT want, a la:
drivers/firmware/efi/libstub/Makefile: 16 cflags-$(CONFIG_ARM64) := $(subst -pg,,$(KBUILD_CFLAGS)) ...
ie, using Make's subst function to copy KBUILD_CFLAGS, filter out results, then use that for cflags-y. https://www.gnu.org/software/make/manual/html_node/Text-Functions.html
I'm curious to know Masahiro's thoughts on this? I can't help but shake the feeling that reassigning KBUILD_CFLAGS should be considered an anti-pattern and warned from checkpatch.pl. For the reasons enumerated above AND in v1: https://lore.kernel.org/lkml/CAKwvOdmLSVH7EVGY1ExU1Fh_hvL=FUzhq-80snDfZ+QhCT2FOA@mail.gmail.com/ (though there may be additional context from hpa answering https://lore.kernel.org/lkml/20180926090841.GC5745@zn.tnic/).
Relying on the compiler's default/implicit C standard (which changed in gcc 5) for parts of the kernel but not others I feel like should be a big red flag.
Shall I prototype up what such a change might look like (not reassigning KBUILD_CFLAGS in arch/x86/boot/Makefile)? Maybe it's harder/uglier than I imagine? -- Thanks, ~Nick Desaulniers
| |