[2/2] s390: boot, purgatory: pass $(CLANG_FLAGS) where needed

Message ID 20190410201300.3528984-2-arnd@arndb.de
State New
Headers show
Series
  • [1/2] s390: only build for new CPUs with clang
Related show

Commit Message

Arnd Bergmann April 10, 2019, 8:12 p.m.
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

Comments

Nick Desaulniers April 10, 2019, 10:14 p.m. | #1
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
Arnd Bergmann April 11, 2019, 8:52 a.m. | #2
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
Nick Desaulniers April 11, 2019, 6:08 p.m. | #3
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
Martin Schwidefsky April 15, 2019, 6:32 a.m. | #4
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.

Patch

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))