On 11/10/2019 16.19, David Hildenbrand wrote:
> On 10.09.19 01:11, Bill Wendling wrote:
>> Clang warns that passing an object that undergoes default argument
>> promotion to "va_start" is undefined behavior:
>>
>> lib/report.c:106:15: error: passing an object that undergoes default
>> argument promotion to 'va_start' has undefined behavior
>> [-Werror,-Wvarargs]
>> va_start(va, pass);
>>
>> Using an "unsigned" type removes the need for argument promotion.
>>
>> Signed-off-by: Bill Wendling <morbo@xxxxxxxxxx>
>> ---
>> lib/libcflat.h | 4 ++--
>> lib/report.c | 6 +++---
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/libcflat.h b/lib/libcflat.h
>> index b94d0ac..b6635d9 100644
>> --- a/lib/libcflat.h
>> +++ b/lib/libcflat.h
>> @@ -99,9 +99,9 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
>> __attribute__((format(printf, 1, 2)));
>> extern void report_prefix_push(const char *prefix);
>> extern void report_prefix_pop(void);
>> -extern void report(const char *msg_fmt, bool pass, ...)
>> +extern void report(const char *msg_fmt, unsigned pass, ...)
>> __attribute__((format(printf, 1, 3)));
>> -extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
>> +extern void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...)
>> __attribute__((format(printf, 1, 4)));
>> extern void report_abort(const char *msg_fmt, ...)
>> __attribute__((format(printf, 1, 2)))
>> diff --git a/lib/report.c b/lib/report.c
>> index ca9b4fd..7d259f6 100644
>> --- a/lib/report.c
>> +++ b/lib/report.c
>> @@ -81,7 +81,7 @@ void report_prefix_pop(void)
>> }
>>
>> static void va_report(const char *msg_fmt,
>> - bool pass, bool xfail, bool skip, va_list va)
>> + unsigned pass, bool xfail, bool skip, va_list va)
>> {
>> const char *prefix = skip ? "SKIP"
>> : xfail ? (pass ? "XPASS" : "XFAIL")
>> @@ -104,7 +104,7 @@ static void va_report(const char *msg_fmt,
>> spin_unlock(&lock);
>> }
>>
>> -void report(const char *msg_fmt, bool pass, ...)
>> +void report(const char *msg_fmt, unsigned pass, ...)
>> {
>> va_list va;
>> va_start(va, pass);
>> @@ -112,7 +112,7 @@ void report(const char *msg_fmt, bool pass, ...)
>> va_end(va);
>> }
>>
>> -void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
>> +void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...)
>> {
>> va_list va;
>> va_start(va, pass);
>>
>
> This patch breaks the selftest on s390x:
>
> t460s: ~/git/kvm-unit-tests (kein Branch, Rebase von Branch master im Gange) $ ./run_tests.sh
> FAIL selftest-setup (14 tests, 2 unexpected failures)
>
> t460s: ~/git/kvm-unit-tests (kein Branch, Rebase von Branch master im Gange) $ cat logs/selftest-setup.log
> timeout -k 1s --foreground 90s /usr/bin/qemu-system-s390x -nodefaults -nographic -machine s390-ccw-virtio,accel=tcg -chardev stdio,id=con0 -device sclpconsole,chardev=con0 -kernel s390x/selftest.elf -smp 1 -append test 123 # -initrd /tmp/tmp.JwIjS9RWlv
> PASS: selftest: true
> PASS: selftest: argc == 3
> PASS: selftest: argv[0] == PROGNAME
> PASS: selftest: argv[1] == test
> PASS: selftest: argv[2] == 123
> PASS: selftest: 3.0/2.0 == 1.5
> PASS: selftest: Program interrupt: expected(1) == received(1)
> PASS: selftest: Program interrupt: expected(5) == received(5)
> FAIL: selftest: malloc: got vaddr
> PASS: selftest: malloc: access works
> FAIL: selftest: malloc: got 2nd vaddr
> PASS: selftest: malloc: access works
> PASS: selftest: malloc: addresses differ
> PASS: selftest: Program interrupt: expected(5) == received(5)
> SUMMARY: 14 tests, 2 unexpected failures
>
> EXIT: STATUS=3
>
>
>
> A fix for the test would look like this:
>
> diff --git a/s390x/selftest.c b/s390x/selftest.c
> index f4acdc4..dc1c476 100644
> --- a/s390x/selftest.c
> +++ b/s390x/selftest.c
> @@ -49,9 +49,10 @@ static void test_malloc(void)
> *tmp2 = 123456789;
> mb();
>
> - report("malloc: got vaddr", (uintptr_t)tmp & 0xf000000000000000ul);
> + report("malloc: got vaddr", !!((uintptr_t)tmp & 0xf000000000000000ul));
> report("malloc: access works", *tmp == 123456789);
> - report("malloc: got 2nd vaddr", (uintptr_t)tmp2 & 0xf000000000000000ul);
> + report("malloc: got 2nd vaddr",
> + !!((uintptr_t)tmp2 & 0xf000000000000000ul));
> report("malloc: access works", (*tmp2 == 123456789));
> report("malloc: addresses differ", tmp != tmp2);
>
>
> But I am not sure if that is the right fix.
>
> (why don't we run sanity tests to detect that, this tests works
> just fine with s390x TCG)
This patch also broke the test_64bit() function in powerpc/emulator.c:
https://gitlab.com/huth/kvm-unit-tests/-/jobs/318694752
Thomas
