[PATCH] tiny-printf: Support %i

[Marek Vasut]

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 ?