[U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier

[Andre Przywara]

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.przywara at 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;
>>