[PATCH] tiny-printf: Support %i

[Tom Rini]

On Tue, Apr 14, 2020 at 03:40:14PM +0200, Marek Vasut wrote:
> On 4/14/20 3:26 PM, Tom Rini wrote:
> > On Tue, Apr 14, 2020 at 02:24:18PM +0200, Marek Vasut wrote:
> >> On 4/14/20 5:03 AM, Tom Rini wrote:
> >>> On Tue, Apr 14, 2020 at 03:17:16AM +0200, Marek Vasut wrote:
> >>>> On 4/14/20 1:27 AM, Tom Rini wrote:
> >>>>> On Fri, Apr 10, 2020 at 08:54:49PM +0200, Marek Vasut wrote:
> >>>>>
> >>>>>> The most basic printf("%i", value) formating string was missing,
> >>>>>> add it for the sake of convenience.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>>>> Cc: Simon Glass <sjg at chromium.org>
> >>>>>> Cc: Stefan Roese <sr at denx.de>
> >>>>>> ---
> >>>>>>  lib/tiny-printf.c | 3 ++-
> >>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> >>>>>> index 1138c7012a..8fc7e48d99 100644
> >>>>>> --- a/lib/tiny-printf.c
> >>>>>> +++ b/lib/tiny-printf.c
> >>>>>> @@ -242,6 +242,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
> >>>>>>  				goto abort;
> >>>>>>  			case 'u':
> >>>>>>  			case 'd':
> >>>>>> +			case 'i':
> >>>>>>  				div = 1000000000;
> >>>>>>  				if (islong) {
> >>>>>>  					num = va_arg(va, unsigned long);
> >>>>>> @@ -251,7 +252,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
> >>>>>>  					num = va_arg(va, unsigned int);
> >>>>>>  				}
> >>>>>>  
> >>>>>> -				if (ch == 'd') {
> >>>>>> +				if (ch != 'u') {
> >>>>>>  					if (islong && (long)num < 0) {
> >>>>>>  						num = -(long)num;
> >>>>>>  						out(info, '-');
> >>>>>
> >>>>> How much does the size change and where do we see this as a problem?
> >>>>
> >>>> Any code which uses %i in SPL just misbehaves, e.g.
> >>>> printf("%s[%i] value=%x", __func__, __LINE__, val);
> >>>> prints function name and then incorrect value, because %i is ignored.
> >>>> This is also documented in the commit message.
> >>>>
> >>>> U-Boot grows in size massively due to all the DM/DT bloat which is being
> >>>> forced upon everyone, but there the uncontrolled growth is apparently OK
> >>>> even if it brings no obvious improvement, rather the opposite. And yet
> >>>> here, size increase suddenly matters? Sorry, that's not right.
> >>>>
> >>>> The code grows by 6 bytes.
> >>>
> >>> Yes, it matters for _tiny-printf_ as that's where we have little to no
> >>> room for growth.
> >>
> >> How many systems that use tiny-printf in SPL are also forced to use DM
> >> in SPL ?
> > 
> > I don't know how many times I've said no one is forced to switch to DM
> > in SPL.
> 
> This is beside the point, there are boards which use SPL and DM, because
> the non-DM drivers are steadily going away. So the growth in SPL size is
> there, either directly or as a side-effect.
> 
> >>> So it's just debug prints you were doing that ran in
> >>> to a problem?  Thanks!
> >>
> >> git grep %i indicates ~400 sites where %i is used, so no, not just debug
> >> prints. All of those are broken. And no, I'm not inclined to patch all
> >> the code to use %d instead of %i just because handling %i is a problem.
> > 
> > Not all 400 of them but the ones that are expected to be used in SPL and
> > with TINY_PRINTF need to, yes.  Go look at the git log of tiny-printf.c,
> > we've changed things around to avoid growth when at all possible.
> 
> I appreciate that. However, I would also appreciate if printf() behaved
> in a sane manner, and missing %i support is really weird.
> 
> > Because yes, I don't want to grow a few hundred boards by 6 bytes when
> > we have a reasonable alternative.  There's 300 hits, of which a dozen
> > are non-debug and likely to ever be in SPL code.
> 
> Why don't we instead replace %d with %i altogether then ? The %d seems
> to be seldom used outside of U-Boot, where it is only used because of
> this tiny-printf limitation, while %i is used quite often.
> 
> > And no, this isn't the
> > first time I've raised such an issue, it's just the first time you've
> > been hit by this, sorry.
> 
> Does this therefore set a precedent that we are allowed to block any and
> all patches which grow SPL size, no matter how useful they might be ?

This is following the precedent that was set for tiny printf a while ago
with some other "it would be nice if..." format that could instead be
handled differently, again for the case of tiny printf.  It is not
supposed to cover everything, or most things.  It is supposed to let
SPL/TPL still have printf in otherwise very tight situations.

And as a reminder, I throw every PR through a before/after size check
and flag growth that's global and not fixing a bug that can't be fixed
some other way.  Change your prints to %d and fix the problem without a
size change.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200414/ec952317/attachment.sig>