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

[Simon Glass]

Hi Andre,

On 28 November 2016 at 18:13, André Przywara  wrote:
> On 28/11/16 00:22, André Przywara wrote:
>> On 27/11/16 17:02, Simon Glass wrote:
>>
>> Hi,
>>
>>> On 23 November 2016 at 20:19, Siarhei Siamashka
>>>  wrote:
 On Sun, 20 Nov 2016 14:56:59 +
 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.

 I can't help but notice that the changes of this kind are in a way
 defeating the original purpose of tiny-printf. And it is surely not
 the first patch adding features to tiny-printf. I guess, in the end
 we may end up with a large and bloated printf implementation again :-)

 A possible solution might be just a strict check when parsing format
 modifiers and abort with an error message (yeah, this will introduce
 some size increase, but hopefully the last one). This way we
 acknowledge the fact that tiny-printf is a reduced incomplete
 implementation, and that the callers need to take this into account.

 As for the "l" modifier. How much does it add to the code size? IMHO
 this information should be mentioned in the commit message. Can the
 AArch64 crash dump code be modified to avoid using it? Or can we have
 the "l" modifier supported on 64-bit platforms only?

> Signed-off-by: Andre Przywara 
> ---
>  lib/tiny-printf.c | 43 +--
>  1 file changed, 33 insertions(+), 10 deletions(-)
>>>
>>> I think I tested this patch as adding 36 bytes on Thumb2 so not too
>>> terrible. But I do agree with the sentiment.
>
> Simon, what is your compiler?

4.9 I suspect for that test. I build with various ones as I have been
caught by breaking a slightly older compiler.

> Both with GCC 5.3.0 and GCC 6.2.0 I get exactly 6/4 bytes more of .text,
> which is not too bad for parsing (but ignoring) two new modifiers.
> It turns out that (at least these two versions of) GCCs are quite clever
> here and optimize away almost everything.
> Looking closer one can see that the if and else branches become
> identical if sizeof(long) == sizeof(int) == 4, so the compiler happily
> merges the code, removes the "if (long)" check and in turn the whole
> long-handling code on 32-bit.
> This is the patch as sent, without any further hints in the code.
>
> If anyone really wants to save code space, I suggest to switch to a
> later compiler:
>
> GCC 5.3.0:
>  textdata bss dec hex filename
> origin/master   orangepi_plus_defconfig GCC 5.3.0
> 18881 488 232   196014c91 spl/u-boot-spl
>   758   0   0 758 2f6 spl/lib/tiny-printf.o
>
> master+tiny_printf %l,%- orangepi_plus_defconfig GCC 5.3.0
> 18887 488 232   196074c97 spl/u-boot-spl
>   758   0   0 758 2f6 spl/lib/tiny-printf.o
>
> GCC 6.2.0:
> origin/master   orangepi_plus_defconfig GCC 6.2.0
> 16871 488 232   1759144b7 spl/u-boot-spl
>   698   0   0 698 2ba spl/lib/tiny-printf.o
>
> master+tiny_printf %l,%- orangepi_plus_defconfig GCC 6.2.0
> 16875 488 232   1759544bb spl/u-boot-spl
>   702   0   0 702 2be spl/lib/tiny-printf.o
>
> On 64-bit (only GCC 6.2.0) this results in more code, as expected:
> HEAD of patch set w/o tiny-printf %l, pine64_plus_defconfig
> 25824 392 360   2657667d0 spl/u-boot-spl
>  1542   0   01542 606 spl/lib/tiny-printf.o
> HEAD of patch set, pine64_plus_defconfig
> 25972 392 360   267246864 spl/u-boot-spl
>  1690   0   01690 69a spl/lib/tiny-printf.o
>
> So this is 148 Bytes more in .text. I can trade three simple patches
> that cut off 80 Bytes in sunxi/armv8 to at least offset this a bit,
> though this isn't really a regression, as there was no SPL64 for sunxi
> before.
> So apart from Alex' bug fix I won't change the patch, if people can live
> with that.

That seems fine to me. Also this useful info could go in a note in your patch.

>
> Cheers,
> Andre.
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[André Przywara]

