Hi Simon,
On Tue, 16 Dec 2014 21:44:00 -0700 Simon Glass <[email protected]> wrote: > Hi Masahiro, > > On 15 December 2014 at 18:47, Masahiro YAMADA <[email protected]> > wrote: > > Simon, > > > > > > 2014-12-02 5:06 GMT+09:00 Simon Glass <[email protected]>: > >> Hi Masahiro, > >> > >> On 26 November 2014 at 00:45, Masahiro Yamada <[email protected]> > >> wrote: > >>> Hi Tom, Simon, other developers, > >>> > >>> > >>> > >>> Since commit 0d296cc2d3b8 (Provide option to avoid defining a custom > >>> version of uintptr_t) > >>> and commit 4166ecb247a1 (Add some standard headers external code might > >>> need), > >>> I have been wondering if they were right decisions. > >>> > >>> > >>> > >>> As arch/arm/include/asm/types.h of Linux Kernel says, > >>> using 'stdint.h' is not feasible. > >>> > >>> -------------------------------------->8------------------------------------- > >>> /* > >>> * The C99 types uintXX_t that are usually defined in 'stdint.h' are not > >>> as > >>> * unambiguous on ARM as you would expect. For the types below, there is a > >>> * difference on ARM between GCC built for bare metal ARM, GCC built for > >>> glibc > >>> * and the kernel itself, which results in build errors if you try to > >>> build with > >>> * -ffreestanding and include 'stdint.h' (such as when you include > >>> 'arm_neon.h' > >>> * in order to use NEON intrinsics) > >>> * > >>> * As the typedefs for these types in 'stdint.h' are based on builtin > >>> defines > >>> * supplied by GCC, we can tweak these to align with the kernel's idea of > >>> those > >>> * types, so 'linux/types.h' and 'stdint.h' can be safely included from > >>> the same > >>> * source file (provided that -ffreestanding is used). > >>> * > >>> * int32_t uint32_t uintptr_t > >>> * bare metal GCC long unsigned long unsigned int > >>> * glibc GCC int unsigned int unsigned int > >>> * kernel int unsigned int unsigned long > >>> */ > >>> --------------------------------------8<---------------------------------------- > >>> > >> > >> To me this doesn't matter. I feel that int32_t should probably be int > >> on 32-bit ARM, but actually so long as it is consistent then it is > >> fine if it is long. In fact so long as these types are defined > >> consistently, everything works. > > > > Gabe Black and you broke the consistency. > > See my explanation below. > > > > > > > > > >>> > >>> Actually, the kernel never includes <stdint.h> except host programs. > >>> > >>> > >>> Commit 0d296cc2d3b8 introduced "USE_STDINT", but it causes type conflicts > >>> depending on which GCC is used. > >>> > >>> > >>> With ARM bare metal GCC, > >>> > >>> yamada@beagle:~/workspace/u-boot$ make omap3_beagle_defconfig all > >>> CROSS_COMPILE=arm-none-eabi- USE_STDINT=1 > >>> # > >>> # configuration written to .config > >>> # > >>> # > >>> # configuration written to spl/.config > >>> # > >>> scripts/kconfig/conf --silentoldconfig Kconfig > >>> scripts/kconfig/conf --silentoldconfig Kconfig > >>> CHK include/config.h > >>> GEN include/autoconf.mk > >>> GEN include/autoconf.mk.dep > >>> GEN spl/include/autoconf.mk > >>> CHK include/config/uboot.release > >>> CHK include/generated/version_autogenerated.h > >>> CHK include/generated/timestamp_autogenerated.h > >>> UPD include/generated/timestamp_autogenerated.h > >>> CC lib/asm-offsets.s > >>> In file included from > >>> /opt/arm-2011.03/bin/../lib/gcc/arm-none-eabi/4.5.2/include/stdint.h:5:0, > >>> from include/compiler.h:117, > >>> from include/image.h:19, > >>> from include/common.h:85, > >>> from lib/asm-offsets.c:15: > >>> /opt/arm-2011.03/bin/../lib/gcc/arm-none-eabi/4.5.2/include/stdint-gcc.h:40:24: > >>> error: conflicting types for 'int32_t' > >>> include/linux/types.h:99:17: note: previous declaration of 'int32_t' was > >>> here > >>> /opt/arm-2011.03/bin/../lib/gcc/arm-none-eabi/4.5.2/include/stdint-gcc.h:52:25: > >>> error: conflicting types for 'uint32_t' > >>> include/linux/types.h:105:17: note: previous declaration of 'uint32_t' > >>> was here > >>> make[2]: *** [lib/asm-offsets.s] Error 1 > >>> make[1]: *** [prepare0] Error 2 > >>> make: *** [__build_one_by_one] Error 2 > >>> > >>> > >> > >> While toolchain is this? I don't see this problem - it seems broken. > > > > Not broken at all. > > I downloaded it from Mentor Graphics > > http://www.mentor.com/embedded-software/codesourcery > > > > This is a bare-metal compiler. > > If you do not understand, you should read arch/arm/include/asm/types.h > > of Linux Kernel once again. > > > > OK, so perhaps for that compiler you cannot use USE_STDINT=1? What > does it define for the conflicting type? It does define "uint32_t" as "unsigned long". > > > > > > Until commit 0d296cc2d, U-Boot has used the hard-coded defines like Linux. > > Well it still does, only that it also now has the *option* of using stdint.h. No. It is not "Do not worry, it is optional" things. You have changed code here ard threr for the optional feature. Commit aac618a32 (ext4: Use inttypes for printf() string) Commit 19ea4678c (Use int64_t for time types) Commit 6bf672592 (Use uint64_t instead of u64 in put_dec()) Commit c6da9ae8a (Tidy up data sizes and function comment in display_options) etc. > > > > In my understaing, we should only use ILP32 and LP64 compilers. > > > > > > short int long longlong > > pointer > > ILP32 (32bit system) 16 32 32 64 32 > > LP64 (64bit Unix-like system) 16 32 64 64 64 > > > > > > Whether it is 32bit or 64bit system, we can hard-code > > > > u32/uint32_t as unsigned int > > u64/uint64_t as unsigned long long > > uintptr_t as unsigned long > > > > We do not need to refer to compiler-provided headers. > > > > Moreover, we __should not__ refer to compiler-provided <stdint.h>. > > > > > > Including <stdint.h> means that we use uint32_t defined by the compiler. > > It is "unsigned int" on some compilers, and "unsigned long" on > > some compilers. > > I don't seem to have that problem, or at least I have not noticed it > with printf(). You are not trying to see the problem. You should try. Go to Linaro page and download the bare-metal toolchain. http://www.linaro.org/downloads/ > > > > We still have the hard-coded uint32_t define in <linux/types.h> > > This causes the type conflict error I showed above. > > > > OK, we can fix it, but horrible problems still remain. > > > > If we depends on <stdint.h>, we do not know if uint32_t is defined > > as "unsigned int" or "unsigned long". > > > > To print out 32bit variables, we would always have to use PRId32. > > Do you want to modify all the printf() printing 32bit variables > > just to make them unreadable? > > > > Ick, I have not seen this. Can you given an example of where this happens? Again, you are not trying to see that. Fine. Check my series: http://patchwork.ozlabs.org/patch/423341/ > > > > > >>> OK, we can fix "int32_t" and "uint32_t", but it still seems strange to > >>> see that > >>> "uint32_t" is defined as "unsigned long", whereas "u32" is defined as > >>> "unsigned int". > >>> > >>> If so, must we fix "u32", "s32", ... all the fixed-width typedefs ? > >>> > >>> I notice including <linux/types.h> in the U-Boot source tree and > >>> <stdint.h> provided by your compiler at the same time is a nightmare. > >>> > >>> If we lean toward <stdint.h>, we must ban <linux/types.h>, > >>> but <stdint.h> is not available all the time. > >>> For example, kernel.org tool-chains do not provide <stdint.h>. > >>> > >>> Maybe we can drastically re-write <linux/types.h> and friends > >>> to resolve the type conflicts, but I do not think we should not drift > >>> apart from the kernel > >>> because we have borrowed many source files from Linux. > >> > >> So far I don't see a big problem. > > > > Horrible problem! > > > >> I feel that, were stdint.h available > >> earlier, then types.h might not have been written and we would just > >> use stdint.h. Presumably stdint.h has been created to fix these sorts > >> of problems, and in fact for new projects, they would not define their > >> own types. > > > > What is missing is how to pass the variable width nicely to > > printf()/scanf(). > > > > printf("foo =" PRId32 "\n", foo_32); > > printf("bar =" PRId64 "\n", bar_64); > > > > This is ridiculous. Extremely unreadable. > > > > > > I want something like this: > > printf("foo = %32x \n", foo_32); > > printf("bar =" %64x \n", bar_64); > > > > > > If this had been provided, Linux and U-Boot might have adopted <stdint.h>, > > but in fact, PRI* is an awful workaround. > > Agreed it's not very readable but so long as it only applies to 64-bit > values and only to printf() it doesn't see bad to me. Provided we > resolve what you have brought up above. Check my series. > > > >> IMO in time the kernel and U-Boot might move to stdint.h, and having > >> it as an option at the moment helps us understand the issues. It does > >> not break any builds. > > > > I double it. > > Using <stdint.h> is a misjudge. It will mess up printf() everywhere. > > > > Linux Kernel has guideline how to print each variable type. > > Documentation/printk-formats.txt > > > > > >> I could be wrong though, time will tell. We should keep an eye on it. > > > > If we keep wrong code in the code base, someone might continue development > > base on it, which is also wrong. > > > > In order not to lose our time, wrong code should be immediately removed. > > How do you explain stdint.h? So many projects use it - it is now part > of the C99 standard. Has the world gone mad? It is trade-off and consistency things. If you make a decision to use <stdint.h>, it must be consistent everywhere. <stdint.h> gives int{8,16,32,64}_t and uint{8,16,32,64}_t, uintptr_t etc. We should always use them for fixed-width variable types and should never use hard-coded ones. That means we should not use include/linux/types.h and friends to hard-code u8/u16/u32/u64 etc. We always have to use PRIxN, PRIdN etc. to avoid printf-related warnings. For that, compiler-provided <inttypes.h> must be included. When you start a new project, you can include <stdint.h> and <inttypes.h> and follow the that rule from the beginning. You will see horrible things if you try to apply that rule on U-Boot. Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
