Hi Andre, On 28 November 2016 at 18:13, André Przywara <andre.przyw...@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.siamas...@gmail.com> wrote: >>>> On Sun, 20 Nov 2016 14:56:59 +0000 >>>> Andre Przywara <andre.przyw...@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.przyw...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot