On 11/10/2019 18.24, Thomas Huth wrote:
> 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
... and I think it even broke the intel_iommu test:
https://gitlab.com/huth/kvm-unit-tests/-/jobs/318694755
https://travis-ci.com/huth/kvm-unit-tests/jobs/244827719#L1087
... why did nobody notice at least that one?
(I strongly like to recommend to run either Travis or gitlab-ci on
changes in the common lib/ directory first, to see whether it breaks
anything for the other architectures)
Thomas
