Hi all,
We noticed that you all added support for building s390 with clang,
which is great! I have noticed a few warnings for which I will send
patches but this one has me stumped.
In file included from ../lib/crypto/sha256.c:16:
In file included from ../include/linux/module.h:13:
In file included from ../include/linux/stat.h:19:
In file included from ../include/linux/time.h:6:
In file included from ../include/linux/seqlock.h:36:
In file included from ../include/linux/spinlock.h:51:
In file included from ../include/linux/preempt.h:78:
In file included from ../arch/s390/include/asm/preempt.h:6:
In file included from ../include/linux/thread_info.h:38:
In file included from ../arch/s390/include/asm/thread_info.h:26:
../arch/s390/include/asm/page.h:45:6: warning: converting the result of '<<' to a boolean always evaluates to false [-Wtautological-constant-compare]
if (PAGE_DEFAULT_KEY)
^
../arch/s390/include/asm/page.h:23:44: note: expanded from macro 'PAGE_DEFAULT_KEY'
#define PAGE_DEFAULT_KEY (PAGE_DEFAULT_ACC << 4)
^
1 warning generated.
PAGE_DEFAULT_PAGE is always 0, meaning this function never does what it
is supposed to. Is this intentional? It seems that commit 0b642ede4796
("[PATCH] s390: default storage key") added this and it mentions that it
can be overwritten at build time but I do not see any infrastructure for
doing that. Any clarification that you can give so we can solve this
warning would be much appreciated!
Cheers,
Nathan
On 08.02.20 13:57, Nathan Chancellor wrote:
> Hi all,
>
> We noticed that you all added support for building s390 with clang,
> which is great! I have noticed a few warnings for which I will send
> patches but this one has me stumped.
>
> In file included from ../lib/crypto/sha256.c:16:
> In file included from ../include/linux/module.h:13:
> In file included from ../include/linux/stat.h:19:
> In file included from ../include/linux/time.h:6:
> In file included from ../include/linux/seqlock.h:36:
> In file included from ../include/linux/spinlock.h:51:
> In file included from ../include/linux/preempt.h:78:
> In file included from ../arch/s390/include/asm/preempt.h:6:
> In file included from ../include/linux/thread_info.h:38:
> In file included from ../arch/s390/include/asm/thread_info.h:26:
> ../arch/s390/include/asm/page.h:45:6: warning: converting the result of '<<' to a boolean always evaluates to false [-Wtautological-constant-compare]
> if (PAGE_DEFAULT_KEY)
> ^
> ../arch/s390/include/asm/page.h:23:44: note: expanded from macro 'PAGE_DEFAULT_KEY'
> #define PAGE_DEFAULT_KEY (PAGE_DEFAULT_ACC << 4)
> ^
> 1 warning generated.
>
> PAGE_DEFAULT_PAGE is always 0, meaning this function never does what it
> is supposed to. Is this intentional? It seems that commit 0b642ede4796
> ("[PATCH] s390: default storage key") added this and it mentions that it
> can be overwritten at build time but I do not see any infrastructure for
> doing that. Any clarification that you can give so we can solve this
> warning would be much appreciated!
Yes, it is a debugging tool that we use from time to time. The user would then
change PAGE_DEFAULT_ACC in the header file when needed. It was not worth a config
option as normal users should not use it.
On Mon, Feb 10, 2020 at 08:55:46AM +0100, Christian Borntraeger wrote:
>
>
> On 08.02.20 13:57, Nathan Chancellor wrote:
> > Hi all,
> >
> > We noticed that you all added support for building s390 with clang,
> > which is great! I have noticed a few warnings for which I will send
> > patches but this one has me stumped.
> >
> > In file included from ../lib/crypto/sha256.c:16:
> > In file included from ../include/linux/module.h:13:
> > In file included from ../include/linux/stat.h:19:
> > In file included from ../include/linux/time.h:6:
> > In file included from ../include/linux/seqlock.h:36:
> > In file included from ../include/linux/spinlock.h:51:
> > In file included from ../include/linux/preempt.h:78:
> > In file included from ../arch/s390/include/asm/preempt.h:6:
> > In file included from ../include/linux/thread_info.h:38:
> > In file included from ../arch/s390/include/asm/thread_info.h:26:
> > ../arch/s390/include/asm/page.h:45:6: warning: converting the result of '<<' to a boolean always evaluates to false [-Wtautological-constant-compare]
> > if (PAGE_DEFAULT_KEY)
> > ^
> > ../arch/s390/include/asm/page.h:23:44: note: expanded from macro 'PAGE_DEFAULT_KEY'
> > #define PAGE_DEFAULT_KEY (PAGE_DEFAULT_ACC << 4)
> > ^
> > 1 warning generated.
This warning only shows up for the decompressor code and purgatory which
have separate set of build flags not derived from top level KBUILD_CFLAGS.
For the rest of the code this warning is suppressed by:
Makefile:
740 ifdef CONFIG_CC_IS_CLANG
...
744 # Quiet clang warning: comparison of unsigned expression < 0 is always false
745 KBUILD_CFLAGS += -Wno-tautological-compare
At the same time both decompressor and purgatory Makefiles include
CLANG_FLAGS into their CFLAGS. And this -Wno-tautological-compare is
clang specific. So I believe this option belongs to CLANG_FLAGS
rather than being included into KBUILD_CFLAGS under ifdef
CONFIG_CC_IS_CLANG. But this raises question about other clang
specific options inside that ifdef CONFIG_CC_IS_CLANG. Should they all
be made part of CLANG_FLAGS?
> >
> > PAGE_DEFAULT_PAGE is always 0, meaning this function never does what it
> > is supposed to. Is this intentional? It seems that commit 0b642ede4796
> > ("[PATCH] s390: default storage key") added this and it mentions that it
> > can be overwritten at build time but I do not see any infrastructure for
> > doing that. Any clarification that you can give so we can solve this
> > warning would be much appreciated!
>
> Yes, it is a debugging tool that we use from time to time. The user would then
> change PAGE_DEFAULT_ACC in the header file when needed. It was not worth a config
> option as normal users should not use it.
>
On Tue, Feb 11, 2020 at 02:01:29PM +0100, Vasily Gorbik wrote:
> On Mon, Feb 10, 2020 at 08:55:46AM +0100, Christian Borntraeger wrote:
> >
> >
> > On 08.02.20 13:57, Nathan Chancellor wrote:
> > > Hi all,
> > >
> > > We noticed that you all added support for building s390 with clang,
> > > which is great! I have noticed a few warnings for which I will send
> > > patches but this one has me stumped.
> > >
> > > In file included from ../lib/crypto/sha256.c:16:
> > > In file included from ../include/linux/module.h:13:
> > > In file included from ../include/linux/stat.h:19:
> > > In file included from ../include/linux/time.h:6:
> > > In file included from ../include/linux/seqlock.h:36:
> > > In file included from ../include/linux/spinlock.h:51:
> > > In file included from ../include/linux/preempt.h:78:
> > > In file included from ../arch/s390/include/asm/preempt.h:6:
> > > In file included from ../include/linux/thread_info.h:38:
> > > In file included from ../arch/s390/include/asm/thread_info.h:26:
> > > ../arch/s390/include/asm/page.h:45:6: warning: converting the result of '<<' to a boolean always evaluates to false [-Wtautological-constant-compare]
> > > if (PAGE_DEFAULT_KEY)
> > > ^
> > > ../arch/s390/include/asm/page.h:23:44: note: expanded from macro 'PAGE_DEFAULT_KEY'
> > > #define PAGE_DEFAULT_KEY (PAGE_DEFAULT_ACC << 4)
> > > ^
> > > 1 warning generated.
>
> This warning only shows up for the decompressor code and purgatory which
> have separate set of build flags not derived from top level KBUILD_CFLAGS.
> For the rest of the code this warning is suppressed by:
> Makefile:
> 740 ifdef CONFIG_CC_IS_CLANG
> ...
> 744 # Quiet clang warning: comparison of unsigned expression < 0 is always false
> 745 KBUILD_CFLAGS += -Wno-tautological-compare
>
> At the same time both decompressor and purgatory Makefiles include
> CLANG_FLAGS into their CFLAGS. And this -Wno-tautological-compare is
> clang specific. So I believe this option belongs to CLANG_FLAGS
> rather than being included into KBUILD_CFLAGS under ifdef
> CONFIG_CC_IS_CLANG. But this raises question about other clang
> specific options inside that ifdef CONFIG_CC_IS_CLANG. Should they all
> be made part of CLANG_FLAGS?
Hi Vasily,
I am trying to turn on -Wtautological-compare for the kernel as a whole,
hence me trying to deal with this one now :) That flag controls a bunch
of useful subwarnings that can point out potentially problematic code.
I think that it would be worth adding warnings that we want disabled in
all code to CLANG_FLAGS but as of right now, this is the only instance
of this warning that I see within the s390 code so it is probably just
worth silencing with an explicit comparison (!= 0). I will send a patch
for this later.
Cheers,
Nathan