From: Joel Stanley <joel@jms.id.au> Date: 2018-09-14 04:07:04
v2 rolls up all of the fixes I have into the one series, and adds a few
more fixes.
With a one patch[1] on top of clang master I can build and boot a
powernv kernel. Note that we can't build altivec code using clang
yet[2], hence disabling MD_RAID456.
$ make ARCH=powerpc powernv_defconfig
$ ./scripts/config -d BTRFS_FS -d MD_RAID456
$ make CC=clang-8 CLANG_TRIPLE=powerpc64le-linux-gnu -j128
The build is very noisy as the ftrace recordmcount build step is not
picking up -Qunused-arguments.
$ qemu-system-ppc64 -M powernv -m 3G -nographic -kernel zImage.epapr \
-L ~/skiboot/ -initrd ~/rootfs.cpio.xz
Linux version 4.19.0-rc3-00005-g7f51dcdecc6b (joel@ozrom3) (clang version 8.0.0 (trunk 342199)) #5 SMP Fri Sep 14 13:28:57 ACST 2018
I have discovered Nick D's clangBuiltLinux issue tracker[3] and will be
updating that as we find and fix issues.
[1] https://reviews.llvm.org/D50965
[2] https://bugs.llvm.org/show_bug.cgi?id=38887
[3] https://github.com/clangBuiltLinux/linux/issues
Anton Blanchard (2):
powerpc/Makefiles: Fix clang/llvm build
powerpc: Fix duplicate const clang warning in user access code
Joel Stanley (3):
powerpc/boot: Fix crt0.S syntax for clang
powerpc/boot: Ensure _zimage_start is a weak symbol
powerpc: Remove -mno-sched-epilog
arch/powerpc/Makefile | 7 +------
arch/powerpc/boot/crt0.S | 8 +++++---
arch/powerpc/include/asm/uaccess.h | 6 +++---
arch/powerpc/kernel/Makefile | 8 ++++----
arch/powerpc/kernel/trace/Makefile | 2 +-
arch/powerpc/platforms/powermac/Makefile | 2 +-
arch/powerpc/xmon/Makefile | 2 +-
7 files changed, 16 insertions(+), 19 deletions(-)
--
2.17.1
From: Joel Stanley <joel@jms.id.au> Date: 2018-09-14 04:07:14
From: Anton Blanchard <redacted>
Commit 15a3204d24a3 ("powerpc/64s: Set assembler machine type to POWER4")
passes -mpower4 to the assembler. We have more recent instructions in our
assembly files, but gas permits them. The clang/llvm integrated assembler
is more strict, and we get a build failure.
Fix this by calling the assembler with -mcpu=power8 if as supports it,
else fall back to power4.
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Anton Blanchard <redacted>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Joel Stanley <joel@jms.id.au> Date: 2018-09-14 04:07:19
Clang's assembler does not like the syntax of the cmpdi:
arch/powerpc/boot/crt0.S:168:22: error: unexpected modifier on variable reference
cmpdi 12,RELACOUNT@l
^
arch/powerpc/boot/crt0.S:168:11: error: unknown operand
cmpdi 12,RELACOUNT@l
^
Enclosing RELACOUNT in () makes it is happy. Tested with GCC 8 and Clang
8 (trunk).
Reported to clang as https://bugs.llvm.org/show_bug.cgi?id=38945
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Fix for !powerpc64 too, add bug link to commit message
---
arch/powerpc/boot/crt0.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Joel Stanley <joel@jms.id.au> Date: 2018-09-14 04:07:26
When building with clang crt0's _zimage_start is not marked weak, which
breaks the build when linking the kernel image:
$ objdump -t arch/powerpc/boot/crt0.o |grep _zimage_start$
0000000000000058 g .text 0000000000000000 _zimage_start
ld: arch/powerpc/boot/wrapper.a(crt0.o): in function '_zimage_start':
(.text+0x58): multiple definition of '_zimage_start';
arch/powerpc/boot/pseries-head.o:(.text+0x0): first defined here
Clang requires the .weak directive to appear after the symbol is
declared. The binutils manual says:
This directive sets the weak attribute on the comma separated list of
symbol names. If the symbols do not already exist, they will be
created.
So it appears this is different with clang. The only reference I could
see for this was an OpenBSD mailing list post[1].
Changing it to be after the declaration fixes building with Clang, and
still works with GCC.
$ objdump -t arch/powerpc/boot/crt0.o |grep _zimage_start$
0000000000000058 w .text 0000000000000000 _zimage_start
Reported to clang as https://bugs.llvm.org/show_bug.cgi?id=38921
[1] https://groups.google.com/forum/#!topic/fa.openbsd.tech/PAgKKen2YCY
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Add comment explaining this is a workaround for clang. I have also
opened a clang bug to see if they plan on changing behaviour
---
arch/powerpc/boot/crt0.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Joel Stanley <joel@jms.id.au> Date: 2018-09-14 04:07:33
From: Anton Blanchard <redacted>
This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
...").
We see a large number of duplicate const errors in the user access
code when building with llvm/clang:
include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
[-Wduplicate-decl-specifier]
ret = __get_user(c, uaddr);
The problem is we are doing const __typeof__(*(ptr)), which will hit the
warning if ptr is marked const.
Removing const does not seem to have any effect on GCC code generation.
Signed-off-by: Anton Blanchard <redacted>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
If we don't want to apply this, other options are suppressing the
warning, or wait for a fix to land in clang
(https://github.com/ClangBuiltLinux/linux/issues/52).
---
arch/powerpc/include/asm/uaccess.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Joel Stanley <joel@jms.id.au> Date: 2018-09-14 04:07:40
This effectively reverts 7563dc645853 ("powerpc: Work around gcc's
-fno-omit-frame-pointer bug"), a workaround for a bug in GCC 4.1.3 when
building 2.6.26 kernel.
The flag is not supported by clang, but reading the history of the
bug[1] suggests it is no longer required by supported GCC versions, with
GCC 4.6 now being the minimum.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=11414
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
Last time this was proposed there was an issue reported:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-September/121214.html
If some machines still have an issue, we could instead wrap this in a
flag check.
---
arch/powerpc/Makefile | 5 -----
arch/powerpc/kernel/Makefile | 8 ++++----
arch/powerpc/kernel/trace/Makefile | 2 +-
arch/powerpc/platforms/powermac/Makefile | 2 +-
arch/powerpc/xmon/Makefile | 2 +-
5 files changed, 7 insertions(+), 12 deletions(-)
@@ -229,11 +229,6 @@ ifdef CONFIG_6xxKBUILD_CFLAGS+=-mcpu=powerpcendif-# Work around a gcc code-gen bug with -fno-omit-frame-pointer.-ifdef CONFIG_FUNCTION_TRACER-KBUILD_CFLAGS+=-mno-sched-epilog-endif-cpu-as-$(CONFIG_4xx)+=-Wa,-m405cpu-as-$(CONFIG_ALTIVEC)+=$(callas-option,-Wa$(comma)-maltivec)cpu-as-$(CONFIG_E200)+=-Wa,-me200
@@ -22,10 +22,10 @@ CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)ifdef CONFIG_FUNCTION_TRACER# Do not trace early boot code-CFLAGS_REMOVE_cputable.o=-mno-sched-epilog$(CC_FLAGS_FTRACE)-CFLAGS_REMOVE_prom_init.o=-mno-sched-epilog$(CC_FLAGS_FTRACE)-CFLAGS_REMOVE_btext.o=-mno-sched-epilog$(CC_FLAGS_FTRACE)-CFLAGS_REMOVE_prom.o=-mno-sched-epilog$(CC_FLAGS_FTRACE)+CFLAGS_REMOVE_cputable.o=$(CC_FLAGS_FTRACE)+CFLAGS_REMOVE_prom_init.o=$(CC_FLAGS_FTRACE)+CFLAGS_REMOVE_btext.o=$(CC_FLAGS_FTRACE)+CFLAGS_REMOVE_prom.o=$(CC_FLAGS_FTRACE)endifobj-y:=cputable.optrace.osyscalls.o\
@@ -3,7 +3,7 @@ CFLAGS_bootx_init.o += -fPICifdef CONFIG_FUNCTION_TRACER# Do not trace early boot code-CFLAGS_REMOVE_bootx_init.o=-mno-sched-epilog$(CC_FLAGS_FTRACE)+CFLAGS_REMOVE_bootx_init.o=$(CC_FLAGS_FTRACE)endifobj-y+=pic.osetup.otime.ofeature.opci.o\
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-09-14 05:06:36
On Fri, 14 Sep 2018 13:36:49 +0930
Joel Stanley [off-list ref] wrote:
This effectively reverts 7563dc645853 ("powerpc: Work around gcc's
-fno-omit-frame-pointer bug"), a workaround for a bug in GCC 4.1.3 when
building 2.6.26 kernel.
The flag is not supported by clang, but reading the history of the
bug[1] suggests it is no longer required by supported GCC versions, with
GCC 4.6 now being the minimum.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=11414
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
Last time this was proposed there was an issue reported:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-September/121214.html
If some machines still have an issue, we could instead wrap this in a
flag check.
I don't think we can remove it completely because up to at least 4.6
maybe 4.8 has problems.
I have a few patches lying around I started looking at this... I'll
send them.
Thanks,
Nick
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2018-09-14 17:41:56
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
From: Anton Blanchard <redacted>
Commit 15a3204d24a3 ("powerpc/64s: Set assembler machine type to POWER4")
passes -mpower4 to the assembler. We have more recent instructions in our
assembly files, but gas permits them. The clang/llvm integrated assembler
is more strict, and we get a build failure.
Note that we disable clang's integrated assembler in the top level
Makefile for now, but it will still validate constraints for inline
assembly. Do you know which case is meant by "build failure?"
Is there a link to the Clang bug? It would be good to have that
context in the commit message.
quoted hunk
Fix this by calling the assembler with -mcpu=power8 if as supports it,
else fall back to power4.
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Anton Blanchard <redacted>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2018-09-14 17:47:22
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
quoted hunk
Clang's assembler does not like the syntax of the cmpdi:
arch/powerpc/boot/crt0.S:168:22: error: unexpected modifier on variable reference
cmpdi 12,RELACOUNT@l
^
arch/powerpc/boot/crt0.S:168:11: error: unknown operand
cmpdi 12,RELACOUNT@l
^
Enclosing RELACOUNT in () makes it is happy. Tested with GCC 8 and Clang
8 (trunk).
Reported to clang as https://bugs.llvm.org/show_bug.cgi?id=38945
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Fix for !powerpc64 too, add bug link to commit message
---
arch/powerpc/boot/crt0.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -80,7 +80,7 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */lwzr9,4(r12)/*getRELApointerinr9*/b12f11:addisr8,r8,(-RELACOUNT)@ha-cmpwir8,RELACOUNT@l+cmpwir8,(RELACOUNT)@lbne12flwzr0,4(r12)/*getRELACOUNTvalueinr0*/12:addir12,r12,8
@@ -165,7 +165,7 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */ldr13,8(r11)/*getRELApointerinr13*/b11f10:addisr12,r12,(-RELACOUNT)@ha-cmpdir12,RELACOUNT@l+cmpdir12,(RELACOUNT)@l
Yep, as we can see above, when RELACOUNT is negated, it's wrapped in
parens. It's important for Clang's assembler to match GAS eventually,
but for now, this change simply cononicalizes all of the the
references to RELACOUNT in this source file.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
bne 11f
ld r8,8(r11) /* get RELACOUNT value in r8 */
11: addi r11,r11,16
--
2.17.1
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2018-09-14 17:51:05
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
quoted hunk
When building with clang crt0's _zimage_start is not marked weak, which
breaks the build when linking the kernel image:
$ objdump -t arch/powerpc/boot/crt0.o |grep _zimage_start$
0000000000000058 g .text 0000000000000000 _zimage_start
ld: arch/powerpc/boot/wrapper.a(crt0.o): in function '_zimage_start':
(.text+0x58): multiple definition of '_zimage_start';
arch/powerpc/boot/pseries-head.o:(.text+0x0): first defined here
Clang requires the .weak directive to appear after the symbol is
declared. The binutils manual says:
This directive sets the weak attribute on the comma separated list of
symbol names. If the symbols do not already exist, they will be
created.
So it appears this is different with clang. The only reference I could
see for this was an OpenBSD mailing list post[1].
Changing it to be after the declaration fixes building with Clang, and
still works with GCC.
$ objdump -t arch/powerpc/boot/crt0.o |grep _zimage_start$
0000000000000058 w .text 0000000000000000 _zimage_start
Reported to clang as https://bugs.llvm.org/show_bug.cgi?id=38921
[1] https://groups.google.com/forum/#!topic/fa.openbsd.tech/PAgKKen2YCY
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Add comment explaining this is a workaround for clang. I have also
opened a clang bug to see if they plan on changing behaviour
---
arch/powerpc/boot/crt0.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Sure, definitely a long tail of binutils/GAS functionality to match
from Clang's integrated assembler, but this is a small fix that
results in no functional change for GCC/binutils and enables
Clang/bfd.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
--
Thanks,
~Nick Desaulniers
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2018-09-14 17:57:21
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
From: Anton Blanchard <redacted>
This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
...").
We see a large number of duplicate const errors in the user access
code when building with llvm/clang:
include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
[-Wduplicate-decl-specifier]
ret = __get_user(c, uaddr);
The problem is we are doing const __typeof__(*(ptr)), which will hit the
warning if ptr is marked const.
Removing const does not seem to have any effect on GCC code generation.
I wouldn't expect it to for a local variable with such localized
usage. I myself am quite liberal in applying `const` to everything,
so I will try to fix this in Clang as well, but this should silence
the warning for users of older versions of Clang and results in no
functional change.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
quoted hunk
Signed-off-by: Anton Blanchard <redacted>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
If we don't want to apply this, other options are suppressing the
warning, or wait for a fix to land in clang
(https://github.com/ClangBuiltLinux/linux/issues/52).
---
arch/powerpc/include/asm/uaccess.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Heh, did PASemi sell boxes? Interesting, I'll have to read up on my history.
On Thu, Sep 13, 2018 at 10:06 PM Nicholas Piggin [off-list ref] wrote:
I don't think we can remove it completely because up to at least 4.6
maybe 4.8 has problems.
I have a few patches lying around I started looking at this... I'll
send them.
Yeah, it's too bad the link above doesn't mention gcc version.
The gcc bugreport mentions fixing the bug in
7563dc64585324f443f5ac107eb6d89ee813a2d2, not sure how to check what
release version of gcc that is? (Do they tag releases?)
Nick, do you have a test case or more context about this still being
an issue in gcc 4.8? (maybe I should wait for your patch series?)
--
Thanks,
~Nick Desaulniers
Heh, did PASemi sell boxes? Interesting, I'll have to read up on my history.
On Thu, Sep 13, 2018 at 10:06 PM Nicholas Piggin [off-list ref] wrote:
quoted
I don't think we can remove it completely because up to at least 4.6
maybe 4.8 has problems.
I have a few patches lying around I started looking at this... I'll
send them.
Yeah, it's too bad the link above doesn't mention gcc version.
The gcc bugreport mentions fixing the bug in
7563dc64585324f443f5ac107eb6d89ee813a2d2, not sure how to check what
release version of gcc that is? (Do they tag releases?)
I'm not sure, that's not in my gcc tree AFAIKS.
Nick, do you have a test case or more context about this still being
an issue in gcc 4.8? (maybe I should wait for your patch series?)
Heh, did PASemi sell boxes? Interesting, I'll have to read up on my history.
On Thu, Sep 13, 2018 at 10:06 PM Nicholas Piggin [off-list ref] wrote:
quoted
I don't think we can remove it completely because up to at least 4.6
maybe 4.8 has problems.
I have a few patches lying around I started looking at this... I'll
send them.
Yeah, it's too bad the link above doesn't mention gcc version.
The gcc bugreport mentions fixing the bug in
7563dc64585324f443f5ac107eb6d89ee813a2d2, not sure how to check what
release version of gcc that is? (Do they tag releases?)
I'm not sure, that's not in my gcc tree AFAIKS.
This is a git hash in the kernel tree.
quoted
Nick, do you have a test case or more context about this still being
an issue in gcc 4.8? (maybe I should wait for your patch series?)
Heh, did PASemi sell boxes? Interesting, I'll have to read up on my history.
On Thu, Sep 13, 2018 at 10:06 PM Nicholas Piggin [off-list ref] wrote:
quoted
I don't think we can remove it completely because up to at least 4.6
maybe 4.8 has problems.
I have a few patches lying around I started looking at this... I'll
send them.
Yeah, it's too bad the link above doesn't mention gcc version.
The gcc bugreport mentions fixing the bug in
7563dc64585324f443f5ac107eb6d89ee813a2d2, not sure how to check what
release version of gcc that is? (Do they tag releases?)
I'm not sure, that's not in my gcc tree AFAIKS.
This is a git hash in the kernel tree.
quoted
quoted
Nick, do you have a test case or more context about this still being
an issue in gcc 4.8? (maybe I should wait for your patch series?)
Cool, cc me on the thread if you'd like me to add my reviewed-by tag
visibly on the thread.
quoted
PR44199 was backported to 4.4 and PR52828 is fixed in 4.8.
Are those GCC versions? If so, what does that mean for the users of
the many GCC releases between 4.4 and 4.8?
Yes, GCC bug 44199 was fixed in GCC version 4.4, etc. It of course also
is fixed in all later versions; so GCC releases between 4.4 and 4.8 have
the fix for PR44199 but not that for PR52828.
I didn't check exactly what 4.4.x versions have the fix, etc. I always
assume anyone using x.y.z uses the highest z available.
I don't know if those are the only fixes you need; those are the two
bugs mentioned in Nicholas' patch (that MARC link above).
Segher
From: Joel Stanley <joel@jms.id.au> Date: 2018-09-19 07:46:00
On Sat, 15 Sep 2018 at 03:27, Nick Desaulniers [off-list ref] wrote:
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
quoted
From: Anton Blanchard <redacted>
This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
...").
We see a large number of duplicate const errors in the user access
code when building with llvm/clang:
include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
[-Wduplicate-decl-specifier]
ret = __get_user(c, uaddr);
The problem is we are doing const __typeof__(*(ptr)), which will hit the
warning if ptr is marked const.
Removing const does not seem to have any effect on GCC code generation.
I wouldn't expect it to for a local variable with such localized
usage. I myself am quite liberal in applying `const` to everything,
so I will try to fix this in Clang as well, but this should silence
the warning for users of older versions of Clang and results in no
functional change.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Nick has written a clang patch that suppresses the warning in the same
way as GCC. Assuming it gets merged, as we depend on clang-8 we could
chose to not merge the kernel patch.
https://reviews.llvm.org/D52248
Cheers,
Joel
From: Michael Ellerman <hidden> Date: 2018-09-20 04:21:04
On Fri, 2018-09-14 at 04:06:47 UTC, Joel Stanley wrote:
When building with clang crt0's _zimage_start is not marked weak, which
breaks the build when linking the kernel image:
$ objdump -t arch/powerpc/boot/crt0.o |grep _zimage_start$
0000000000000058 g .text 0000000000000000 _zimage_start
ld: arch/powerpc/boot/wrapper.a(crt0.o): in function '_zimage_start':
(.text+0x58): multiple definition of '_zimage_start';
arch/powerpc/boot/pseries-head.o:(.text+0x0): first defined here
Clang requires the .weak directive to appear after the symbol is
declared. The binutils manual says:
This directive sets the weak attribute on the comma separated list of
symbol names. If the symbols do not already exist, they will be
created.
So it appears this is different with clang. The only reference I could
see for this was an OpenBSD mailing list post[1].
Changing it to be after the declaration fixes building with Clang, and
still works with GCC.
$ objdump -t arch/powerpc/boot/crt0.o |grep _zimage_start$
0000000000000058 w .text 0000000000000000 _zimage_start
Reported to clang as https://bugs.llvm.org/show_bug.cgi?id=38921
[1] https://groups.google.com/forum/#!topic/fa.openbsd.tech/PAgKKen2YCY
Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
From: Michael Ellerman <hidden> Date: 2018-09-20 04:21:06
On Fri, 2018-09-14 at 04:06:48 UTC, Joel Stanley wrote:
From: Anton Blanchard <redacted>
This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
...").
We see a large number of duplicate const errors in the user access
code when building with llvm/clang:
include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
[-Wduplicate-decl-specifier]
ret = __get_user(c, uaddr);
The problem is we are doing const __typeof__(*(ptr)), which will hit the
warning if ptr is marked const.
Removing const does not seem to have any effect on GCC code generation.
Signed-off-by: Anton Blanchard <redacted>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
On Sat, 15 Sep 2018 at 03:27, Nick Desaulniers [off-list ref] wrote:
quoted
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
quoted
From: Anton Blanchard <redacted>
This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
...").
We see a large number of duplicate const errors in the user access
code when building with llvm/clang:
include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
[-Wduplicate-decl-specifier]
ret = __get_user(c, uaddr);
The problem is we are doing const __typeof__(*(ptr)), which will hit the
warning if ptr is marked const.
Removing const does not seem to have any effect on GCC code generation.
I wouldn't expect it to for a local variable with such localized
usage. I myself am quite liberal in applying `const` to everything,
so I will try to fix this in Clang as well, but this should silence
the warning for users of older versions of Clang and results in no
functional change.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Nick has written a clang patch that suppresses the warning in the same
way as GCC. Assuming it gets merged, as we depend on clang-8 we could
chose to not merge the kernel patch.
https://reviews.llvm.org/D52248
Seems like Michael has merged this patch anyway.
Christophe
From: Joel Stanley <joel@jms.id.au> Date: 2018-09-20 07:04:38
On Sat, 15 Sep 2018 at 03:11, Nick Desaulniers [off-list ref] wrote:
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
quoted
From: Anton Blanchard <redacted>
Commit 15a3204d24a3 ("powerpc/64s: Set assembler machine type to POWER4")
passes -mpower4 to the assembler. We have more recent instructions in our
assembly files, but gas permits them. The clang/llvm integrated assembler
is more strict, and we get a build failure.
Note that we disable clang's integrated assembler in the top level
Makefile for now, but it will still validate constraints for inline
assembly. Do you know which case is meant by "build failure?"
An example of a failure is:
CC init/do_mounts.o
/tmp/do_mounts-58cd1d.s: Assembler messages:
/tmp/do_mounts-58cd1d.s:251: Error: unrecognized opcode: `isel'
/tmp/do_mounts-58cd1d.s:748: Error: unrecognized opcode: `isel'
/tmp/do_mounts-58cd1d.s:759: Error: unrecognized opcode: `isel'
/tmp/do_mounts-58cd1d.s:1373: Error: unrecognized opcode: `isel'
The full build line at the bottom of this email. The m flags:
-mlittle-endian -m64 -msoft-float -mabi=elfv2 -mcmodel=medium
-mcpu=power8 -mtune=power9 -mno-altivec -mno-vsx -Wa,-maltivec
-Wa,-mpower4 -mlittle-endian
If we remove the -Wa,-mpower4 it works. This is because isel is not
available on power4.
Digging deeper, clang drives gas like this:
"/usr/bin/as" -a64 -mppc64 -mlittle-endian -mpower8 -I
./arch/powerpc/include -I ./arch/powerpc/include/generated -I
./include -I ./arch/powerpc/include/uapi -I
./arch/powerpc/include/generated/uapi -I ./include/uapi -I
./include/generated/uapi -I arch/powerpc -I arch/powerpc -maltivec
-mpower4 -o init/do_mounts.o /tmp/do_mounts-3b0a3d.s
GCC drives it:
as -v -I ./arch/powerpc/include -I ./arch/powerpc/include/generated -I
./include -I ./arch/powerpc/include/uapi -I
./arch/powerpc/include/generated/uapi -I ./include/uapi -I
./include/generated/uapi -I arch/powerpc -I arch/powerpc -a64 -mpower8
-many -mlittle -maltivec -mpower4 -o init/do_mounts.o
I think the important difference is GCC adds -many:
-many
Generate code for any architecture (PWR/PWRX/PPC).
This is probably what we mean when building the kernel. I will get
some advice from our toolchain people first to understand what -many
does.
Is there a link to the Clang bug? It would be good to have that
context in the commit message.
I haven't opened a clang bug. I'm not sure that we need one, unless we
want it to add -many to the assembler call like gcc does.
Cheers,
Joel
--
/scratch/joel/llvm-build/bin/clang-8 -Wp,-MD,init/.do_mounts.o.d
-nostdinc -isystem /scratch/joel/llvm-build/lib/clang/8.0.0/include
-I./arch/powerpc/include -I./arch/powerpc/include/generated
-I./include -I./arch/powerpc/include/uapi
-I./arch/powerpc/include/generated/uapi -I./include/uapi
-I./include/generated/uapi -include ./include/linux/kconfig.h -include
./include/linux/compiler_types.h -D__KERNEL__ -Iarch/powerpc
-DHAVE_AS_ATHIGH=1 -Qunused-arguments -Wall -Wundef
-Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common
-fshort-wchar -Werror-implicit-function-declaration
-Wno-format-security -std=gnu89 -no-integrated-as -fno-PIE
-mlittle-endian -m64 -msoft-float -pipe -Iarch/powerpc -mabi=elfv2
-mcmodel=medium -mcpu=power8 -mtune=power9 -mno-altivec -mno-vsx
-funit-at-a-time -fno-dwarf2-cfi-asm -Wa,-maltivec -Wa,-mpower4
-mlittle-endian -fno-delete-null-pointer-checks -Oz
-Wframe-larger-than=2048 -fno-stack-protector
-Wno-format-invalid-specifier -Wno-gnu -Wno-address-of-packed-member
-Wno-tautological-compare -mno-global-merge -Wno-unused-const-variable
-fomit-frame-pointer -Wdeclaration-after-statement -Wno-pointer-sign
-fno-strict-overflow -fno-merge-all-constants -fno-stack-check
-Werror=implicit-int -Werror=strict-prototypes -Werror=date-time
-Werror=incompatible-pointer-types -Wno-initializer-overrides
-Wno-unused-value -Wno-format -Wno-sign-compare
-Wno-format-zero-length -Wno-uninitialized -fno-function-sections
-fno-data-sections -DKBUILD_BASENAME='"do_mounts"'
-DKBUILD_MODNAME='"mounts"' -c -o init/do_mounts.o init/do_mounts.c
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-09-20 12:35:47
Christophe LEROY [off-list ref] writes:
Le 19/09/2018 =C3=A0 09:45, Joel Stanley a =C3=A9crit=C2=A0:
quoted
On Sat, 15 Sep 2018 at 03:27, Nick Desaulniers [off-list ref]=
wrote:
quoted
quoted
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley [off-list ref] wrote:
quoted
From: Anton Blanchard <redacted>
This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
...").
We see a large number of duplicate const errors in the user access
code when building with llvm/clang:
include/linux/pagemap.h:576:8: warning: duplicate 'const' declarati=
on specifier
quoted
quoted
quoted
[-Wduplicate-decl-specifier]
ret =3D __get_user(c, uaddr);
The problem is we are doing const __typeof__(*(ptr)), which will hit t=
he
quoted
quoted
quoted
warning if ptr is marked const.
Removing const does not seem to have any effect on GCC code generation.
I wouldn't expect it to for a local variable with such localized
usage. I myself am quite liberal in applying `const` to everything,
so I will try to fix this in Clang as well, but this should silence
the warning for users of older versions of Clang and results in no
functional change.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
=20
Nick has written a clang patch that suppresses the warning in the same
way as GCC. Assuming it gets merged, as we depend on clang-8 we could
chose to not merge the kernel patch.
=20
https://reviews.llvm.org/D52248
Seems like Michael has merged this patch anyway.
I'll happily revert it though if someone tells me to.
cheers