[PATCH] tiny-printf: Support %i

[Tom Rini]

On Tue, Apr 14, 2020 at 05:22:19PM +0200, Marek Vasut wrote:
> On 4/14/20 4:11 PM, Tom Rini wrote:
> > 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.
> 
> Except the way it is right now, a lot of output is broken in SPL in an
> inobvious way, which makes working with and/or debugging SPL difficult
> and special, compared to U-Boot and other software.

No, it's not "a lot", it's whatever you ran in to.  We don't have a lot
of "%i" usage to start with and even less that's not a debug print and
also in SPL code.

> > 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.
> 
> Sorry, no, "change your formatting strings to cater for our broken
> printf() implementation" really is not the solution here.

Except it is, for the specifically limited printf limitation.  Adapt the
print for the limitation and move on.  It's not even "rework the code
slightly so the print is correct", it's switch to %d.

-- 
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/e7a123e8/attachment.sig>