[U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
tiny-printf does not know about the "l" modifier so far, which breaks the crash dump on AArch64, because it uses %lx to print the registers. Add an easy way of handling longs correctly. Signed-off-by: Andre Przywara --- lib/tiny-printf.c | 43 +-- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 30ac759..b01099d 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt) info->zs = 1; } -static void div_out(struct printf_info *info, unsigned int *num, - unsigned int div) +static void div_out(struct printf_info *info, unsigned long *num, + unsigned long div) { unsigned char dgt = 0; @@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) { char ch; char *p; - unsigned int num; + unsigned long num; char buf[12]; - unsigned int div; + unsigned long div; while ((ch = *(fmt++))) { if (ch != '%') { @@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) } else { bool lz = false; int width = 0; + bool islong = false; ch = *(fmt++); + if (ch == '-') + ch = *(fmt++); + if (ch == '0') { ch = *(fmt++); lz = 1; @@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) ch = *fmt++; } } + if (ch == 'l') { + ch = *(fmt++); + islong = true; + } + info->bf = buf; p = info->bf; info->zs = 0; @@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) goto abort; case 'u': case 'd': - num = va_arg(va, unsigned int); - if (ch == 'd' && (int)num < 0) { - num = -(int)num; + div = 10; + if (islong) { + num = va_arg(va, unsigned long); + if (sizeof(long) > 4) + div *= div * 10; + } else { + num = va_arg(va, unsigned int); + } + + if (ch == 'd' && (long)num < 0) { + num = -(long)num; out(info, '-'); } if (!num) { out_dgt(info, 0); } else { - for (div = 10; div; div /= 10) + for (; div; div /= 10) div_out(info, &num, div); } break; case 'x': - num = va_arg(va, unsigned int); + if (islong) { + num = va_arg(va, unsigned long); + div = 1UL << (sizeof(long) * 8 - 4); + } else { + num = va_arg(va, unsigned int); + div = 0x1000; + } if (!num) { out_dgt(info, 0); } else { - for (div = 0x1000; div; div /= 0x10) + for (; div; div /= 0x10) div_out(info, &num, div); } break; -- 2.8.2 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
On 20/11/2016 15:56, Andre Przywara wrote: tiny-printf does not know about the "l" modifier so far, which breaks the crash dump on AArch64, because it uses %lx to print the registers. Add an easy way of handling longs correctly. Signed-off-by: Andre Przywara --- lib/tiny-printf.c | 43 +-- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 30ac759..b01099d 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt) info->zs = 1; } -static void div_out(struct printf_info *info, unsigned int *num, - unsigned int div) +static void div_out(struct printf_info *info, unsigned long *num, + unsigned long div) { unsigned char dgt = 0; @@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) { char ch; char *p; - unsigned int num; + unsigned long num; char buf[12]; - unsigned int div; + unsigned long div; while ((ch = *(fmt++))) { if (ch != '%') { @@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) } else { bool lz = false; int width = 0; + bool islong = false; ch = *(fmt++); + if (ch == '-') + ch = *(fmt++); + What does this do? I don't see '-' mentioned in the patch description. if (ch == '0') { ch = *(fmt++); lz = 1; @@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) ch = *fmt++; } } + if (ch == 'l') { + ch = *(fmt++); + islong = true; + } + info->bf = buf; p = info->bf; info->zs = 0; @@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) goto abort; case 'u': case 'd': - num = va_arg(va, unsigned int); - if (ch == 'd' && (int)num < 0) { - num = -(int)num; + div = 10; + if (islong) { Check here if sizeof(long) > 4, so that the whole branch gets optimized away on 32bit. + num = va_arg(va, unsigned long); + if (sizeof(long) > 4) + div *= div * 10; + } else { + num = va_arg(va, unsigned int); + } + + if (ch == 'd' && (long)num < 0) { + num = -(long)num; Num is a long now and before. So if you have a 32bit signed input, it will sign extend incorrectly here. You need an additional check if (islong) num = -(long)num; else num = -(int)num; Let's hope the compiler on 32bit is smart enough to know that it can combine those two cases :). out(info, '-'); } if (!num) { out_dgt(info, 0); } else { - for (div = 10; div; div /= 10) + for (; div; div /= 10) Any particular reason for that change? div_out(info, &num, div); } break; case 'x': - num = va_arg(va, unsigned int); + if (islong) { Same comment as above. Alex + num = va_arg(va, unsigned long); + div = 1UL << (sizeof(long) * 8 - 4); + } else { + num = va_arg(va, unsigned int); + div = 0x1000; + } if (!num) { out_dgt(info, 0); } else { - for (div = 0x1000; div; div /= 0x10) + for (; div; div /= 0x10)
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
Hi Alex, On 21/11/16 15:42, Alexander Graf wrote: > > > On 20/11/2016 15:56, Andre Przywara wrote: >> tiny-printf does not know about the "l" modifier so far, which breaks >> the crash dump on AArch64, because it uses %lx to print the registers. >> Add an easy way of handling longs correctly. >> >> Signed-off-by: Andre Przywara >> --- >> lib/tiny-printf.c | 43 +-- >> 1 file changed, 33 insertions(+), 10 deletions(-) >> >> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >> index 30ac759..b01099d 100644 >> --- a/lib/tiny-printf.c >> +++ b/lib/tiny-printf.c >> @@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt) >> info->zs = 1; >> } >> >> -static void div_out(struct printf_info *info, unsigned int *num, >> -unsigned int div) >> +static void div_out(struct printf_info *info, unsigned long *num, >> +unsigned long div) >> { >> unsigned char dgt = 0; >> >> @@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char >> *fmt, va_list va) >> { >> char ch; >> char *p; >> -unsigned int num; >> +unsigned long num; >> char buf[12]; >> -unsigned int div; >> +unsigned long div; >> >> while ((ch = *(fmt++))) { >> if (ch != '%') { >> @@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char >> *fmt, va_list va) >> } else { >> bool lz = false; >> int width = 0; >> +bool islong = false; >> >> ch = *(fmt++); >> +if (ch == '-') >> +ch = *(fmt++); >> + > > What does this do? I don't see '-' mentioned in the patch description. Argh, apparently the comment in the commit message got lost during a patch reshuffle. Sorry, will re-add it. We need it because some SPL printf uses '-', just ignoring it here seems fine for SPL purposes though. > >> if (ch == '0') { >> ch = *(fmt++); >> lz = 1; >> @@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char >> *fmt, va_list va) >> ch = *fmt++; >> } >> } >> +if (ch == 'l') { >> +ch = *(fmt++); >> +islong = true; >> +} >> + >> info->bf = buf; >> p = info->bf; >> info->zs = 0; >> @@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char >> *fmt, va_list va) >> goto abort; >> case 'u': >> case 'd': >> -num = va_arg(va, unsigned int); >> -if (ch == 'd' && (int)num < 0) { >> -num = -(int)num; >> +div = 10; >> +if (islong) { > > Check here if sizeof(long) > 4, so that the whole branch gets optimized > away on 32bit. Good idea. >> +num = va_arg(va, unsigned long); >> +if (sizeof(long) > 4) >> +div *= div * 10; >> +} else { >> +num = va_arg(va, unsigned int); >> +} >> + >> +if (ch == 'd' && (long)num < 0) { >> +num = -(long)num; > > Num is a long now and before. So if you have a 32bit signed input, it > will sign extend incorrectly here. You need an additional check > > if (islong) > num = -(long)num; > else > num = -(int)num; > > Let's hope the compiler on 32bit is smart enough to know that it can > combine those two cases :). > >> out(info, '-'); >> } >> if (!num) { >> out_dgt(info, 0); >> } else { >> -for (div = 10; div; div /= 10) >> +for (; div; div /= 10) > > Any particular reason for that change? This algorithm so far only cared for 32-bit values, so it set the start divider to 1E9. This is not sufficient for 64-bit longs in AA64. So I compute div above, depending on the actual size of long. >> div_out(info, &num, div); >> } >> break; >> case 'x': >> -num = va_arg(va, unsigned int); >> +if (islong) { > > Same comment as above. Thanks, I will take a look at the rest. Cheers, Andre. > >> +num = va_arg(va, unsigned long); >> +div = 1UL << (sizeof(long) * 8 - 4); >> +} else { >> +num = va_arg(va, unsigned int); >> +div = 0x1000; >> +} >> if (!num) { >> out_dgt(info, 0); >> } else { >> -for (div = 0x1000; div; div /= 0x10) >> +for (; div; div /= 0x10) >> div_out(info, &num, div); >> } >> break; >> _
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
On 21/11/2016 16:56, Andre Przywara wrote: Hi Alex, On 21/11/16 15:42, Alexander Graf wrote: On 20/11/2016 15:56, Andre Przywara wrote: tiny-printf does not know about the "l" modifier so far, which breaks the crash dump on AArch64, because it uses %lx to print the registers. Add an easy way of handling longs correctly. Signed-off-by: Andre Przywara --- lib/tiny-printf.c | 43 +-- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 30ac759..b01099d 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt) info->zs = 1; } -static void div_out(struct printf_info *info, unsigned int *num, -unsigned int div) +static void div_out(struct printf_info *info, unsigned long *num, +unsigned long div) { unsigned char dgt = 0; @@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) { char ch; char *p; -unsigned int num; +unsigned long num; char buf[12]; -unsigned int div; +unsigned long div; while ((ch = *(fmt++))) { if (ch != '%') { @@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) } else { bool lz = false; int width = 0; +bool islong = false; ch = *(fmt++); +if (ch == '-') +ch = *(fmt++); + What does this do? I don't see '-' mentioned in the patch description. Argh, apparently the comment in the commit message got lost during a patch reshuffle. Sorry, will re-add it. We need it because some SPL printf uses '-', just ignoring it here seems fine for SPL purposes though. if (ch == '0') { ch = *(fmt++); lz = 1; @@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) ch = *fmt++; } } +if (ch == 'l') { +ch = *(fmt++); +islong = true; +} + info->bf = buf; p = info->bf; info->zs = 0; @@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va) goto abort; case 'u': case 'd': -num = va_arg(va, unsigned int); -if (ch == 'd' && (int)num < 0) { -num = -(int)num; +div = 10; +if (islong) { Check here if sizeof(long) > 4, so that the whole branch gets optimized away on 32bit. Good idea. +num = va_arg(va, unsigned long); +if (sizeof(long) > 4) +div *= div * 10; +} else { +num = va_arg(va, unsigned int); +} + +if (ch == 'd' && (long)num < 0) { +num = -(long)num; Num is a long now and before. So if you have a 32bit signed input, it will sign extend incorrectly here. You need an additional check if (islong) num = -(long)num; else num = -(int)num; Let's hope the compiler on 32bit is smart enough to know that it can combine those two cases :). out(info, '-'); } if (!num) { out_dgt(info, 0); } else { -for (div = 10; div; div /= 10) +for (; div; div /= 10) Any particular reason for that change? This algorithm so far only cared for 32-bit values, so it set the start divider to 1E9. This is not sufficient for 64-bit longs in AA64. So I compute div above, depending on the actual size of long. Ah, I missed the div *= up there. Sure, then it makes sense. Btw, have you checked that the compiler is smart enough to do constant propagation here? Multiplications can be very expensive. Alex ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
On Sun, 20 Nov 2016 14:56:59 + Andre Przywara wrote: > tiny-printf does not know about the "l" modifier so far, which breaks > the crash dump on AArch64, because it uses %lx to print the registers. > Add an easy way of handling longs correctly. I can't help but notice that the changes of this kind are in a way defeating the original purpose of tiny-printf. And it is surely not the first patch adding features to tiny-printf. I guess, in the end we may end up with a large and bloated printf implementation again :-) A possible solution might be just a strict check when parsing format modifiers and abort with an error message (yeah, this will introduce some size increase, but hopefully the last one). This way we acknowledge the fact that tiny-printf is a reduced incomplete implementation, and that the callers need to take this into account. As for the "l" modifier. How much does it add to the code size? IMHO this information should be mentioned in the commit message. Can the AArch64 crash dump code be modified to avoid using it? Or can we have the "l" modifier supported on 64-bit platforms only? > Signed-off-by: Andre Przywara > --- > lib/tiny-printf.c | 43 +-- > 1 file changed, 33 insertions(+), 10 deletions(-) > > diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > index 30ac759..b01099d 100644 > --- a/lib/tiny-printf.c > +++ b/lib/tiny-printf.c > @@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt) > info->zs = 1; > } > > -static void div_out(struct printf_info *info, unsigned int *num, > - unsigned int div) > +static void div_out(struct printf_info *info, unsigned long *num, > + unsigned long div) > { > unsigned char dgt = 0; > > @@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char *fmt, > va_list va) > { > char ch; > char *p; > - unsigned int num; > + unsigned long num; > char buf[12]; > - unsigned int div; > + unsigned long div; > > while ((ch = *(fmt++))) { > if (ch != '%') { > @@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char *fmt, > va_list va) > } else { > bool lz = false; > int width = 0; > + bool islong = false; > > ch = *(fmt++); > + if (ch == '-') > + ch = *(fmt++); > + > if (ch == '0') { > ch = *(fmt++); > lz = 1; > @@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char *fmt, > va_list va) > ch = *fmt++; > } > } > + if (ch == 'l') { > + ch = *(fmt++); > + islong = true; > + } > + > info->bf = buf; > p = info->bf; > info->zs = 0; > @@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char *fmt, > va_list va) > goto abort; > case 'u': > case 'd': > - num = va_arg(va, unsigned int); > - if (ch == 'd' && (int)num < 0) { > - num = -(int)num; > + div = 10; > + if (islong) { > + num = va_arg(va, unsigned long); > + if (sizeof(long) > 4) > + div *= div * 10; > + } else { > + num = va_arg(va, unsigned int); > + } > + > + if (ch == 'd' && (long)num < 0) { > + num = -(long)num; > out(info, '-'); > } > if (!num) { > out_dgt(info, 0); > } else { > - for (div = 10; div; div /= 10) > + for (; div; div /= 10) > div_out(info, &num, div); > } > break; > case 'x': > - num = va_arg(va, unsigned int); > + if (islong) { > + num = va_arg(va, unsigned long); > + div = 1UL << (sizeof(long) * 8 - 4); > + } else { > + num = va_arg(va, unsigned int); > +
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
Hi, On 23 November 2016 at 20:19, Siarhei Siamashka wrote: > On Sun, 20 Nov 2016 14:56:59 + > Andre Przywara wrote: > >> tiny-printf does not know about the "l" modifier so far, which breaks >> the crash dump on AArch64, because it uses %lx to print the registers. >> Add an easy way of handling longs correctly. > > I can't help but notice that the changes of this kind are in a way > defeating the original purpose of tiny-printf. And it is surely not > the first patch adding features to tiny-printf. I guess, in the end > we may end up with a large and bloated printf implementation again :-) > > A possible solution might be just a strict check when parsing format > modifiers and abort with an error message (yeah, this will introduce > some size increase, but hopefully the last one). This way we > acknowledge the fact that tiny-printf is a reduced incomplete > implementation, and that the callers need to take this into account. > > As for the "l" modifier. How much does it add to the code size? IMHO > this information should be mentioned in the commit message. Can the > AArch64 crash dump code be modified to avoid using it? Or can we have > the "l" modifier supported on 64-bit platforms only? > >> Signed-off-by: Andre Przywara >> --- >> lib/tiny-printf.c | 43 +-- >> 1 file changed, 33 insertions(+), 10 deletions(-) I think I tested this patch as adding 36 bytes on Thumb2 so not too terrible. But I do agree with the sentiment. Why is aarch64 using tiny-printf? Surely all though chips have heaps of space?! Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
On 24/11/16 03:19, Siarhei Siamashka wrote: > On Sun, 20 Nov 2016 14:56:59 + > Andre Przywara wrote: > >> tiny-printf does not know about the "l" modifier so far, which breaks >> the crash dump on AArch64, because it uses %lx to print the registers. >> Add an easy way of handling longs correctly. > > I can't help but notice that the changes of this kind are in a way > defeating the original purpose of tiny-printf. And it is surely not > the first patch adding features to tiny-printf. I guess, in the end > we may end up with a large and bloated printf implementation again :-) While I appreciate the fight against bloat, I am not sure severely hacked or crippled code is much better. We are not talking about KBs here, it's probably only a small number of double digits bytes. Frankly our existing tiny-printf implementation apparently did not live fully up to its promise of replacing printf with a smaller implementation. It's just that the missing code coverage has hidden this so far. So actually we would need to add this code increase here to the original size comparison. In the end we can't really simplify the code beyond a certain point - otherwise return 0; would be an even smaller implementation. But see below ... > A possible solution might be just a strict check when parsing format > modifiers and abort with an error message (yeah, this will introduce > some size increase, but hopefully the last one). This way we > acknowledge the fact that tiny-printf is a reduced incomplete > implementation, and that the callers need to take this into account. On 64-bit we need "l" to differentiate between 32-bit and 64-bit variables. I believe the crash dump code is shared between SPL and U-Boot proper, and we probably want to keep it that way. > As for the "l" modifier. How much does it add to the code size? IMHO > this information should be mentioned in the commit message. Yeah, good point. I will add the numbers. > Can the AArch64 crash dump code be modified to avoid using it? I really don't want to go there. > Or can we have > the "l" modifier supported on 64-bit platforms only? That sounds more like an option. On 32-bit "l" is pretty useless, and we don't need "ll", which I consider a reasonable limitation. We could just ignore "l", like we do with "-". But on 64-bit that's the way to differentiate between standard integers and addresses (aka longs), and we need that there. I'd rather avoid #ifdefs inside the routine, so I'd try Alex' suggestion of adding " && sizeof(long) > 4" to let the compiler optimize this away. Or I refactor this code into a separate (ifdef'ed) function. Let me check. Cheers, Andre. > >> Signed-off-by: Andre Przywara >> --- >> lib/tiny-printf.c | 43 +-- >> 1 file changed, 33 insertions(+), 10 deletions(-) >> >> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c >> index 30ac759..b01099d 100644 >> --- a/lib/tiny-printf.c >> +++ b/lib/tiny-printf.c >> @@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt) >> info->zs = 1; >> } >> >> -static void div_out(struct printf_info *info, unsigned int *num, >> -unsigned int div) >> +static void div_out(struct printf_info *info, unsigned long *num, >> +unsigned long div) >> { >> unsigned char dgt = 0; >> >> @@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char *fmt, >> va_list va) >> { >> char ch; >> char *p; >> -unsigned int num; >> +unsigned long num; >> char buf[12]; >> -unsigned int div; >> +unsigned long div; >> >> while ((ch = *(fmt++))) { >> if (ch != '%') { >> @@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char *fmt, >> va_list va) >> } else { >> bool lz = false; >> int width = 0; >> +bool islong = false; >> >> ch = *(fmt++); >> +if (ch == '-') >> +ch = *(fmt++); >> + >> if (ch == '0') { >> ch = *(fmt++); >> lz = 1; >> @@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char *fmt, >> va_list va) >> ch = *fmt++; >> } >> } >> +if (ch == 'l') { >> +ch = *(fmt++); >> +islong = true; >> +} >> + >> info->bf = buf; >> p = info->bf; >> info->zs = 0; >> @@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char *fmt, >> va_list va) >> goto abort; >> case 'u': >> case 'd': >> -num = va_arg(va, unsigned int); >> -if (ch == 'd' && (
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
On 27/11/16 17:02, Simon Glass wrote: Hi, > On 23 November 2016 at 20:19, Siarhei Siamashka > wrote: >> On Sun, 20 Nov 2016 14:56:59 + >> Andre Przywara wrote: >> >>> tiny-printf does not know about the "l" modifier so far, which breaks >>> the crash dump on AArch64, because it uses %lx to print the registers. >>> Add an easy way of handling longs correctly. >> >> I can't help but notice that the changes of this kind are in a way >> defeating the original purpose of tiny-printf. And it is surely not >> the first patch adding features to tiny-printf. I guess, in the end >> we may end up with a large and bloated printf implementation again :-) >> >> A possible solution might be just a strict check when parsing format >> modifiers and abort with an error message (yeah, this will introduce >> some size increase, but hopefully the last one). This way we >> acknowledge the fact that tiny-printf is a reduced incomplete >> implementation, and that the callers need to take this into account. >> >> As for the "l" modifier. How much does it add to the code size? IMHO >> this information should be mentioned in the commit message. Can the >> AArch64 crash dump code be modified to avoid using it? Or can we have >> the "l" modifier supported on 64-bit platforms only? >> >>> Signed-off-by: Andre Przywara >>> --- >>> lib/tiny-printf.c | 43 +-- >>> 1 file changed, 33 insertions(+), 10 deletions(-) > > I think I tested this patch as adding 36 bytes on Thumb2 so not too > terrible. But I do agree with the sentiment. Thanks for checking that! > Why is aarch64 using tiny-printf? Surely all though chips have heaps of > space?! Ha, one would hope so, right? But in fact this is basically an existing 32-bit Allwinner chip with 64-bit cores - mostly because they can ;-). Replacing Cortex-A7 cores with A53s seems to be a common exercise. But the point is that even the most capable chip needs to be booted somehow, and here the Allwinner boot ROM still loads only 32KB into some SRAM. This hasn't changed for years, so even the 64-bit chips suffer from the same SPL space limitations. And since AArch64 does not define a tight encoding variant like Thumb, we are even more limited in our code size. Of course this only applies to the SPL, so once we have DRAM up and an MMC driver initialized, we indeed have quite some resources available. Cheers, Andre. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
On 28/11/16 00:22, André Przywara wrote: > On 27/11/16 17:02, Simon Glass wrote: > > Hi, > >> On 23 November 2016 at 20:19, Siarhei Siamashka >> wrote: >>> On Sun, 20 Nov 2016 14:56:59 + >>> Andre Przywara wrote: >>> tiny-printf does not know about the "l" modifier so far, which breaks the crash dump on AArch64, because it uses %lx to print the registers. Add an easy way of handling longs correctly. >>> >>> I can't help but notice that the changes of this kind are in a way >>> defeating the original purpose of tiny-printf. And it is surely not >>> the first patch adding features to tiny-printf. I guess, in the end >>> we may end up with a large and bloated printf implementation again :-) >>> >>> A possible solution might be just a strict check when parsing format >>> modifiers and abort with an error message (yeah, this will introduce >>> some size increase, but hopefully the last one). This way we >>> acknowledge the fact that tiny-printf is a reduced incomplete >>> implementation, and that the callers need to take this into account. >>> >>> As for the "l" modifier. How much does it add to the code size? IMHO >>> this information should be mentioned in the commit message. Can the >>> AArch64 crash dump code be modified to avoid using it? Or can we have >>> the "l" modifier supported on 64-bit platforms only? >>> Signed-off-by: Andre Przywara --- lib/tiny-printf.c | 43 +-- 1 file changed, 33 insertions(+), 10 deletions(-) >> >> I think I tested this patch as adding 36 bytes on Thumb2 so not too >> terrible. But I do agree with the sentiment. Simon, what is your compiler? Both with GCC 5.3.0 and GCC 6.2.0 I get exactly 6/4 bytes more of .text, which is not too bad for parsing (but ignoring) two new modifiers. It turns out that (at least these two versions of) GCCs are quite clever here and optimize away almost everything. Looking closer one can see that the if and else branches become identical if sizeof(long) == sizeof(int) == 4, so the compiler happily merges the code, removes the "if (long)" check and in turn the whole long-handling code on 32-bit. This is the patch as sent, without any further hints in the code. If anyone really wants to save code space, I suggest to switch to a later compiler: GCC 5.3.0: textdata bss dec hex filename origin/master orangepi_plus_defconfig GCC 5.3.0 18881 488 232 196014c91 spl/u-boot-spl 758 0 0 758 2f6 spl/lib/tiny-printf.o master+tiny_printf %l,%- orangepi_plus_defconfig GCC 5.3.0 18887 488 232 196074c97 spl/u-boot-spl 758 0 0 758 2f6 spl/lib/tiny-printf.o GCC 6.2.0: origin/master orangepi_plus_defconfig GCC 6.2.0 16871 488 232 1759144b7 spl/u-boot-spl 698 0 0 698 2ba spl/lib/tiny-printf.o master+tiny_printf %l,%- orangepi_plus_defconfig GCC 6.2.0 16875 488 232 1759544bb spl/u-boot-spl 702 0 0 702 2be spl/lib/tiny-printf.o On 64-bit (only GCC 6.2.0) this results in more code, as expected: HEAD of patch set w/o tiny-printf %l, pine64_plus_defconfig 25824 392 360 2657667d0 spl/u-boot-spl 1542 0 01542 606 spl/lib/tiny-printf.o HEAD of patch set, pine64_plus_defconfig 25972 392 360 267246864 spl/u-boot-spl 1690 0 01690 69a spl/lib/tiny-printf.o So this is 148 Bytes more in .text. I can trade three simple patches that cut off 80 Bytes in sunxi/armv8 to at least offset this a bit, though this isn't really a regression, as there was no SPL64 for sunxi before. So apart from Alex' bug fix I won't change the patch, if people can live with that. Cheers, Andre. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier
Hi Andre, On 28 November 2016 at 18:13, André Przywara wrote: > On 28/11/16 00:22, André Przywara wrote: >> On 27/11/16 17:02, Simon Glass wrote: >> >> Hi, >> >>> On 23 November 2016 at 20:19, Siarhei Siamashka >>> wrote: On Sun, 20 Nov 2016 14:56:59 + Andre Przywara wrote: > tiny-printf does not know about the "l" modifier so far, which breaks > the crash dump on AArch64, because it uses %lx to print the registers. > Add an easy way of handling longs correctly. I can't help but notice that the changes of this kind are in a way defeating the original purpose of tiny-printf. And it is surely not the first patch adding features to tiny-printf. I guess, in the end we may end up with a large and bloated printf implementation again :-) A possible solution might be just a strict check when parsing format modifiers and abort with an error message (yeah, this will introduce some size increase, but hopefully the last one). This way we acknowledge the fact that tiny-printf is a reduced incomplete implementation, and that the callers need to take this into account. As for the "l" modifier. How much does it add to the code size? IMHO this information should be mentioned in the commit message. Can the AArch64 crash dump code be modified to avoid using it? Or can we have the "l" modifier supported on 64-bit platforms only? > Signed-off-by: Andre Przywara > --- > lib/tiny-printf.c | 43 +-- > 1 file changed, 33 insertions(+), 10 deletions(-) >>> >>> I think I tested this patch as adding 36 bytes on Thumb2 so not too >>> terrible. But I do agree with the sentiment. > > Simon, what is your compiler? 4.9 I suspect for that test. I build with various ones as I have been caught by breaking a slightly older compiler. > Both with GCC 5.3.0 and GCC 6.2.0 I get exactly 6/4 bytes more of .text, > which is not too bad for parsing (but ignoring) two new modifiers. > It turns out that (at least these two versions of) GCCs are quite clever > here and optimize away almost everything. > Looking closer one can see that the if and else branches become > identical if sizeof(long) == sizeof(int) == 4, so the compiler happily > merges the code, removes the "if (long)" check and in turn the whole > long-handling code on 32-bit. > This is the patch as sent, without any further hints in the code. > > If anyone really wants to save code space, I suggest to switch to a > later compiler: > > GCC 5.3.0: > textdata bss dec hex filename > origin/master orangepi_plus_defconfig GCC 5.3.0 > 18881 488 232 196014c91 spl/u-boot-spl > 758 0 0 758 2f6 spl/lib/tiny-printf.o > > master+tiny_printf %l,%- orangepi_plus_defconfig GCC 5.3.0 > 18887 488 232 196074c97 spl/u-boot-spl > 758 0 0 758 2f6 spl/lib/tiny-printf.o > > GCC 6.2.0: > origin/master orangepi_plus_defconfig GCC 6.2.0 > 16871 488 232 1759144b7 spl/u-boot-spl > 698 0 0 698 2ba spl/lib/tiny-printf.o > > master+tiny_printf %l,%- orangepi_plus_defconfig GCC 6.2.0 > 16875 488 232 1759544bb spl/u-boot-spl > 702 0 0 702 2be spl/lib/tiny-printf.o > > On 64-bit (only GCC 6.2.0) this results in more code, as expected: > HEAD of patch set w/o tiny-printf %l, pine64_plus_defconfig > 25824 392 360 2657667d0 spl/u-boot-spl > 1542 0 01542 606 spl/lib/tiny-printf.o > HEAD of patch set, pine64_plus_defconfig > 25972 392 360 267246864 spl/u-boot-spl > 1690 0 01690 69a spl/lib/tiny-printf.o > > So this is 148 Bytes more in .text. I can trade three simple patches > that cut off 80 Bytes in sunxi/armv8 to at least offset this a bit, > though this isn't really a regression, as there was no SPL64 for sunxi > before. > So apart from Alex' bug fix I won't change the patch, if people can live > with that. That seems fine to me. Also this useful info could go in a note in your patch. > > Cheers, > Andre. > Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot