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 <andre.przyw...@arm.com> >> --- >> 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 = 1000000000; >> + 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 = 1000000000; 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 = 0x10000000; >> + } >> if (!num) { >> out_dgt(info, 0); >> } else { >> - for (div = 0x10000000; div; div /= 0x10) >> + for (; div; div /= 0x10) >> div_out(info, &num, div); >> } >> break; >> _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot