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 andre.przywara@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;