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

[Alexander Graf]

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 <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.

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