On 28/11/16 00:22, André Przywara wrote:
> On 27/11/16 17:02, Simon Glass wrote:
> 
> Hi,
> 
>> On 23 November 2016 at 20:19, Siarhei Siamashka
>>  wrote:
>>> On Sun, 20 Nov 2016 14:56:59 +
>>> 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.
>>>
>>> I can't help but notice that the changes of this kind are in a way
>>> defeating the original purpose of tiny-printf. And it is surely not
>>> the first patch adding features to tiny-printf. I guess, in the end
>>> we may end up with a large and bloated printf implementation again :-)
>>>
>>> A possible solution might be just a strict check when parsing format
>>> modifiers and abort with an error message (yeah, this will introduce
>>> some size increase, but hopefully the last one). This way we
>>> acknowledge the fact that tiny-printf is a reduced incomplete
>>> implementation, and that the callers need to take this into account.
>>>
>>> As for the "l" modifier. How much does it add to the code size? IMHO
>>> this information should be mentioned in the commit message. Can the
>>> AArch64 crash dump code be modified to avoid using it? Or can we have
>>> the "l" modifier supported on 64-bit platforms only?
>>>
 Signed-off-by: Andre Przywara 
 ---
  lib/tiny-printf.c | 43 +--
  1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> I think I tested this patch as adding 36 bytes on Thumb2 so not too
>> terrible. But I do agree with the sentiment.

Simon, what is your compiler?
Both with GCC 5.3.0 and GCC 6.2.0 I get exactly 6/4 bytes more of .text,
which is not too bad for parsing (but ignoring) two new modifiers.
It turns out that (at least these two versions of) GCCs are quite clever
here and optimize away almost everything.
Looking closer one can see that the if and else branches become
identical if sizeof(long) == sizeof(int) == 4, so the compiler happily
merges the code, removes the "if (long)" check and in turn the whole
long-handling code on 32-bit.
This is the patch as sent, without any further hints in the code.

If anyone really wants to save code space, I suggest to switch to a
later compiler:

GCC 5.3.0:
 textdata bss dec hex filename
origin/master   orangepi_plus_defconfig GCC 5.3.0
18881 488 232   196014c91 spl/u-boot-spl
  758   0   0 758 2f6 spl/lib/tiny-printf.o

master+tiny_printf %l,%- orangepi_plus_defconfig GCC 5.3.0
18887 488 232   196074c97 spl/u-boot-spl
  758   0   0 758 2f6 spl/lib/tiny-printf.o

GCC 6.2.0:
origin/master   orangepi_plus_defconfig GCC 6.2.0
16871 488 232   1759144b7 spl/u-boot-spl
  698   0   0 698 2ba spl/lib/tiny-printf.o

master+tiny_printf %l,%- orangepi_plus_defconfig GCC 6.2.0
16875 488 232   1759544bb spl/u-boot-spl
  702   0   0 702 2be spl/lib/tiny-printf.o

On 64-bit (only GCC 6.2.0) this results in more code, as expected:
HEAD of patch set w/o tiny-printf %l, pine64_plus_defconfig
25824 392 360   2657667d0 spl/u-boot-spl
 1542   0   01542 606 spl/lib/tiny-printf.o
HEAD of patch set, pine64_plus_defconfig
25972 392 360   267246864 spl/u-boot-spl
 1690   0   01690 69a spl/lib/tiny-printf.o

So this is 148 Bytes more in .text. I can trade three simple patches
that cut off 80 Bytes in sunxi/armv8 to at least offset this a bit,
though this isn't really a regression, as there was no SPL64 for sunxi
before.
So apart from Alex' bug fix I won't change the patch, if people can live
with that.

Cheers,
Andre.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[André Przywara]

On 27/11/16 17:02, Simon Glass wrote:

Hi,

