[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
>> <siarhei.siamashka at gmail.com> wrote:
>>> On Sun, 20 Nov 2016 14:56:59 +0000
>>> Andre Przywara <andre.przywara at arm.com> 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 <andre.przywara at arm.com>
>>>> ---
>>>>  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:
 text    data     bss     dec     hex filename
origin/master   orangepi_plus_defconfig GCC 5.3.0
18881     488     232   19601    4c91 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   19607    4c97 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   17591    44b7 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   17595    44bb 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   26576    67d0 spl/u-boot-spl
 1542       0       0    1542     606 spl/lib/tiny-printf.o
HEAD of patch set, pine64_plus_defconfig
25972     392     360   26724    6864 spl/u-boot-spl
 1690       0       0    1690     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.