[U-Boot] [PATCH 0/3] Fixes for FreeBSD / clang
- Some minor adjustments to the new compiler.h from linux. - Updated README for FreeBSD
Jeroen Hofstee (3): compiler_gcc: do not redefine __gnu_attributes README.clang: update FreeBSD instructions compiler.h: remove duplicated uninitialized_var
doc/README.clang | 6 +++--- include/compiler.h | 3 --- include/linux/compiler-gcc.h | 10 ++++++++++ 3 files changed, 13 insertions(+), 6 deletions(-)
commit fb8ffd7cfc68b3dc44e182356a207d784cb30b34 "compiler*.h: sync include/linux/compiler*.h with Linux 3.16" undid the changes of 7ea50d52849fe8ffa5b5b74c979b60b1045d6fc9 "compiler_gcc: do not redefine __gnu_attributes". Add the checks back whether these macro's are already defined (as it causes a lot of noise on e.g. FreeBSD where these defines are already in cdefs.h)
As the original patch this checkpatch warning is ignored: "WARNING: Adding new packed members is to be done with care"
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl --- include/linux/compiler-gcc.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 02ae99e..e057bd2 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -64,8 +64,12 @@ #endif
#define __deprecated __attribute__((deprecated)) +#ifndef __packed #define __packed __attribute__((packed)) +#endif +#ifndef __weak #define __weak __attribute__((weak)) +#endif
/* * it doesn't make sense on ARM (currently the only user of __naked) to trace @@ -91,8 +95,12 @@ * would be. * [...] */ +#ifndef __pure #define __pure __attribute__((pure)) +#endif +#ifndef __aligned #define __aligned(x) __attribute__((aligned(x))) +#endif #define __printf(a, b) __attribute__((format(printf, a, b))) #define __scanf(a, b) __attribute__((format(scanf, a, b))) #define noinline __attribute__((noinline)) @@ -115,4 +123,6 @@ */ #define uninitialized_var(x) x = x
+#ifndef __always_inline #define __always_inline inline __attribute__((always_inline)) +#endif
Jeroen,
commit fb8ffd7cfc68b3dc44e182356a207d784cb30b34 "compiler*.h: sync include/linux/compiler*.h with Linux 3.16" undid the changes of 7ea50d52849fe8ffa5b5b74c979b60b1045d6fc9 "compiler_gcc: do not redefine __gnu_attributes". Add the checks back whether these macro's are already defined (as it causes a lot of noise on e.g. FreeBSD where these defines are already in cdefs.h)
As the original patch this checkpatch warning is ignored: "WARNING: Adding new packed members is to be done with care"
Strange.
Which source files include cdefs.h?
For building u-boot images, sources should only include headers in the u-boot source tree. The standard system headers should not be used except only a few files such as <stdarg.h>.
This is the same as Linux Kernel.
On the contrary, host programs are allowed to use standard system headers such as <stdio.h>, <stdlib.h> etc, where <linux/compiler.h> should not be included.
The root cause of warnings is _not_ that __packed and __weak are always defined in compiler-gcc.h.
I believe the real problem is there are some source files include both system headers and <linux/compiler.h> at the same time.
Best Regards Masahiro Yamada
Hello Masahiro,
On 18-09-14 04:13, Masahiro Yamada wrote:
Jeroen,
commit fb8ffd7cfc68b3dc44e182356a207d784cb30b34 "compiler*.h: sync include/linux/compiler*.h with Linux 3.16" undid the changes of 7ea50d52849fe8ffa5b5b74c979b60b1045d6fc9 "compiler_gcc: do not redefine __gnu_attributes". Add the checks back whether these macro's are already defined (as it causes a lot of noise on e.g. FreeBSD where these defines are already in cdefs.h)
As the original patch this checkpatch warning is ignored: "WARNING: Adding new packed members is to be done with care"
Strange.
Which source files include cdefs.h?
The host std* include this file, not a source file.
For building u-boot images, sources should only include headers in the u-boot source tree. The standard system headers should not be used except only a few files such as <stdarg.h>.
This is the same as Linux Kernel.
Yes, and stdarg.h includes cdefs.h. So each include of common.h causes several warnings.
On the contrary, host programs are allowed to use standard system headers such as <stdio.h>, <stdlib.h> etc, where <linux/compiler.h> should not be included.
The root cause of warnings is _not_ that __packed and __weak are always defined in compiler-gcc.h.
This only works properly it u-boot for the target does not depend on any host header at all, which as you mentioned is not the case. Hence we shouldn't be surprised if they define something in their namespace and hence checking if that is done is fine.
I believe the real problem is there are some source files include both system headers and <linux/compiler.h> at the same time.
No it is a header issue only. The only alternative I can think of is a custom version of stdarg etc and I don't really like that idea.
Regards, Jeroen
Hi Jeroen,
On Thu, 18 Sep 2014 11:15:25 +0200 Jeroen Hofstee jeroen@myspectrum.nl wrote:
Hello Masahiro,
On 18-09-14 04:13, Masahiro Yamada wrote:
Jeroen,
commit fb8ffd7cfc68b3dc44e182356a207d784cb30b34 "compiler*.h: sync include/linux/compiler*.h with Linux 3.16" undid the changes of 7ea50d52849fe8ffa5b5b74c979b60b1045d6fc9 "compiler_gcc: do not redefine __gnu_attributes". Add the checks back whether these macro's are already defined (as it causes a lot of noise on e.g. FreeBSD where these defines are already in cdefs.h)
As the original patch this checkpatch warning is ignored: "WARNING: Adding new packed members is to be done with care"
Strange.
Which source files include cdefs.h?
The host std* include this file, not a source file.
For building u-boot images, sources should only include headers in the u-boot source tree. The standard system headers should not be used except only a few files such as <stdarg.h>.
This is the same as Linux Kernel.
Yes, and stdarg.h includes cdefs.h. So each include of common.h causes several warnings.
Oh, my dear! This is really unfortunate.
On the contrary, host programs are allowed to use standard system headers such as <stdio.h>, <stdlib.h> etc, where <linux/compiler.h> should not be included.
The root cause of warnings is _not_ that __packed and __weak are always defined in compiler-gcc.h.
This only works properly it u-boot for the target does not depend on any host header at all, which as you mentioned is not the case. Hence we shouldn't be surprised if they define something in their namespace and hence checking if that is done is fine.
I believe the real problem is there are some source files include both system headers and <linux/compiler.h> at the same time.
No it is a header issue only. The only alternative I can think of is a custom version of stdarg etc and I don't really like that idea.
Me neither.
Thanks for explaining this and fully understood.
Acked-by: Masahiro Yamada yamada.m@jp.panasonic.com
(Ideally, I hope a little more comments because I did not know <stdarg.h> on FreeBSD defines __packed, __weak etc.)
Best Regards Masahiro Yamada
The mentioned binutils port got removed while the patch was pending. As Ian pointed out there is another port providing the binutils for arm now. Update the instructions accordingly.
Cc: ian@FreeBSD.org Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl --- doc/README.clang | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/doc/README.clang b/doc/README.clang index 9ad689f..8c5bf12 100644 --- a/doc/README.clang +++ b/doc/README.clang @@ -34,14 +34,14 @@ make HOSTCC=clang CC="clang -target $TRIPLET -mllvm -arm-use-movt=0 -no-integrat FreeBSD 11 (Current): -------------------- Since llvm 3.4 is currently in the base system, the integrated as is -incapable of building U-Boot. Therefore gas from devel/arm-eabi-binutils +incapable of building U-Boot. Therefore gas from devel/arm-gnueabi-binutils is used instead. It needs a symlinks to be picked up correctly though:
-ln -s /usr/local/bin/arm-eabi-as /usr/bin/arm-freebsd-eabi-as +ln -s /usr/local/bin/arm-gnueabi-freebsd-as /usr/bin/arm-freebsd-eabi-as
# The following commands compile U-Boot using the clang xdev toolchain. # NOTE: CROSS_COMPILE and target differ on purpose! -export CROSS_COMPILE=arm-eabi- +export CROSS_COMPILE=arm-gnueabi-freebsd- gmake CC="clang -target arm-freebsd-eabi --sysroot /usr/arm-freebsd -no-integrated-as -mllvm -arm-use-movt=0" rpi_b_defconfig gmake CC="clang -target arm-freebsd-eabi --sysroot /usr/arm-freebsd -no-integrated-as -mllvm -arm-use-movt=0" -j8
Since clang has a different definition for uninitialized_var it will complain that it is redefined in include/compiler.h. Since these are already defined in linux/compiler.h just remove this instance.
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl
--- This is only used in ubifs related code and MAKEALL for arm is fine (besides the 5 boards with build issues) --- include/compiler.h | 3 --- 1 file changed, 3 deletions(-)
diff --git a/include/compiler.h b/include/compiler.h index 9afc11b..1451916 100644 --- a/include/compiler.h +++ b/include/compiler.h @@ -129,9 +129,6 @@ typedef unsigned long int uintptr_t;
#endif /* USE_HOSTCC */
-/* compiler options */ -#define uninitialized_var(x) x = x - #define likely(x) __builtin_expect(!!(x), 1) #define unlikely(x) __builtin_expect(!!(x), 0)
Jeroen,
Since clang has a different definition for uninitialized_var it will complain that it is redefined in include/compiler.h. Since these are already defined in linux/compiler.h just remove this instance.
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl
I don't mind this patch but it has made me realize another problem.
We have both include/compiler.h and include/linux/compiler.h. Some sources use tha former and others use the latter.
I don't know how to use the right one in the right place.
Header file policy is one of the biggest problem in U-boot.
Everyone has added ugly work-arounds to solve his own problem without correct views or judgement.
diff --git a/include/compiler.h b/include/compiler.h index 9afc11b..1451916 100644 --- a/include/compiler.h +++ b/include/compiler.h @@ -129,9 +129,6 @@ typedef unsigned long int uintptr_t;
#endif /* USE_HOSTCC */
-/* compiler options */ -#define uninitialized_var(x) x = x
#define likely(x) __builtin_expect(!!(x), 1) #define unlikely(x) __builtin_expect(!!(x), 0)
I am not sure if likely(x) and unlikely(x) should also duplicated here.
Best Regards Masahiro Yamada
Hello Masahiro,
On 18-09-14 04:14, Masahiro Yamada wrote:
Since clang has a different definition for uninitialized_var it will complain that it is redefined in include/compiler.h. Since these are already defined in linux/compiler.h just remove this instance.
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl
I don't mind this patch but it has made me realize another problem.
We have both include/compiler.h and include/linux/compiler.h. Some sources use tha former and others use the latter.
I don't know how to use the right one in the right place.
no me neither, although it seems arch / drivers tend to use linux/compiler.h more while tools include compiler.h more.
Header file policy is one of the biggest problem in U-boot.
Everyone has added ugly work-arounds to solve his own problem without correct views or judgement.
diff --git a/include/compiler.h b/include/compiler.h index 9afc11b..1451916 100644 --- a/include/compiler.h +++ b/include/compiler.h @@ -129,9 +129,6 @@ typedef unsigned long int uintptr_t;
#endif /* USE_HOSTCC */
-/* compiler options */ -#define uninitialized_var(x) x = x
- #define likely(x) __builtin_expect(!!(x), 1) #define unlikely(x) __builtin_expect(!!(x), 0)
I am not sure if likely(x) and unlikely(x) should also duplicated here.
yup I wondered about that too. likely / unlikely are used a lot more though then the isolated use of uninitialized_var. And as they don't cause any problems (the definitions are the same) I let them alone, but I think they should be removed as well some day.
Regards, Jeroen
On Thu, Sep 18, 2014 at 11:39:44AM +0200, Jeroen Hofstee wrote:
Hello Masahiro,
On 18-09-14 04:14, Masahiro Yamada wrote:
Since clang has a different definition for uninitialized_var it will complain that it is redefined in include/compiler.h. Since these are already defined in linux/compiler.h just remove this instance.
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl
I don't mind this patch but it has made me realize another problem.
We have both include/compiler.h and include/linux/compiler.h. Some sources use tha former and others use the latter.
I don't know how to use the right one in the right place.
no me neither, although it seems arch / drivers tend to use linux/compiler.h more while tools include compiler.h more.
My first guess is that we can't as easily throw <linux/compiler.h> into tools and thus need that around just for tools. Perhaps we should note as much in <compiler.h> and fix regular code to use <linux/compiler.h> ?
On Wed, Sep 17, 2014 at 08:33:48PM +0200, Jeroen Hofstee wrote:
Since clang has a different definition for uninitialized_var it will complain that it is redefined in include/compiler.h. Since these are already defined in linux/compiler.h just remove this instance.
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl
Applied to u-boot/master, thanks!
The libc headers on FreeBSD and likely related projects as well contain an header file, cdefs.h which provides similiar functionality as linux/compiler.h. It provides compiler independent defines like __weak __packed, to allow compiling with multiple compilers which might have a different syntax for such extension.
Since that header file is included in multiple standard headers, like stddef.h and stdarg.h, multiple definitions of those defines will be present if both are included. When compiling u-boot the compiler will warn about it hundreds of times since e.g. common.h will include both files indirectly.
commit 7ea50d52849fe8ffa5b5b74c979b60b1045d6fc9 "compiler_gcc: do not redefine __gnu_attributes" prevented such redefinitions, but this was undone by commit fb8ffd7cfc68b3dc44e182356a207d784cb30b34 "compiler*.h: sync include/linux/compiler*.h with Linux 3.16".
Add the checks back where necessary to prevent such warnings.
As the original patch this checkpatch warning is ignored: "WARNING: Adding new packed members is to be done with care"
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl --- changes since v1: As requested by Masahiro, elaborate a bit about what cdefs.h is and how these warnings are caused. Only this patch is reposted. --- include/linux/compiler-gcc.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 02ae99e..e057bd2 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -64,8 +64,12 @@ #endif
#define __deprecated __attribute__((deprecated)) +#ifndef __packed #define __packed __attribute__((packed)) +#endif +#ifndef __weak #define __weak __attribute__((weak)) +#endif
/* * it doesn't make sense on ARM (currently the only user of __naked) to trace @@ -91,8 +95,12 @@ * would be. * [...] */ +#ifndef __pure #define __pure __attribute__((pure)) +#endif +#ifndef __aligned #define __aligned(x) __attribute__((aligned(x))) +#endif #define __printf(a, b) __attribute__((format(printf, a, b))) #define __scanf(a, b) __attribute__((format(scanf, a, b))) #define noinline __attribute__((noinline)) @@ -115,4 +123,6 @@ */ #define uninitialized_var(x) x = x
+#ifndef __always_inline #define __always_inline inline __attribute__((always_inline)) +#endif
On Thu, 18 Sep 2014 20:10:27 +0200 Jeroen Hofstee jeroen@myspectrum.nl wrote:
The libc headers on FreeBSD and likely related projects as well contain an header file, cdefs.h which provides similiar functionality as linux/compiler.h. It provides compiler independent defines like __weak __packed, to allow compiling with multiple compilers which might have a different syntax for such extension.
Since that header file is included in multiple standard headers, like stddef.h and stdarg.h, multiple definitions of those defines will be present if both are included. When compiling u-boot the compiler will warn about it hundreds of times since e.g. common.h will include both files indirectly.
commit 7ea50d52849fe8ffa5b5b74c979b60b1045d6fc9 "compiler_gcc: do not redefine __gnu_attributes" prevented such redefinitions, but this was undone by commit fb8ffd7cfc68b3dc44e182356a207d784cb30b34 "compiler*.h: sync include/linux/compiler*.h with Linux 3.16".
Add the checks back where necessary to prevent such warnings.
As the original patch this checkpatch warning is ignored: "WARNING: Adding new packed members is to be done with care"
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl
Acked-by: Masahiro Yamada yamada.m@jp.panasonic.com
On Thu, Sep 18, 2014 at 08:10:27PM +0200, Jeroen Hofstee wrote:
The libc headers on FreeBSD and likely related projects as well contain an header file, cdefs.h which provides similiar functionality as linux/compiler.h. It provides compiler independent defines like __weak __packed, to allow compiling with multiple compilers which might have a different syntax for such extension.
Since that header file is included in multiple standard headers, like stddef.h and stdarg.h, multiple definitions of those defines will be present if both are included. When compiling u-boot the compiler will warn about it hundreds of times since e.g. common.h will include both files indirectly.
commit 7ea50d52849fe8ffa5b5b74c979b60b1045d6fc9 "compiler_gcc: do not redefine __gnu_attributes" prevented such redefinitions, but this was undone by commit fb8ffd7cfc68b3dc44e182356a207d784cb30b34 "compiler*.h: sync include/linux/compiler*.h with Linux 3.16".
Add the checks back where necessary to prevent such warnings.
As the original patch this checkpatch warning is ignored: "WARNING: Adding new packed members is to be done with care"
Cc: Masahiro Yamada yamada.m@jp.panasonic.com Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl Acked-by: Masahiro Yamada yamada.m@jp.panasonic.com
Applied to u-boot/master, thanks!
The mentioned binutils port got removed while the patch was pending. As Ian pointed out there is another port providing the binutils for arm now. Update the instructions accordingly.
Cc: ian@FreeBSD.org Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl --- doc/README.clang | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
--- changes since v1: update the name of the scipt as well.
diff --git a/doc/README.clang b/doc/README.clang index 9ad689f..52495d3 100644 --- a/doc/README.clang +++ b/doc/README.clang @@ -34,21 +34,21 @@ make HOSTCC=clang CC="clang -target $TRIPLET -mllvm -arm-use-movt=0 -no-integrat FreeBSD 11 (Current): -------------------- Since llvm 3.4 is currently in the base system, the integrated as is -incapable of building U-Boot. Therefore gas from devel/arm-eabi-binutils +incapable of building U-Boot. Therefore gas from devel/arm-gnueabi-binutils is used instead. It needs a symlinks to be picked up correctly though:
-ln -s /usr/local/bin/arm-eabi-as /usr/bin/arm-freebsd-eabi-as +ln -s /usr/local/bin/arm-gnueabi-freebsd-as /usr/bin/arm-freebsd-eabi-as
# The following commands compile U-Boot using the clang xdev toolchain. # NOTE: CROSS_COMPILE and target differ on purpose! -export CROSS_COMPILE=arm-eabi- +export CROSS_COMPILE=arm-gnueabi-freebsd- gmake CC="clang -target arm-freebsd-eabi --sysroot /usr/arm-freebsd -no-integrated-as -mllvm -arm-use-movt=0" rpi_b_defconfig gmake CC="clang -target arm-freebsd-eabi --sysroot /usr/arm-freebsd -no-integrated-as -mllvm -arm-use-movt=0" -j8
Given that u-boot will default to gcc, above commands can be simplified with a simple wrapper script, listed below.
-/usr/local/bin/arm-eabi-gcc +/usr/local/bin/arm-gnueabi-freebsd-gcc --- #!/bin/sh
On Sun, Sep 21, 2014 at 10:20:22AM +0200, Jeroen Hofstee wrote:
The mentioned binutils port got removed while the patch was pending. As Ian pointed out there is another port providing the binutils for arm now. Update the instructions accordingly.
Cc: ian@FreeBSD.org Cc: Tom Rini trini@ti.com Signed-off-by: Jeroen Hofstee jeroen@myspectrum.nl
Applied to u-boot/master, thanks!
participants (3)
-
Jeroen Hofstee -
Masahiro Yamada -
Tom Rini