> On 23 November 2016 at 20:19, Siarhei Siamashka
>  wrote:
>> On Sun, 20 Nov 2016 14:56:59 +
>> 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.
>>
>> I can't help but notice that the changes of this kind are in a way
>> defeating the original purpose of tiny-printf. And it is surely not
>> the first patch adding features to tiny-printf. I guess, in the end
>> we may end up with a large and bloated printf implementation again :-)
>>
>> A possible solution might be just a strict check when parsing format
>> modifiers and abort with an error message (yeah, this will introduce
>> some size increase, but hopefully the last one). This way we
>> acknowledge the fact that tiny-printf is a reduced incomplete
>> implementation, and that the callers need to take this into account.
>>
>> As for the "l" modifier. How much does it add to the code size? IMHO
>> this information should be mentioned in the commit message. Can the
>> AArch64 crash dump code be modified to avoid using it? Or can we have
>> the "l" modifier supported on 64-bit platforms only?
>>
>>> Signed-off-by: Andre Przywara 
>>> ---
>>>  lib/tiny-printf.c | 43 +--
>>>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> I think I tested this patch as adding 36 bytes on Thumb2 so not too
> terrible. But I do agree with the sentiment.

Thanks for checking that!

> Why is aarch64 using tiny-printf? Surely all though chips have heaps of 
> space?!

Ha, one would hope so, right?
But in fact this is basically an existing 32-bit Allwinner chip with
64-bit cores - mostly because they can ;-). Replacing Cortex-A7 cores
with A53s seems to be a common exercise.

But the point is that even the most capable chip needs to be booted
somehow, and here the Allwinner boot ROM still loads only 32KB into some
SRAM. This hasn't changed for years, so even the 64-bit chips suffer
from the same SPL space limitations.
And since AArch64 does not define a tight encoding variant like Thumb,
we are even more limited in our code size.

Of course this only applies to the SPL, so once we have DRAM up and an
MMC driver initialized, we indeed have quite some resources available.

Cheers,
Andre.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[André Przywara]

On 24/11/16 03:19, Siarhei Siamashka wrote:
> On Sun, 20 Nov 2016 14:56:59 +
> 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.
> 
> I can't help but notice that the changes of this kind are in a way
> defeating the original purpose of tiny-printf. And it is surely not
> the first patch adding features to tiny-printf. I guess, in the end
> we may end up with a large and bloated printf implementation again :-)

While I appreciate the fight against bloat, I am not sure severely
hacked or crippled code is much better. We are not talking about KBs
here, it's probably only a  small number of double digits bytes.
Frankly our existing tiny-printf implementation apparently did not live
fully up to its promise of replacing printf with a smaller
implementation. It's just that the missing code coverage has hidden this
so far. So actually we would need to add this code increase here to the
original size comparison.

In the end we can't really simplify the code beyond a certain point -
otherwise return 0; would be an even smaller implementation.

But see below ...

> A possible solution might be just a strict check when parsing format
> modifiers and abort with an error message (yeah, this will introduce
> some size increase, but hopefully the last one). This way we
> acknowledge the fact that tiny-printf is a reduced incomplete
> implementation, and that the callers need to take this into account.

On 64-bit we need "l" to differentiate between 32-bit and 64-bit variables.
I believe the crash dump code is shared between SPL and U-Boot proper,
and we probably want to keep it that way.

> As for the "l" modifier. How much does it add to the code size? IMHO
> this information should be mentioned in the commit message.

Yeah, good point. I will add the numbers.

> Can the AArch64 crash dump code be modified to avoid using it?

I really don't want to go there.

> Or can we have
> the "l" modifier supported on 64-bit platforms only?

That sounds more like an option. On 32-bit "l" is pretty useless, and we
don't need "ll", which I consider a reasonable limitation.
We could just ignore "l", like we do with "-".

But on 64-bit that's the way to differentiate between standard integers
and addresses (aka longs), and we need that there.
I'd rather avoid #ifdefs inside the routine, so I'd try Alex' suggestion
of adding " && sizeof(long) > 4" to let the compiler optimize this away.
Or I refactor this code into a separate (ifdef'ed) function.

Let me check.

Cheers,
Andre.

> 
>> Signed-off-by: Andre Przywara 
>> ---
>>  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++);
>> +
>>  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);

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

[Simon Glass]

Hi,

On 23 November 2016 at 20:19, Siarhei Siamashka
 wrote:
