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

[Simon Glass]

Hi Andre,

On 28 November 2016 at 18:13, André Przywara <andre.przywara at arm.com> 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
>>> <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?

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

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

>
> Cheers,
> Andre.
>

Regards,
Simon