| Message ID | 1384770105-32364-1-git-send-email-yamada.m@jp.panasonic.com |
|---|---|
| State | Rejected |
| Delegated to: | Wolfgang Denk |
| Headers | show |
Dear Masahiro Yamada, In message <1384770105-32364-1-git-send-email-yamada.m@jp.panasonic.com> you wrote: > Currently U-boot defins bool type by including <stdbool.h> > rather than defining directly. > But it does not work for some cross compilers. Can you explain why this fails? AFAICT, <stdbool.h> is a compiler provided header file, which is mandatory by the C99 ISO standard. So if a compiler fails to work using it, it looks as if the compiler was broken. > In file included from /home/yamada/u-boot/arch/blackfin/include/asm/blackfin.h:13, > from /home/yamada/u-boot/include/common.h:92, > from cmd_test.c:17: > /home/yamada/u-boot/arch/blackfin/include/asm/blackfin_local.h:54: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'bfin_os_log_check' Which code does the compiler generate for line # 54: extern bool bfin_os_log_check(void); > diff --git a/include/linux/stddef.h b/include/linux/stddef.h > index c540f61..5893ec9 100644 > --- a/include/linux/stddef.h > +++ b/include/linux/stddef.h > @@ -12,6 +12,11 @@ > #include <linux/types.h> > #endif > > +enum { > + false = 0, > + true = 1 > +}; > + This looks fishy to me. Files that used to include <stdbool.h> do not necessarily include <linux/stddef.h>, so they may now be missing the definitions of "true" and "false". For example, arch/arm/cpu/armv7/mx6/soc.c (which you touched above) does not include <linux/stddef.h> ... > diff --git a/include/linux/types.h b/include/linux/types.h > index 9aebc4e..157e1ae 100644 > --- a/include/linux/types.h > +++ b/include/linux/types.h > @@ -3,7 +3,6 @@ > > #include <linux/posix_types.h> > #include <asm/types.h> > -#include <stdbool.h> > > #ifndef __KERNEL_STRICT_NAMES > > @@ -18,6 +17,8 @@ typedef __kernel_daddr_t daddr_t; > typedef __kernel_key_t key_t; > typedef __kernel_suseconds_t suseconds_t; > > +typedef _Bool bool; > + > #ifdef __KERNEL__ > typedef __kernel_uid32_t uid_t; > typedef __kernel_gid32_t gid_t; I can't say that I'm happy with that. I think we should first understand the real cause of the problem. Best regards, Wolfgang Denk
On Mon, Nov 18, 2013 at 05:28:56PM +0100, Wolfgang Denk wrote: > Dear Masahiro Yamada, > > In message <1384770105-32364-1-git-send-email-yamada.m@jp.panasonic.com> you wrote: > > Currently U-boot defins bool type by including <stdbool.h> > > rather than defining directly. > > But it does not work for some cross compilers. > > Can you explain why this fails? > > AFAICT, <stdbool.h> is a compiler provided header file, which is > mandatory by the C99 ISO standard. So if a compiler fails to work > using it, it looks as if the compiler was broken. Indeed. And the very ancient but (as far as I know) still "latest" gcc for the SPARC port, which is 3.4.4, has it. So if there's a problem here with blackfin it feels strongly like a toolchain issue.
Tom Rini <trini@ti.com> writes: > On Mon, Nov 18, 2013 at 05:28:56PM +0100, Wolfgang Denk wrote: >> Dear Masahiro Yamada, >> >> In message <1384770105-32364-1-git-send-email-yamada.m@jp.panasonic.com> you wrote: >> > Currently U-boot defins bool type by including <stdbool.h> >> > rather than defining directly. >> > But it does not work for some cross compilers. >> >> Can you explain why this fails? >> >> AFAICT, <stdbool.h> is a compiler provided header file, which is >> mandatory by the C99 ISO standard. So if a compiler fails to work >> using it, it looks as if the compiler was broken. > > Indeed. And the very ancient but (as far as I know) still "latest" gcc > for the SPARC port, which is 3.4.4, has it. So if there's a problem > here with blackfin it feels strongly like a toolchain issue. FWIW, my Blackfin cross-gcc has a correct stdbool.h. Even gcc 2.95 has it.
Hello Wolfgang, Tom. > > Currently U-boot defins bool type by including <stdbool.h> > > rather than defining directly. > > But it does not work for some cross compilers. > > Can you explain why this fails? At first, I have to admit that I misunderstood the reason of the error. It turned out that this is __not__ a compiler issue. I tried another Blackfin cross compiler, which is available at: https://www.kernel.org/pub/tools/crosstool/ But compiling faied at the same place. $ make CROSS_COMPILE=bfin-uclinux- bct-brettl2 <<snipped>> bfin-uclinux-gcc -g -Os -ffixed-P3 -fomit-frame-pointer -mno-fdpic -ffunction-sections -fdata-sections -mcpu=bf536-0.3 -D__KERNEL__ -I/home/yamada/u-boot/include -I/home/yamada/u-boot/arch/blackfin/include -fno-builtin -ffreestanding -nostdinc -isystem /opt/gcc-4.6.3-nolibc/bfin-uclinux/bin/../lib/gcc/bfin-uclinux/4.6.3/include -pipe -DCONFIG_BLACKFIN -Wall -Wstrict-prototypes -fno-stack-protector -Wno-format-nonliteral -Wno-format-security -o cmd_test.o cmd_test.c -c In file included from /home/yamada/u-boot/arch/blackfin/include/asm/blackfin.h:13:0, from /home/yamada/u-boot/include/common.h:92, from cmd_test.c:17: /home/yamada/u-boot/arch/blackfin/include/asm/blackfin_local.h:54:1: error: unknown type name 'bool' make[2]: *** [cmd_test.o] Error 1 make[2]: Leaving directory `/home/yamada/u-boot/common' make[1]: *** [common/built-in.o] Error 2 make[1]: Leaving directory `/home/yamada/u-boot' make: *** [bct-brettl2] Error 2 The issue resides in common/cmd_test.c I posted a patch to fix this build error in a different way. http://patchwork.ozlabs.org/patch/292247/ Tom, I think both can fix the error and you can select. - This patch - http://patchwork.ozlabs.org/patch/292247/ > > +enum { > > + false = 0, > > + true = 1 > > +}; > > + > > This looks fishy to me. Files that used to include <stdbool.h> do > not necessarily include <linux/stddef.h>, so they may now be missing > the definitions of "true" and "false". Yes. I noticed this. But it is Linux's way. So I intentionally put "bool" in <include/linux/types.h> and "true" and "false" in <include/linux/stddef.h>. > I can't say that I'm happy with that. I think we should first > understand the real cause of the problem. Now the real cause is clear. FYI: stdbool.h is usually something like follows. Note it uses _STDBOOL_H as an include gurad. /* * ISO C Standard: 7.16 Boolean type and values <stdbool.h> */ #ifndef _STDBOOL_H #define _STDBOOL_H #ifndef __cplusplus #define bool _Bool #define true 1 #define false 0 #else /* __cplusplus */ /* Supporting <stdbool.h> in C++ is a GCC extension. */ #define _Bool bool #define bool bool #define false false #define true true #endif /* __cplusplus */ /* Signal that all the definitions are present. */ #define __bool_true_false_are_defined 1 #endif /* stdbool.h */ Best Regards Masahiro Yamada
Hi.
I posted v2 of this patch
http://patchwork.ozlabs.org/patch/292258/
I think both of two solutions work.
(1) include <stdbool.h> in common/cmd_test.c
but undef true and false.
http://patchwork.ozlabs.org/patch/292247/
(2) Do not include <stdbool.h> and
define true and false with enum.
http://patchwork.ozlabs.org/patch/292258/
Personally, I prefer (2) to (1) because
- we don't need to tweak common/cmd_test.c any more
- we can reduce the conflict if we have a plan to update
Linux-originated header files.
Best Regards
Masahiro Yamada
Hi Masahiro Yamada, Why would hacking /include/linux/stddef.h and /include/linux/types.h be preferable? Regards, Graeme On Tue, Nov 19, 2013 at 3:37 PM, Masahiro Yamada <yamada.m@jp.panasonic.com>wrote: > Hi. > > I posted v2 of this patch > http://patchwork.ozlabs.org/patch/292258/ > > > I think both of two solutions work. > > (1) include <stdbool.h> in common/cmd_test.c > but undef true and false. > http://patchwork.ozlabs.org/patch/292247/ > > (2) Do not include <stdbool.h> and > define true and false with enum. > http://patchwork.ozlabs.org/patch/292258/ > > > Personally, I prefer (2) to (1) because > - we don't need to tweak common/cmd_test.c any more > - we can reduce the conflict if we have a plan to update > Linux-originated header files. > > > Best Regards > Masahiro Yamada > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
Hello Graeme > Why would hacking /include/linux/stddef.h and /include/linux/types.h be > preferable? The reason is this: > > Personally, I prefer (2) to (1) because > > - we don't need to tweak common/cmd_test.c any more > > - we can reduce the conflict if we have a plan to update > > Linux-originated header files. Some Linux header files are very old. Accoding to git log, for example, include/linux/types.h was added at 2000 and include/linux/stddef.h at 2002. We imported Linux headers and generally add an item to them every time we find some necessary feature is missing. For example, this patch: http://patchwork.ozlabs.org/patch/291435/ Adjusting little by little is one strategy. But we might think of synchronizing Linux-related headers with new ones in future. We have lots of files imported from Linux Kernel. So, basiclly, there is more or less advantage to mimic Linux's way. Anyway, this is my personal option. Opinions about this item may differ among people. Best Regards Masahiro Yamada
Hi Masahiro Yamada, On Tue, Nov 19, 2013 at 4:59 PM, Masahiro Yamada <yamada.m@jp.panasonic.com>wrote: > Hello Graeme > > > > Why would hacking /include/linux/stddef.h and /include/linux/types.h be > > preferable? > > The reason is this: > > > Personally, I prefer (2) to (1) because > > > - we don't need to tweak common/cmd_test.c any more > > > - we can reduce the conflict if we have a plan to update > > > Linux-originated header files. > > > Some Linux header files are very old. > Accoding to git log, for example, include/linux/types.h > was added at 2000 and include/linux/stddef.h at 2002. > I figured that might be the case after I hit send :) > > We imported Linux headers and > generally add an item to them > every time we find some necessary feature is missing. > For example, this patch: > http://patchwork.ozlabs.org/patch/291435/ > Adjusting little by little is one strategy. > But we might think of synchronizing Linux-related headers > with new ones in future. > Would be interesting to see what kinds of chaos would ensue if we did... > We have lots of files imported from Linux Kernel. > So, basiclly, there is more or less advantage to mimic Linux's way. > Agreed - we use the Linux coding standards and probably >90% of our build probably happen on Linux machines. > Anyway, this is my personal option. > Opinions about this item may differ among people. > I also think it would be better to have all headers under /include/linux/ synchronised with Linux. But I've been out of the game so long now, I don't know if I've inadvertently started a flame war... Regards, Graeme P.S. Apologies for my previous top-post - please don't hurt me
diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c index a390296..54ef447 100644 --- a/arch/arm/cpu/armv7/mx6/soc.c +++ b/arch/arm/cpu/armv7/mx6/soc.c @@ -15,7 +15,6 @@ #include <asm/arch/sys_proto.h> #include <asm/imx-common/boot_mode.h> #include <asm/imx-common/dma.h> -#include <stdbool.h> #include <asm/arch/mxc_hdmi.h> #include <asm/arch/crm_regs.h> diff --git a/include/galileo/core.h b/include/galileo/core.h index 95013fa..2d1b2ed 100644 --- a/include/galileo/core.h +++ b/include/galileo/core.h @@ -14,7 +14,7 @@ space). The macros take care of Big/Little endian conversions. /* includes */ #include "gt64260R.h" -#include <stdbool.h> +#include <linux/types.h> extern unsigned int INTERNAL_REG_BASE_ADDR; diff --git a/include/linux/stddef.h b/include/linux/stddef.h index c540f61..5893ec9 100644 --- a/include/linux/stddef.h +++ b/include/linux/stddef.h @@ -12,6 +12,11 @@ #include <linux/types.h> #endif +enum { + false = 0, + true = 1 +}; + #ifndef __CHECKER__ #undef offsetof #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER) diff --git a/include/linux/types.h b/include/linux/types.h index 9aebc4e..157e1ae 100644 --- a/include/linux/types.h +++ b/include/linux/types.h @@ -3,7 +3,6 @@ #include <linux/posix_types.h> #include <asm/types.h> -#include <stdbool.h> #ifndef __KERNEL_STRICT_NAMES @@ -18,6 +17,8 @@ typedef __kernel_daddr_t daddr_t; typedef __kernel_key_t key_t; typedef __kernel_suseconds_t suseconds_t; +typedef _Bool bool; + #ifdef __KERNEL__ typedef __kernel_uid32_t uid_t; typedef __kernel_gid32_t gid_t;
Currently U-boot defins bool type by including <stdbool.h> rather than defining directly. But it does not work for some cross compilers. This commit changes header files to define bool, true, false in the same way as Linux Kernel does. Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com> --- Refer to include/linux/types.h and include/linux/stddef.h of Linux Kernel. When I compile Blackfin boards, I use this crosstool: http://dev.gentoo.org/~vapier/u-boot/bfin.tar.xz But with this crosstool, I cannot build bct-brettl2 board. For the current u-boot/master, compiling bct-brettl2 failed with a error $ LANG=C CROSS_COMPILE=bfin-elf- ./MAKEALL bct-brettl2 Configuring for bct-brettl2 board... /home/yamada/u-boot/arch/blackfin/include/asm/blackfin_local.h:54: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'bfin_os_log_check' make[1]: *** [cmd_test.o] Error 1 make: *** [common/built-in.o] Error 2 bfin-elf-size: './u-boot': No such file In file included from /home/yamada/u-boot/arch/blackfin/include/asm/serial.h:48, from initcode.c:19: /home/yamada/u-boot/arch/blackfin/include/asm/serial1.h: In function 'uart_init': /home/yamada/u-boot/arch/blackfin/include/asm/serial1.h:238: warning: implicit declaration of function 'BUG' initcode.c: In function 'serial_init': initcode.c:150: warning: unused variable 'uart_base' initcode.c: In function 'initcode': initcode.c:743: warning: 'sdivB' is used uninitialized in this function initcode.c:1004: note: 'sdivB' was declared here initcode.c:743: warning: 'divB' is used uninitialized in this function initcode.c:1004: note: 'divB' was declared here initcode.c:744: warning: 'vcoB' is used uninitialized in this function initcode.c:1004: note: 'vcoB' was declared here In file included from /home/yamada/u-boot/arch/blackfin/include/asm/blackfin.h:13, from /home/yamada/u-boot/include/common.h:92, from cmd_test.c:17: /home/yamada/u-boot/arch/blackfin/include/asm/blackfin_local.h:54: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'bfin_os_log_check' make[1]: *** [cmd_test.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [common/built-in.o] Error 2 make: *** Waiting for unfinished jobs.... --------------------- SUMMARY ---------------------------- Boards compiled: 1 Boards with errors: 1 ( bct-brettl2 ) ---------------------------------------------------------- After applying this patch, I still have warnings but I can get u-boot binary. $ CROSS_COMPILE=bfin-elf- ./MAKEALL bct-brettl2 Configuring for bct-brettl2 board... text data bss dec hex filename 180664 6018 61896 248578 3cb02 ./u-boot In file included from /home/yamada/u-boot/arch/blackfin/include/asm/serial.h:48, from initcode.c:19: /home/yamada/u-boot/arch/blackfin/include/asm/serial1.h: In function ‘uart_init’: /home/yamada/u-boot/arch/blackfin/include/asm/serial1.h:238: warning: implicit declaration of function ‘BUG’ initcode.c: In function ‘serial_init’: initcode.c:150: warning: unused variable ‘uart_base’ initcode.c: In function ‘initcode’: initcode.c:743: warning: ‘sdivB’ is used uninitialized in this function initcode.c:1004: note: ‘sdivB’ was declared here initcode.c:743: warning: ‘divB’ is used uninitialized in this function initcode.c:1004: note: ‘divB’ was declared here initcode.c:744: warning: ‘vcoB’ is used uninitialized in this function initcode.c:1004: note: ‘vcoB’ was declared here --------------------- SUMMARY ---------------------------- Boards compiled: 1 Boards with warnings but no errors: 1 ( bct-brettl2 ) ---------------------------------------------------------- I think this patch will not give a bad impact to the other boards. I did build test for all target boards. (But output binaries differ. And I did not check run-time test.) arch/arm/cpu/armv7/mx6/soc.c | 1 - include/galileo/core.h | 2 +- include/linux/stddef.h | 5 +++++ include/linux/types.h | 3 ++- 4 files changed, 8 insertions(+), 3 deletions(-)