On 4/15/20 1:01 AM, Tom Rini wrote:
> 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 <ma...@denx.de>
>>>>>>>>>> Cc: Simon Glass <s...@chromium.org>
>>>>>>>>>> Cc: Stefan Roese <s...@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 it prevents any sort of sane code reuse, unless you want to spam the
code with #ifdef TINY_PRINTF.
>>> 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.
See above.