> On Sun, 20 Nov 2016 14:56:59 +
> 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.
>
> I can't help but notice that the changes of this kind are in a way
> defeating the original purpose of tiny-printf. And it is surely not
> the first patch adding features to tiny-printf. I guess, in the end
> we may end up with a large and bloated printf implementation again :-)
>
> A possible solution might be just a strict check when parsing format
> modifiers and abort with an error message (yeah, this will introduce
> some size increase, but hopefully the last one). This way we
> acknowledge the fact that tiny-printf is a reduced incomplete
> implementation, and that the callers need to take this into account.
>
> As for the "l" modifier. How much does it add to the code size? IMHO
> this information should be mentioned in the commit message. Can the
> AArch64 crash dump code be modified to avoid using it? Or can we have
> the "l" modifier supported on 64-bit platforms only?
>
>> Signed-off-by: Andre Przywara 
>> ---
>>  lib/tiny-printf.c | 43 +--
>>  1 file changed, 33 insertions(+), 10 deletions(-)

I think I tested this patch as adding 36 bytes on Thumb2 so not too
terrible. But I do agree with the sentiment.

Why is aarch64 using tiny-printf? Surely all though chips have heaps of space?!

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Siarhei Siamashka]

On Sun, 20 Nov 2016 14:56:59 +
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.

I can't help but notice that the changes of this kind are in a way
defeating the original purpose of tiny-printf. And it is surely not
the first patch adding features to tiny-printf. I guess, in the end
we may end up with a large and bloated printf implementation again :-)

A possible solution might be just a strict check when parsing format
modifiers and abort with an error message (yeah, this will introduce
some size increase, but hopefully the last one). This way we
acknowledge the fact that tiny-printf is a reduced incomplete
implementation, and that the callers need to take this into account.

As for the "l" modifier. How much does it add to the code size? IMHO
this information should be mentioned in the commit message. Can the
AArch64 crash dump code be modified to avoid using it? Or can we have
the "l" modifier supported on 64-bit platforms only?

> Signed-off-by: Andre Przywara 
> ---
>  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++);
> +
>   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 = 10;
> + 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' && (long)num < 0) {
> + num = -(long)num;
>   out(info, '-');
>   }
>   if (!num) {
>   out_dgt(info, 0);
>   } else {
> - for (div = 10; div; div /= 10)
> + for (; div; div /= 10)
>   div_out(info, , 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 {
> + 

Re: [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 
---
 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 = 10;
+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 = 10; 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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 
>> ---
>>  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 = 10;
>> +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 = 10; 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, , 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 = 0x1000;
>> +}
>>  if (!num) {
>>  out_dgt(info, 0);
>>  } else {
>> -for (div = 0x1000; div; div /= 0x10)
>> +for (; div; div /= 0x10)
>>  div_out(info, , div);
>>  }
>>  break;

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

[Alexander Graf]

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


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 = 10;
+   if (islong) {


Check here if sizeof(long) > 4, so that the whole branch gets optimized 
away on 32bit.



+   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 = 10; div; div /= 10)
+   for (; div; div /= 10)


Any particular reason for that change?


div_out(info, , div);
}
break;
case 'x':
-   num = va_arg(va, unsigned int);
+   if (islong) {


Same comment as above.


Alex


+   num = va_arg(va, unsigned long);
+   div = 1UL << (sizeof(long) * 8 - 4);
+   } else {
+   num = va_arg(va, unsigned int);
+   div = 0x1000;
+   }
if (!num) {
out_dgt(info, 0);
} else {
-   for (div = 0x1000; div; div /= 0x10)
+   for (; div; div /= 0x10)
  

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

[Andre Przywara]

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 
---
 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++);
+
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 = 10;
+   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' && (long)num < 0) {
+   num = -(long)num;
out(info, '-');
}
if (!num) {
out_dgt(info, 0);
} else {
-   for (div = 10; div; div /= 10)
+   for (; div; div /= 10)
div_out(info, , 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 = 0x1000;
+   }
if (!num) {
out_dgt(info, 0);
} else {
-   for (div = 0x1000; div; div /= 0x10)
+   for (; div; div /= 0x10)
div_out(info, , div);
}
break;
-- 
2.8.2

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot