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

[Alexander Graf]

On 20/11/2016 15:56, Andre Przywara 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.
>
> 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++);
> +

What does this do? I don't see '-' mentioned in the patch description.

>  			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) {

Check here if sizeof(long) > 4, so that the whole branch gets optimized 
away on 32bit.

> +					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;

Num is a long now and before. So if you have a 32bit signed input, it 
will sign extend incorrectly here. You need an additional check

   if (islong)
     num = -(long)num;
   else
     num = -(int)num;

Let's hope the compiler on 32bit is smart enough to know that it can 
combine those two cases :).

>  					out(info, '-');
>  				}
>  				if (!num) {
>  					out_dgt(info, 0);
>  				} else {
> -					for (div = 1000000000; div; div /= 10)
> +					for (; div; div /= 10)

Any particular reason for that change?

>  						div_out(info, &num, div);
>  				}
>  				break;
>  			case 'x':
> -				num = va_arg(va, unsigned int);
> +				if (islong) {

Same comment as above.


Alex

> +					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;
>