| Message ID | 1532613591-8444-4-git-send-email-philipp.tomsich@theobroma-systems.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Support 4GB of memory on 32bit systems | expand |
2018-07-26 22:59 GMT+09:00 Philipp Tomsich <philipp.tomsich@theobroma-systems.com>: > With the ram-size variable changed to u64, we'll need appropriate > macros for printing u64 values correctly either in 32bit builds > (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models). Best to > make the PRIx64 macro available everywhere. > > This include inttypes.h from common.h to make the various macros for > formatted printing available to everyone. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- NACK. PRIx64 is evil crap. I would make the code super ugly. Do not use it. The right thing to do is use the same typedefs for all architectures. typedef unsigned char u8; typedef unsigned short u16; typedef unsigned int u32; typedef unsigned long long u64; This works for both ILP32 and LP64. Use '%llx' for printing u64 variables _always_. This is what Linux exactly does. In fact, Linux defined fixed-width types differently for different architectures in old days. After long time effort, Linux unified the fixed-width types for the kernel space. https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h See Linux commit: commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf Author: Geert Uytterhoeven <geert@linux-m68k.org> Date: Thu Jan 23 15:53:43 2014 -0800 asm/types.h: Remove include/asm-generic/int-l64.h And, I fixed ARM Trusted Firmware in the same way: https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0 U-Boot is still doing wrong, and core developers in U-Boot do not understand this, unfortunately. > include/common.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/common.h b/include/common.h > index 940161f..9de9145 100644 > --- a/include/common.h > +++ b/include/common.h > @@ -30,6 +30,7 @@ typedef volatile unsigned char vu_char; > #include <linux/stringify.h> > #include <asm/ptrace.h> > #include <stdarg.h> > +#include <inttypes.h> > #include <stdio.h> > #include <linux/kernel.h> > > -- > 2.1.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi Philipp > -----Original Message----- > From: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > Sent: jeudi 26 juillet 2018 16:00 > > With the ram-size variable changed to u64, we'll need appropriate macros for > printing u64 values correctly either in 32bit builds (i.e. ILP32 models) or in 64bit > builds (i.e. LP64 models). Best to make the PRIx64 macro available everywhere. > > This include inttypes.h from common.h to make the various macros for > formatted printing available to everyone. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > > include/common.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/common.h b/include/common.h index 940161f..9de9145 > 100644 > --- a/include/common.h > +++ b/include/common.h > @@ -30,6 +30,7 @@ typedef volatile unsigned char vu_char; > #include <linux/stringify.h> > #include <asm/ptrace.h> > #include <stdarg.h> > +#include <inttypes.h> > #include <stdio.h> > #include <linux/kernel.h> I think the include should be moved after the line 40 and __STDC_FORMAT_MACROS definition : -/* Bring in printf format macros if inttypes.h is included */ +/* Bring in printf format macros defined in inttypes.h */ #define __STDC_FORMAT_MACROS +#include <inttypes.h> Regards Patrick
+Tom Hi Masahiro, On 30 July 2018 at 19:41, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-07-26 22:59 GMT+09:00 Philipp Tomsich > <philipp.tomsich@theobroma-systems.com>: >> With the ram-size variable changed to u64, we'll need appropriate >> macros for printing u64 values correctly either in 32bit builds >> (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models). Best to >> make the PRIx64 macro available everywhere. >> >> This include inttypes.h from common.h to make the various macros for >> formatted printing available to everyone. >> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >> --- > > > NACK. > > > PRIx64 is evil crap. I would make the code super ugly. > Do not use it. > > > The right thing to do is use the same typedefs > for all architectures. > > typedef unsigned char u8; > typedef unsigned short u16; > typedef unsigned int u32; > typedef unsigned long long u64; > > This works for both ILP32 and LP64. > > > Use '%llx' for printing u64 variables _always_. > > > > This is what Linux exactly does. > > > > In fact, Linux defined fixed-width types differently > for different architectures in old days. > > > After long time effort, Linux unified > the fixed-width types for the kernel space. > > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h > > > > See Linux commit: > > commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf > Author: Geert Uytterhoeven <geert@linux-m68k.org> > Date: Thu Jan 23 15:53:43 2014 -0800 > > asm/types.h: Remove include/asm-generic/int-l64.h > > > > > > And, I fixed ARM Trusted Firmware in the same way: > > https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0 > > > > > > U-Boot is still doing wrong, > and core developers in U-Boot do not understand this, unfortunately. While this works in many cases we do seem to have problems with some toolchains. Perhaps things are better now as my problems were a a few years back. Things like size_t with %z caused problems too. I remember m68k producing warnings when I tried this. I am certainly interested in converting over to this other approach. I am also OK with the PRi stuff, since it only affects a relatively small number of cases. Regards, Simon
On Thu, Aug 02, 2018 at 10:56:11AM -0600, Simon Glass wrote: > +Tom > > Hi Masahiro, > > On 30 July 2018 at 19:41, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > 2018-07-26 22:59 GMT+09:00 Philipp Tomsich > > <philipp.tomsich@theobroma-systems.com>: > >> With the ram-size variable changed to u64, we'll need appropriate > >> macros for printing u64 values correctly either in 32bit builds > >> (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models). Best to > >> make the PRIx64 macro available everywhere. > >> > >> This include inttypes.h from common.h to make the various macros for > >> formatted printing available to everyone. > >> > >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > >> --- > > > > > > NACK. > > > > > > PRIx64 is evil crap. I would make the code super ugly. > > Do not use it. > > > > > > The right thing to do is use the same typedefs > > for all architectures. > > > > typedef unsigned char u8; > > typedef unsigned short u16; > > typedef unsigned int u32; > > typedef unsigned long long u64; > > > > This works for both ILP32 and LP64. > > > > > > Use '%llx' for printing u64 variables _always_. > > > > > > > > This is what Linux exactly does. > > > > > > > > In fact, Linux defined fixed-width types differently > > for different architectures in old days. > > > > > > After long time effort, Linux unified > > the fixed-width types for the kernel space. > > > > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h > > > > > > > > See Linux commit: > > > > commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf > > Author: Geert Uytterhoeven <geert@linux-m68k.org> > > Date: Thu Jan 23 15:53:43 2014 -0800 > > > > asm/types.h: Remove include/asm-generic/int-l64.h > > > > > > > > > > > > And, I fixed ARM Trusted Firmware in the same way: > > > > https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0 > > > > > > > > > > > > U-Boot is still doing wrong, > > and core developers in U-Boot do not understand this, unfortunately. > > While this works in many cases we do seem to have problems with some > toolchains. Perhaps things are better now as my problems were a a few > years back. Things like size_t with %z caused problems too. I remember > m68k producing warnings when I tried this. > > I am certainly interested in converting over to this other approach. I > am also OK with the PRi stuff, since it only affects a relatively > small number of cases. It would certainly be worth giving things another try with current compilers.
2018-08-04 0:01 GMT+09:00 Tom Rini <trini@konsulko.com>: > On Thu, Aug 02, 2018 at 10:56:11AM -0600, Simon Glass wrote: >> +Tom >> >> Hi Masahiro, >> >> On 30 July 2018 at 19:41, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> > 2018-07-26 22:59 GMT+09:00 Philipp Tomsich >> > <philipp.tomsich@theobroma-systems.com>: >> >> With the ram-size variable changed to u64, we'll need appropriate >> >> macros for printing u64 values correctly either in 32bit builds >> >> (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models). Best to >> >> make the PRIx64 macro available everywhere. >> >> >> >> This include inttypes.h from common.h to make the various macros for >> >> formatted printing available to everyone. >> >> >> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >> >> --- >> > >> > >> > NACK. >> > >> > >> > PRIx64 is evil crap. I would make the code super ugly. >> > Do not use it. >> > >> > >> > The right thing to do is use the same typedefs >> > for all architectures. >> > >> > typedef unsigned char u8; >> > typedef unsigned short u16; >> > typedef unsigned int u32; >> > typedef unsigned long long u64; >> > >> > This works for both ILP32 and LP64. >> > >> > >> > Use '%llx' for printing u64 variables _always_. >> > >> > >> > >> > This is what Linux exactly does. >> > >> > >> > >> > In fact, Linux defined fixed-width types differently >> > for different architectures in old days. >> > >> > >> > After long time effort, Linux unified >> > the fixed-width types for the kernel space. >> > >> > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/int-ll64.h >> > >> > >> > >> > See Linux commit: >> > >> > commit 0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf >> > Author: Geert Uytterhoeven <geert@linux-m68k.org> >> > Date: Thu Jan 23 15:53:43 2014 -0800 >> > >> > asm/types.h: Remove include/asm-generic/int-l64.h >> > >> > >> > >> > >> > >> > And, I fixed ARM Trusted Firmware in the same way: >> > >> > https://github.com/ARM-software/arm-trusted-firmware/commit/0a2d5b43c81ed6132761023bf43755f13122ddf0 >> > >> > >> > >> > >> > >> > U-Boot is still doing wrong, >> > and core developers in U-Boot do not understand this, unfortunately. >> >> While this works in many cases we do seem to have problems with some >> toolchains. Perhaps things are better now as my problems were a a few >> years back. Please pin-point the pre-built toolchains with which you had problems. In other words, please show a compiler that has: sizeof(int) != 4 or sizeof(long long) != 8 >> Things like size_t with %z caused problems too. I remember >> m68k producing warnings when I tried this. This is true, but the typedefs of uint{8,16,32,64}_t and the typedef of size_t are _different_ problems. Please do not mix them up to exaggerate things. Most 32 bit architectures use "unsigned int" size_t, and all 64 bit architectures use "unsigned long" size_t as stated in https://github.com/torvalds/linux/blob/v4.17/include/uapi/asm-generic/posix_types.h#L63 Linux kernel hard-codes size_t and ssize_t, thus people are supposed to use a compiler with proper size_t define. Such compilers are provided in https://mirrors.edge.kernel.org/pub/tools/crosstool/ If you are not happy about hard-coding size_t, you can do like this: typedef __SIZE_TYPE__ size_t; typedef __typeof(__builtin_choose_expr(__builtin_types_compatible_p(size_t, unsigned long), (long)0, (int)0)) ssize_t; >> I am certainly interested in converting over to this other approach. I >> am also OK with the PRi stuff, since it only affects a relatively >> small number of cases. Whether the code is right or not should not be judged by the number of use cases. > It would certainly be worth giving things another try with current > compilers. > > -- > Tom > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot >
diff --git a/include/common.h b/include/common.h index 940161f..9de9145 100644 --- a/include/common.h +++ b/include/common.h @@ -30,6 +30,7 @@ typedef volatile unsigned char vu_char; #include <linux/stringify.h> #include <asm/ptrace.h> #include <stdarg.h> +#include <inttypes.h> #include <stdio.h> #include <linux/kernel.h>
With the ram-size variable changed to u64, we'll need appropriate macros for printing u64 values correctly either in 32bit builds (i.e. ILP32 models) or in 64bit builds (i.e. LP64 models). Best to make the PRIx64 macro available everywhere. This include inttypes.h from common.h to make the various macros for formatted printing available to everyone. Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> --- include/common.h | 1 + 1 file changed, 1 insertion(+)