[U-Boot] [PATCH 05/24] SPL: tiny-printf: add "l" modifier

[André Przywara]

On 24/11/16 03:19, Siarhei Siamashka wrote:
> On Sun, 20 Nov 2016 14:56:59 +0000
> Andre Przywara <andre.przywara at 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 :-)

While I appreciate the fight against bloat, I am not sure severely
hacked or crippled code is much better. We are not talking about KBs
here, it's probably only a  small number of double digits bytes.
Frankly our existing tiny-printf implementation apparently did not live
fully up to its promise of replacing printf with a smaller
implementation. It's just that the missing code coverage has hidden this
so far. So actually we would need to add this code increase here to the
original size comparison.

In the end we can't really simplify the code beyond a certain point -
otherwise return 0; would be an even smaller implementation.

But see below ...

> 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.

On 64-bit we need "l" to differentiate between 32-bit and 64-bit variables.
I believe the crash dump code is shared between SPL and U-Boot proper,
and we probably want to keep it that way.

> As for the "l" modifier. How much does it add to the code size? IMHO
> this information should be mentioned in the commit message.

Yeah, good point. I will add the numbers.

> Can the AArch64 crash dump code be modified to avoid using it?

I really don't want to go there.

> Or can we have
> the "l" modifier supported on 64-bit platforms only?

That sounds more like an option. On 32-bit "l" is pretty useless, and we
don't need "ll", which I consider a reasonable limitation.
We could just ignore "l", like we do with "-".

But on 64-bit that's the way to differentiate between standard integers
and addresses (aka longs), and we need that there.
I'd rather avoid #ifdefs inside the routine, so I'd try Alex' suggestion
of adding " && sizeof(long) > 4" to let the compiler optimize this away.
Or I refactor this code into a separate (ifdef'ed) function.

Let me check.

Cheers,
Andre.

> 
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>>  lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
>>  1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
>> index 30ac759..b01099d 100644
>> --- a/lib/tiny-printf.c
>> +++ b/lib/tiny-printf.c
>> @@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt)
>>  	info->zs = 1;
>>  }
>>  
>> -static void div_out(struct printf_info *info, unsigned int *num,
>> -		    unsigned int div)
>> +static void div_out(struct printf_info *info, unsigned long *num,
>> +		    unsigned long div)
>>  {
>>  	unsigned char dgt = 0;
>>  
>> @@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
>>  {
>>  	char ch;
>>  	char *p;
>> -	unsigned int num;
>> +	unsigned long num;
>>  	char buf[12];
>> -	unsigned int div;
>> +	unsigned long div;
>>  
>>  	while ((ch = *(fmt++))) {
>>  		if (ch != '%') {
>> @@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
>>  		} else {
>>  			bool lz = false;
>>  			int width = 0;
>> +			bool islong = false;
>>  
>>  			ch = *(fmt++);
>> +			if (ch == '-')
>> +				ch = *(fmt++);
>> +
>>  			if (ch == '0') {
>>  				ch = *(fmt++);
>>  				lz = 1;
>> @@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
>>  					ch = *fmt++;
>>  				}
>>  			}
>> +			if (ch == 'l') {
>> +				ch = *(fmt++);
>> +				islong = true;
>> +			}
>> +
>>  			info->bf = buf;
>>  			p = info->bf;
>>  			info->zs = 0;
>> @@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
>>  				goto abort;
>>  			case 'u':
>>  			case 'd':
>> -				num = va_arg(va, unsigned int);
>> -				if (ch == 'd' && (int)num < 0) {
>> -					num = -(int)num;
>> +				div = 1000000000;
>> +				if (islong) {
>> +					num = va_arg(va, unsigned long);
>> +					if (sizeof(long) > 4)
>> +						div *= div * 10;
>> +				} else {
>> +					num = va_arg(va, unsigned int);
>> +				}
>> +
>> +				if (ch == 'd' && (long)num < 0) {
>> +					num = -(long)num;
>>  					out(info, '-');
>>  				}
>>  				if (!num) {
>>  					out_dgt(info, 0);
>>  				} else {
>> -					for (div = 1000000000; div; div /= 10)
>> +					for (; div; div /= 10)
>>  						div_out(info, &num, div);
>>  				}
>>  				break;
>>  			case 'x':
>> -				num = va_arg(va, unsigned int);
>> +				if (islong) {
>> +					num = va_arg(va, unsigned long);
>> +					div = 1UL << (sizeof(long) * 8 - 4);
>> +				} else {
>> +					num = va_arg(va, unsigned int);
>> +					div = 0x10000000;
>> +				}
>>  				if (!num) {
>>  					out_dgt(info, 0);
>>  				} else {
>> -					for (div = 0x10000000; div; div /= 0x10)
>> +					for (; div; div /= 0x10)
>>  						div_out(info, &num, div);
>>  				}
>>  				break;
> 
> 
>