| Message ID | 1480902750-839-5-git-send-email-andre.przywara@arm.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On 4 December 2016 at 18:52, Andre Przywara <andre.przywara@arm.com> 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. Also there are printfs > using the '-' modifier, which we choose to ignore for simplicity. > > Using a relatively decent compiler (GCC 5.3.0) this does _not_ increase > the code size of tiny-printf.o for 32-bit builds (where long and int > are actually the same), actually it looses three (ARM Thumb2) instructions > from the actual SPL (numbers for orangepi_plus_defconfig): > text data bss dec hex filename > 758 0 0 758 2f6 spl/lib/tiny-printf.o before > 18839 488 232 19559 4c67 spl/u-boot-spl before > 758 0 0 758 2f6 spl/lib/tiny-printf.o after > 18833 488 232 19553 4c61 spl/u-boot-spl after > > This adds some substantial amount of code to a 64-bit build, though: > (taken after a later commit, which enables the ARM64 SPL build for sunxi) > text data bss dec hex filename > 1542 0 0 1542 606 spl/lib/tiny-printf.o before > 25830 392 360 26582 67d6 spl/u-boot-spl before > 1758 0 0 1758 6de spl/lib/tiny-printf.o after > 26040 392 360 26792 68a8 spl/u-boot-spl after > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > lib/tiny-printf.c | 50 +++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 39 insertions(+), 11 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On Mon, 5 Dec 2016 01:52:11 +0000 Andre Przywara <andre.przywara@arm.com> 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. Also there are printfs > using the '-' modifier, which we choose to ignore for simplicity. If the '-' modifier is so useless in practice, then why don't we just remove it from the format string of the caller rather than making the printf implementation deviate from the expected behaviour? From "man 3 printf": " - The converted value is to be left adjusted on the field boundary. (The default is right justification.) The converted value is padded on the right with blanks, rather than on the left with blanks or zeros. A - overrides a 0 if both are given." Either way, I think that this change needs to be done as a separate patch. Smuggling it as a part of this "l" modifier patch looks rather fishy ;-) > > Using a relatively decent compiler (GCC 5.3.0) this does _not_ increase > the code size of tiny-printf.o for 32-bit builds (where long and int > are actually the same), actually it looses three (ARM Thumb2) instructions > from the actual SPL (numbers for orangepi_plus_defconfig): > text data bss dec hex filename > 758 0 0 758 2f6 spl/lib/tiny-printf.o before > 18839 488 232 19559 4c67 spl/u-boot-spl before > 758 0 0 758 2f6 spl/lib/tiny-printf.o after > 18833 488 232 19553 4c61 spl/u-boot-spl after Very cool :-) > > This adds some substantial amount of code to a 64-bit build, though: > (taken after a later commit, which enables the ARM64 SPL build for sunxi) > text data bss dec hex filename > 1542 0 0 1542 606 spl/lib/tiny-printf.o before > 25830 392 360 26582 67d6 spl/u-boot-spl before > 1758 0 0 1758 6de spl/lib/tiny-printf.o after > 26040 392 360 26792 68a8 spl/u-boot-spl after OK, I guess we have to live with this. One more win for Thumb2 vs. AArch64 though. And thanks for adding these stats to the commit message. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > lib/tiny-printf.c | 50 +++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 39 insertions(+), 11 deletions(-) > > diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c > index 30ac759..dfa8432 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,43 @@ 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; > - out(info, '-'); > + div = 1000000000; > + 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') { > + if (islong && (long)num < 0) { > + num = -(long)num; > + out(info, '-'); > + } else if (!islong && (int)num < 0) { > + num = -(int)num; > + out(info, '-'); > + } > } > if (!num) { > out_dgt(info, 0); > } else { > - for (div = 1000000000; 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 = 0x10000000; > + } > if (!num) { > out_dgt(info, 0); > } else { > - for (div = 0x10000000; div; div /= 0x10) > + for (; div; div /= 0x10) > div_out(info, &num, div); > } > break;
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 30ac759..dfa8432 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,43 @@ 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; - out(info, '-'); + div = 1000000000; + 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') { + if (islong && (long)num < 0) { + num = -(long)num; + out(info, '-'); + } else if (!islong && (int)num < 0) { + num = -(int)num; + out(info, '-'); + } } if (!num) { out_dgt(info, 0); } else { - for (div = 1000000000; 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 = 0x10000000; + } if (!num) { out_dgt(info, 0); } else { - for (div = 0x10000000; div; div /= 0x10) + for (; div; div /= 0x10) div_out(info, &num, div); } break;
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. Also there are printfs using the '-' modifier, which we choose to ignore for simplicity. Using a relatively decent compiler (GCC 5.3.0) this does _not_ increase the code size of tiny-printf.o for 32-bit builds (where long and int are actually the same), actually it looses three (ARM Thumb2) instructions from the actual SPL (numbers for orangepi_plus_defconfig): text data bss dec hex filename 758 0 0 758 2f6 spl/lib/tiny-printf.o before 18839 488 232 19559 4c67 spl/u-boot-spl before 758 0 0 758 2f6 spl/lib/tiny-printf.o after 18833 488 232 19553 4c61 spl/u-boot-spl after This adds some substantial amount of code to a 64-bit build, though: (taken after a later commit, which enables the ARM64 SPL build for sunxi) text data bss dec hex filename 1542 0 0 1542 606 spl/lib/tiny-printf.o before 25830 392 360 26582 67d6 spl/u-boot-spl before 1758 0 0 1758 6de spl/lib/tiny-printf.o after 26040 392 360 26792 68a8 spl/u-boot-spl after Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- lib/tiny-printf.c | 50 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 11 deletions(-)