| Message ID | 20190410201300.3528984-2-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] s390: only build for new CPUs with clang | expand |
On Wed, Apr 10, 2019 at 1:13 PM Arnd Bergmann <arnd@arndb.de> wrote: > > The purgatory and boot Makefiles do not inherit the original cflags, > so clang falls back to the default target architecture when building it, > typically this would be x86 when cross-compiling. > > Add $(CLANG_FLAGS) everywhere so we pass the correct --target=s390x-linux > option when cross-compiling. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/s390/Makefile | 5 +++-- > arch/s390/purgatory/Makefile | 1 + > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/Makefile b/arch/s390/Makefile > index 9c079a506325..443990791099 100644 > --- a/arch/s390/Makefile > +++ b/arch/s390/Makefile > @@ -17,12 +17,13 @@ KBUILD_CFLAGS_MODULE += -fPIC > KBUILD_AFLAGS += -m64 > KBUILD_CFLAGS += -m64 > aflags_dwarf := -Wa,-gdwarf-2 > -KBUILD_AFLAGS_DECOMPRESSOR := -m64 -D__ASSEMBLY__ > +KBUILD_AFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -D__ASSEMBLY__ > KBUILD_AFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),$(aflags_dwarf)) > -KBUILD_CFLAGS_DECOMPRESSOR := -m64 -O2 > +KBUILD_CFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -O2 > KBUILD_CFLAGS_DECOMPRESSOR += -DDISABLE_BRANCH_PROFILING -D__NO_FORTIFY > KBUILD_CFLAGS_DECOMPRESSOR += -fno-delete-null-pointer-checks -msoft-float > KBUILD_CFLAGS_DECOMPRESSOR += -fno-asynchronous-unwind-tables Thanks for the respin with Nathan's suggestion. > +KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning,pointer-sign) What's up with this ^ ? Seems like the top level sets it (without cc-disable-warning :( ), but then KBUILD_CFLAGS_DECOMPRESSOR discards it. Does Clang actually flag code in this arch (that GCC doesn't)? > KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-option,-ffreestanding) > KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),-g) > KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO_DWARF4), $(call cc-option, -gdwarf-4,)) > diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile > index ce6a3f75065b..ecd0b3847fef 100644 > --- a/arch/s390/purgatory/Makefile > +++ b/arch/s390/purgatory/Makefile > @@ -22,6 +22,7 @@ KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes > KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare > KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding > KBUILD_CFLAGS += -c -MD -Os -m64 -msoft-float -fno-common > +KBUILD_CFLAGS += $(CLANG_FLAGS) > KBUILD_CFLAGS += $(call cc-option,-fno-PIE) > KBUILD_AFLAGS := $(filter-out -DCC_USING_EXPOLINE,$(KBUILD_AFLAGS)) > > -- > 2.20.0 > -- Thanks, ~Nick Desaulniers
On Thu, Apr 11, 2019 at 12:14 AM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > On Wed, Apr 10, 2019 at 1:13 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > The purgatory and boot Makefiles do not inherit the original cflags, > > so clang falls back to the default target architecture when building it, > > typically this would be x86 when cross-compiling. > > > > Add $(CLANG_FLAGS) everywhere so we pass the correct --target=s390x-linux > > option when cross-compiling. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > arch/s390/Makefile | 5 +++-- > > arch/s390/purgatory/Makefile | 1 + > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/arch/s390/Makefile b/arch/s390/Makefile > > index 9c079a506325..443990791099 100644 > > --- a/arch/s390/Makefile > > +++ b/arch/s390/Makefile > > @@ -17,12 +17,13 @@ KBUILD_CFLAGS_MODULE += -fPIC > > KBUILD_AFLAGS += -m64 > > KBUILD_CFLAGS += -m64 > > aflags_dwarf := -Wa,-gdwarf-2 > > -KBUILD_AFLAGS_DECOMPRESSOR := -m64 -D__ASSEMBLY__ > > +KBUILD_AFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -D__ASSEMBLY__ > > KBUILD_AFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),$(aflags_dwarf)) > > -KBUILD_CFLAGS_DECOMPRESSOR := -m64 -O2 > > +KBUILD_CFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -O2 > > KBUILD_CFLAGS_DECOMPRESSOR += -DDISABLE_BRANCH_PROFILING -D__NO_FORTIFY > > KBUILD_CFLAGS_DECOMPRESSOR += -fno-delete-null-pointer-checks -msoft-float > > KBUILD_CFLAGS_DECOMPRESSOR += -fno-asynchronous-unwind-tables > > Thanks for the respin with Nathan's suggestion. > > > +KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning,pointer-sign) > > What's up with this ^ ? Seems like the top level sets it (without > cc-disable-warning :( ), but then KBUILD_CFLAGS_DECOMPRESSOR discards > it. Does Clang actually flag code in this arch (that GCC doesn't)? Oops, that should have been a separate patch. I think what happens is that clang warns more aggressively about pointer sign bugs than gcc in some cases, and some of those cases happen in s390 header files that are included by both the kernel and the decompressor. The full warning log without this change is rather long, see https://pastebin.com/KG9xaTNB I also tried patching the code to avoid the warnings, but I'm not entirely happy with that result either, see https://pastebin.com/pSMz5eZA Arnd
On Thu, Apr 11, 2019 at 1:52 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Apr 11, 2019 at 12:14 AM 'Nick Desaulniers' via Clang Built > Linux <clang-built-linux@googlegroups.com> wrote: > > On Wed, Apr 10, 2019 at 1:13 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > The purgatory and boot Makefiles do not inherit the original cflags, > > > so clang falls back to the default target architecture when building it, > > > typically this would be x86 when cross-compiling. > > > > > > Add $(CLANG_FLAGS) everywhere so we pass the correct --target=s390x-linux > > > option when cross-compiling. > > > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > --- > > > arch/s390/Makefile | 5 +++-- > > > arch/s390/purgatory/Makefile | 1 + > > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/s390/Makefile b/arch/s390/Makefile > > > index 9c079a506325..443990791099 100644 > > > --- a/arch/s390/Makefile > > > +++ b/arch/s390/Makefile > > > @@ -17,12 +17,13 @@ KBUILD_CFLAGS_MODULE += -fPIC > > > KBUILD_AFLAGS += -m64 > > > KBUILD_CFLAGS += -m64 > > > aflags_dwarf := -Wa,-gdwarf-2 > > > -KBUILD_AFLAGS_DECOMPRESSOR := -m64 -D__ASSEMBLY__ > > > +KBUILD_AFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -D__ASSEMBLY__ > > > KBUILD_AFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),$(aflags_dwarf)) > > > -KBUILD_CFLAGS_DECOMPRESSOR := -m64 -O2 > > > +KBUILD_CFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -O2 > > > KBUILD_CFLAGS_DECOMPRESSOR += -DDISABLE_BRANCH_PROFILING -D__NO_FORTIFY > > > KBUILD_CFLAGS_DECOMPRESSOR += -fno-delete-null-pointer-checks -msoft-float > > > KBUILD_CFLAGS_DECOMPRESSOR += -fno-asynchronous-unwind-tables > > > > Thanks for the respin with Nathan's suggestion. > > > > > +KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning,pointer-sign) > > > > What's up with this ^ ? Seems like the top level sets it (without > > cc-disable-warning :( ), but then KBUILD_CFLAGS_DECOMPRESSOR discards > > it. Does Clang actually flag code in this arch (that GCC doesn't)? > > Oops, that should have been a separate patch. > > I think what happens is that clang warns more aggressively about pointer sign > bugs than gcc in some cases, and some of those cases happen in s390 > header files that are included by both the kernel and the decompressor. > > The full warning log without this change is rather long, see > https://pastebin.com/KG9xaTNB From this link, it looks like the definitions of: __atomic64_or __atomic64_and __atomic64_xor and their *_barrier variants are problematic. I think converting those to use unsigned long is the way to go. Shouldn't you be doing bitwise ops on unsigned types anyways? The warnings with __atomic64_add are tougher to read/understand since at that point the log lines look like they start to mix together. > > I also tried patching the code to avoid the warnings, but I'm not entirely > happy with that result either, see > https://pastebin.com/pSMz5eZA That's no terrible, IMO, particularly with the change I suggest above. -- Thanks, ~Nick Desaulniers
On Thu, 11 Apr 2019 11:08:31 -0700 Nick Desaulniers <ndesaulniers@google.com> wrote: > On Thu, Apr 11, 2019 at 1:52 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Thu, Apr 11, 2019 at 12:14 AM 'Nick Desaulniers' via Clang Built > > Linux <clang-built-linux@googlegroups.com> wrote: > > > On Wed, Apr 10, 2019 at 1:13 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > > > The purgatory and boot Makefiles do not inherit the original cflags, > > > > so clang falls back to the default target architecture when building it, > > > > typically this would be x86 when cross-compiling. > > > > > > > > Add $(CLANG_FLAGS) everywhere so we pass the correct --target=s390x-linux > > > > option when cross-compiling. > > > > > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > --- > > > > arch/s390/Makefile | 5 +++-- > > > > arch/s390/purgatory/Makefile | 1 + > > > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/s390/Makefile b/arch/s390/Makefile > > > > index 9c079a506325..443990791099 100644 > > > > --- a/arch/s390/Makefile > > > > +++ b/arch/s390/Makefile > > > > @@ -17,12 +17,13 @@ KBUILD_CFLAGS_MODULE += -fPIC > > > > KBUILD_AFLAGS += -m64 > > > > KBUILD_CFLAGS += -m64 > > > > aflags_dwarf := -Wa,-gdwarf-2 > > > > -KBUILD_AFLAGS_DECOMPRESSOR := -m64 -D__ASSEMBLY__ > > > > +KBUILD_AFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -D__ASSEMBLY__ > > > > KBUILD_AFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),$(aflags_dwarf)) > > > > -KBUILD_CFLAGS_DECOMPRESSOR := -m64 -O2 > > > > +KBUILD_CFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -O2 > > > > KBUILD_CFLAGS_DECOMPRESSOR += -DDISABLE_BRANCH_PROFILING -D__NO_FORTIFY > > > > KBUILD_CFLAGS_DECOMPRESSOR += -fno-delete-null-pointer-checks -msoft-float > > > > KBUILD_CFLAGS_DECOMPRESSOR += -fno-asynchronous-unwind-tables > > > > > > Thanks for the respin with Nathan's suggestion. > > > > > > > +KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning,pointer-sign) > > > > > > What's up with this ^ ? Seems like the top level sets it (without > > > cc-disable-warning :( ), but then KBUILD_CFLAGS_DECOMPRESSOR discards > > > it. Does Clang actually flag code in this arch (that GCC doesn't)? > > > > Oops, that should have been a separate patch. > > > > I think what happens is that clang warns more aggressively about pointer sign > > bugs than gcc in some cases, and some of those cases happen in s390 > > header files that are included by both the kernel and the decompressor. > > > > The full warning log without this change is rather long, see > > https://pastebin.com/KG9xaTNB > > From this link, it looks like the definitions of: > __atomic64_or > __atomic64_and > __atomic64_xor > and their *_barrier variants are problematic. I think converting > those to use unsigned long is the way to go. Shouldn't you be doing > bitwise ops on unsigned types anyways? These functions follow the type of atomic64_t which is a "long" wrapped in a structure. We do not want to change that to unsigned long, are we? Then having some of the functions operate on "long" and others on "unsigned long" seem odd. > The warnings with __atomic64_add are tougher to read/understand since > at that point the log lines look like they start to mix together. > > > > > I also tried patching the code to avoid the warnings, but I'm not entirely > > happy with that result either, see > > https://pastebin.com/pSMz5eZA > > That's no terrible, IMO, particularly with the change I suggest above. That is not too bad, the only change I do not like is the s/u8/char/ in struct ipl_block_fcp. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin.
diff --git a/arch/s390/Makefile b/arch/s390/Makefile index 9c079a506325..443990791099 100644 --- a/arch/s390/Makefile +++ b/arch/s390/Makefile @@ -17,12 +17,13 @@ KBUILD_CFLAGS_MODULE += -fPIC KBUILD_AFLAGS += -m64 KBUILD_CFLAGS += -m64 aflags_dwarf := -Wa,-gdwarf-2 -KBUILD_AFLAGS_DECOMPRESSOR := -m64 -D__ASSEMBLY__ +KBUILD_AFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -D__ASSEMBLY__ KBUILD_AFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),$(aflags_dwarf)) -KBUILD_CFLAGS_DECOMPRESSOR := -m64 -O2 +KBUILD_CFLAGS_DECOMPRESSOR := $(CLANG_FLAGS) -m64 -O2 KBUILD_CFLAGS_DECOMPRESSOR += -DDISABLE_BRANCH_PROFILING -D__NO_FORTIFY KBUILD_CFLAGS_DECOMPRESSOR += -fno-delete-null-pointer-checks -msoft-float KBUILD_CFLAGS_DECOMPRESSOR += -fno-asynchronous-unwind-tables +KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning,pointer-sign) KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-option,-ffreestanding) KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),-g) KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO_DWARF4), $(call cc-option, -gdwarf-4,)) diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile index ce6a3f75065b..ecd0b3847fef 100644 --- a/arch/s390/purgatory/Makefile +++ b/arch/s390/purgatory/Makefile @@ -22,6 +22,7 @@ KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding KBUILD_CFLAGS += -c -MD -Os -m64 -msoft-float -fno-common +KBUILD_CFLAGS += $(CLANG_FLAGS) KBUILD_CFLAGS += $(call cc-option,-fno-PIE) KBUILD_AFLAGS := $(filter-out -DCC_USING_EXPOLINE,$(KBUILD_AFLAGS))
The purgatory and boot Makefiles do not inherit the original cflags, so clang falls back to the default target architecture when building it, typically this would be x86 when cross-compiling. Add $(CLANG_FLAGS) everywhere so we pass the correct --target=s390x-linux option when cross-compiling. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/s390/Makefile | 5 +++-- arch/s390/purgatory/Makefile | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.20.0