[PATCH] lib: sbi: Fix printf handling of long long

[Jessica Clarke]

On 27 Jul 2022, at 15:32, Andrew Jones <ajones at ventanamicro.com> wrote:
> 
> On Wed, Jul 27, 2022 at 10:07:39PM +0800, dramforever wrote:
>> Do not attempt to unnecessarily align va_list before reading long long
>> arguments.
> 
> Can you please elaborate on why it was presumed necessary once, but is now
> deemed not necessary?

Because of this nonsense that follows:

	if (sizeof(unsigned long long) ==
			sizeof(unsigned long)) {
		tmp = va_arg(args, unsigned long long);
		acnt += sizeof(unsigned long long);
	} else {
		((unsigned long *)&tmp)[0] =
			va_arg(args, unsigned long);
		((unsigned long *)&tmp)[1] =
			va_arg(args, unsigned long);
		acnt += 2*sizeof(unsigned long);
	}

For RV64 the condition of the while is always false as variadic
arguments are already padded to 8 bytes. For RV32 the implementation of
va_arg will dynamically align the pointer to 8 bytes at run time
(mirroring what this loop does) so it would seem unnecessary, except
for the nonsense that follows. Since unsigned long is only 32-bit, it
ends up taking the else branch and reading two unsigned longs, which
will *not* get the same dynamic alignment from the compiler. So long as
that nonsense also exists the code is needed on RV32. Where this gets
more problematic, though, is on RV64, since acnt does not stay in sync
with how much args has been offset; all variadic arguments are padded
to 8 bytes, but for (unsigned) int the printf implementation here only
increments acnt by sizeof(int). Thus, on RV64, if you pass an odd
number of ints and then a long long the implementation will erroneously
skip an entry, which is presumably what resulted in this being found
and the patch in question.

This should all be changed to just reading an unsigned long long
because that will do the right thing (if not that is a serious compiler
bug), not try to buggily reimplement va_arg ABI lowering.

Note this code has existed right from 9e8ff05cb61f ("Initial commit.”).

Jess

>> Signed-off-by: dramforever <dramforever at live.com>
>> ---
>> lib/sbi/sbi_console.c | 5 -----
>> 1 file changed, 5 deletions(-)
>> 
>> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
>> index 34c843d..7af50e5 100644
>> --- a/lib/sbi/sbi_console.c
>> +++ b/lib/sbi/sbi_console.c
>> @@ -261,11 +261,6 @@ static int print(char **out, u32 *out_len, const char *format, va_list args)
>> 				continue;
>> 			}
>> 			if (*format == 'l' && *(format + 1) == 'l') {
>> -				while (acnt &
>> -				 (sizeof(unsigned long long) - 1)) {
>> -					va_arg(args, int);
>> -					acnt += sizeof(int);
>> -				}
> 
> I think this should generate a "set but not used" warning with clang, as
> this was the only place acnt was getting used. We can just remove all of
> it.
> 
> Thanks,
> drew
> 
>> 				if (sizeof(unsigned long long) ==
>> 				 sizeof(unsigned long)) {
>> 					tmp = va_arg(args, unsigned long long);
>> -- 
>> 2.37.0
>> 
>> 
>> -- 
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi