Message ID | CAGG=3QUL_OrjaWn+gF4z-R8brR2=3661hGk0uUAK2y8Dff7Mvg@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [kvm-unit-tests] lib: use an argument which doesn't require default argument promotion | expand |
On Mon, Sep 9, 2019 at 4:12 PM Bill Wendling <morbo@google.com> 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@google.com> Reviewed-by: Jim Mattson <jmattson@google.com>
On Tue, Sep 10, 2019 at 9:43 AM Jim Mattson <jmattson@google.com> wrote: > > On Mon, Sep 9, 2019 at 4:12 PM Bill Wendling <morbo@google.com> 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@google.com> > Reviewed-by: Jim Mattson <jmattson@google.com>
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@google.com> > --- > 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); > Applied, thanks. Paolo
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@google.com> > --- > 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)
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@google.com> >> --- >> 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
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@google.com> >>> --- >>> 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
I apologize for the breakage. I'm not sure how this escaped me. Here's a proposed fix. Thoughts? commit 5fa1940140fd75a97f3ac5ae2e4de9e1bef645d0 Author: Bill Wendling <morbo@google.com> Date: Fri Oct 11 11:26:03 2019 -0700 Use a status enum for reporting pass/fail Some values passed into "report" as "pass/fail" are larger than the size of the parameter. Use instead a status enum so that the size of the argument no longer matters. diff --git a/lib/libcflat.h b/lib/libcflat.h index b6635d9..8f80a1c 100644 --- a/lib/libcflat.h +++ b/lib/libcflat.h @@ -95,13 +95,22 @@ extern int vsnprintf(char *buf, int size, const char *fmt, va_list va) extern int vprintf(const char *fmt, va_list va) __attribute__((format(printf, 1, 0))); +enum status { PASSED, FAILED }; + +#define STATUS(x) ((x) != 0 ? PASSED : FAILED) + +#define report(msg_fmt, status, ...) \ + report_status(msg_fmt, STATUS(status) __VA_OPT__(,) __VA_ARGS__) +#define report_xfail(msg_fmt, xfail, status, ...) \ + report_xfail_status(msg_fmt, xfail, STATUS(status) __VA_OPT__(,) __VA_ARGS__) + 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, unsigned pass, ...) +extern void report_status(const char *msg_fmt, unsigned pass, ...) __attribute__((format(printf, 1, 3))); -extern void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) +extern void report_xfail_status(const char *msg_fmt, bool xfail, enum status status, ...) __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 2a5f549..4ba2ac0 100644 --- a/lib/report.c +++ b/lib/report.c @@ -80,12 +80,12 @@ void report_prefix_pop(void) spin_unlock(&lock); } -static void va_report(const char *msg_fmt, - bool pass, bool xfail, bool skip, va_list va) +static void va_report(const char *msg_fmt, enum status status, bool xfail, + bool skip, va_list va) { const char *prefix = skip ? "SKIP" - : xfail ? (pass ? "XPASS" : "XFAIL") - : (pass ? "PASS" : "FAIL"); + : xfail ? (status == PASSED ? "XPASS" : "XFAIL") + : (status == PASSED ? "PASS" : "FAIL"); spin_lock(&lock); @@ -96,27 +96,27 @@ static void va_report(const char *msg_fmt, puts("\n"); if (skip) skipped++; - else if (xfail && !pass) + else if (xfail && status == FAILED) xfailures++; - else if (xfail || !pass) + else if (xfail || status == FAILED) failures++; spin_unlock(&lock); } -void report(const char *msg_fmt, unsigned pass, ...) +void report_status(const char *msg_fmt, enum status status, ...) { va_list va; - va_start(va, pass); - va_report(msg_fmt, pass, false, false, va); + va_start(va, status); + va_report(msg_fmt, status, false, false, va); va_end(va); } -void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) +void report_xfail_status(const char *msg_fmt, bool xfail, enum status status, ...) { va_list va; - va_start(va, pass); - va_report(msg_fmt, pass, xfail, false, va); + va_start(va, status); + va_report(msg_fmt, status, xfail, false, va); va_end(va); } On Fri, Oct 11, 2019 at 9:36 AM Thomas Huth <thuth@redhat.com> wrote: > > 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@google.com> > >>> --- > >>> 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
On 11/10/2019 20.36, Bill Wendling wrote: > I apologize for the breakage. I'm not sure how this escaped me. Here's > a proposed fix. Thoughts? > > commit 5fa1940140fd75a97f3ac5ae2e4de9e1bef645d0 > Author: Bill Wendling <morbo@google.com> > Date: Fri Oct 11 11:26:03 2019 -0700 > > Use a status enum for reporting pass/fail > > Some values passed into "report" as "pass/fail" are larger than the > size of the parameter. Use instead a status enum so that the size of the > argument no longer matters. > > diff --git a/lib/libcflat.h b/lib/libcflat.h > index b6635d9..8f80a1c 100644 > --- a/lib/libcflat.h > +++ b/lib/libcflat.h > @@ -95,13 +95,22 @@ extern int vsnprintf(char *buf, int size, const > char *fmt, va_list va) > extern int vprintf(const char *fmt, va_list va) > __attribute__((format(printf, 1, 0))); > > +enum status { PASSED, FAILED }; > + > +#define STATUS(x) ((x) != 0 ? PASSED : FAILED) > + > +#define report(msg_fmt, status, ...) \ > + report_status(msg_fmt, STATUS(status) __VA_OPT__(,) __VA_ARGS__) > +#define report_xfail(msg_fmt, xfail, status, ...) \ > + report_xfail_status(msg_fmt, xfail, STATUS(status) __VA_OPT__(,) __VA_ARGS__) > + > 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, unsigned pass, ...) > +extern void report_status(const char *msg_fmt, unsigned pass, ...) > __attribute__((format(printf, 1, 3))); > -extern void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) > +extern void report_xfail_status(const char *msg_fmt, bool xfail, enum > status status, ...) > __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 2a5f549..4ba2ac0 100644 > --- a/lib/report.c > +++ b/lib/report.c > @@ -80,12 +80,12 @@ void report_prefix_pop(void) > spin_unlock(&lock); > } > > -static void va_report(const char *msg_fmt, > - bool pass, bool xfail, bool skip, va_list va) > +static void va_report(const char *msg_fmt, enum status status, bool xfail, > + bool skip, va_list va) > { > const char *prefix = skip ? "SKIP" > - : xfail ? (pass ? "XPASS" : "XFAIL") > - : (pass ? "PASS" : "FAIL"); > + : xfail ? (status == PASSED ? "XPASS" : "XFAIL") > + : (status == PASSED ? "PASS" : "FAIL"); > > spin_lock(&lock); > > @@ -96,27 +96,27 @@ static void va_report(const char *msg_fmt, > puts("\n"); > if (skip) > skipped++; > - else if (xfail && !pass) > + else if (xfail && status == FAILED) > xfailures++; > - else if (xfail || !pass) > + else if (xfail || status == FAILED) > failures++; > > spin_unlock(&lock); > } > > -void report(const char *msg_fmt, unsigned pass, ...) > +void report_status(const char *msg_fmt, enum status status, ...) > { > va_list va; > - va_start(va, pass); > - va_report(msg_fmt, pass, false, false, va); > + va_start(va, status); > + va_report(msg_fmt, status, false, false, va); > va_end(va); > } > > -void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) > +void report_xfail_status(const char *msg_fmt, bool xfail, enum status > status, ...) > { > va_list va; > - va_start(va, pass); > - va_report(msg_fmt, pass, xfail, false, va); > + va_start(va, status); > + va_report(msg_fmt, status, xfail, false, va); > va_end(va); > } That's certainly a solution... but I wonder whether it might be easier to simply fix the failing tests instead, to make sure that they do not pass a value > sizeof(int) to report() and report_xfail_status() ? Another idea would be to swap the parameters of report() and report_xfail_status() : diff --git a/lib/libcflat.h b/lib/libcflat.h index b94d0ac..d6d1323 100644 --- a/lib/libcflat.h +++ b/lib/libcflat.h @@ -99,10 +99,10 @@ 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, ...) - __attribute__((format(printf, 1, 3))); -extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) - __attribute__((format(printf, 1, 4))); +extern void report(bool pass, const char *msg_fmt, ...) + __attribute__((format(printf, 2, 3))); +extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) + __attribute__((format(printf, 3, 4))); extern void report_abort(const char *msg_fmt, ...) __attribute__((format(printf, 1, 2))) __attribute__((noreturn)); diff --git a/lib/report.c b/lib/report.c index ca9b4fd..2255dc3 100644 --- a/lib/report.c +++ b/lib/report.c @@ -104,18 +104,18 @@ static void va_report(const char *msg_fmt, spin_unlock(&lock); } -void report(const char *msg_fmt, bool pass, ...) +void report(bool pass, const char *msg_fmt, ...) { va_list va; - va_start(va, pass); + va_start(va, msg_fmt); va_report(msg_fmt, pass, false, false, va); va_end(va); } -void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) +void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) { va_list va; - va_start(va, pass); + va_start(va, msg_fmt); va_report(msg_fmt, pass, xfail, false, va); va_end(va); } ... then we can keep the "bool" - but we have to fix all calling sites, too. Paolo, any preferences? Thomas
> That's certainly a solution... but I wonder whether it might be easier > to simply fix the failing tests instead, to make sure that they do not > pass a value > sizeof(int) to report() and report_xfail_status() ? I really don't like that. Like if assert() would suddenly ignore anything above (32bit) - If this is the case then I shall be silent :D > > Another idea would be to swap the parameters of report() and > report_xfail_status() : > > diff --git a/lib/libcflat.h b/lib/libcflat.h > index b94d0ac..d6d1323 100644 > --- a/lib/libcflat.h > +++ b/lib/libcflat.h > @@ -99,10 +99,10 @@ 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, ...) > - __attribute__((format(printf, 1, > 3))); > -extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) > - __attribute__((format(printf, 1, > 4))); > +extern void report(bool pass, const char *msg_fmt, ...) > + __attribute__((format(printf, 2, > 3))); > +extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) > + __attribute__((format(printf, 3, > 4))); > extern void report_abort(const char *msg_fmt, ...) > __attribute__((format(printf, 1, > 2))) > __attribute__((noreturn)); > diff --git a/lib/report.c b/lib/report.c > index ca9b4fd..2255dc3 100644 > --- a/lib/report.c > +++ b/lib/report.c > @@ -104,18 +104,18 @@ static void va_report(const char *msg_fmt, > spin_unlock(&lock); > } > > -void report(const char *msg_fmt, bool pass, ...) > +void report(bool pass, const char *msg_fmt, ...) > { > va_list va; > - va_start(va, pass); > + va_start(va, msg_fmt); > va_report(msg_fmt, pass, false, false, va); > va_end(va); > } > > -void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) > +void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) > { > va_list va; > - va_start(va, pass); > + va_start(va, msg_fmt); > va_report(msg_fmt, pass, xfail, false, va); > va_end(va); > } > > ... then we can keep the "bool" - but we have to fix all calling sites, too. At least for my taste, please do keep the bool. This sounds like one way to do it. > > Paolo, any preferences? > > Thomas >
On Mon, Oct 14, 2019 at 12:57 AM Thomas Huth <thuth@redhat.com> wrote: > > On 11/10/2019 20.36, Bill Wendling wrote: > > I apologize for the breakage. I'm not sure how this escaped me. Here's > > a proposed fix. Thoughts? > > > > commit 5fa1940140fd75a97f3ac5ae2e4de9e1bef645d0 > > Author: Bill Wendling <morbo@google.com> > > Date: Fri Oct 11 11:26:03 2019 -0700 > > > > Use a status enum for reporting pass/fail > > > > Some values passed into "report" as "pass/fail" are larger than the > > size of the parameter. Use instead a status enum so that the size of the > > argument no longer matters. > > > > diff --git a/lib/libcflat.h b/lib/libcflat.h > > index b6635d9..8f80a1c 100644 > > --- a/lib/libcflat.h > > +++ b/lib/libcflat.h > > @@ -95,13 +95,22 @@ extern int vsnprintf(char *buf, int size, const > > char *fmt, va_list va) > > extern int vprintf(const char *fmt, va_list va) > > __attribute__((format(printf, 1, 0))); > > > > +enum status { PASSED, FAILED }; > > + > > +#define STATUS(x) ((x) != 0 ? PASSED : FAILED) > > + > > +#define report(msg_fmt, status, ...) \ > > + report_status(msg_fmt, STATUS(status) __VA_OPT__(,) __VA_ARGS__) > > +#define report_xfail(msg_fmt, xfail, status, ...) \ > > + report_xfail_status(msg_fmt, xfail, STATUS(status) __VA_OPT__(,) __VA_ARGS__) > > + > > 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, unsigned pass, ...) > > +extern void report_status(const char *msg_fmt, unsigned pass, ...) > > __attribute__((format(printf, 1, 3))); > > -extern void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) > > +extern void report_xfail_status(const char *msg_fmt, bool xfail, enum > > status status, ...) > > __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 2a5f549..4ba2ac0 100644 > > --- a/lib/report.c > > +++ b/lib/report.c > > @@ -80,12 +80,12 @@ void report_prefix_pop(void) > > spin_unlock(&lock); > > } > > > > -static void va_report(const char *msg_fmt, > > - bool pass, bool xfail, bool skip, va_list va) > > +static void va_report(const char *msg_fmt, enum status status, bool xfail, > > + bool skip, va_list va) > > { > > const char *prefix = skip ? "SKIP" > > - : xfail ? (pass ? "XPASS" : "XFAIL") > > - : (pass ? "PASS" : "FAIL"); > > + : xfail ? (status == PASSED ? "XPASS" : "XFAIL") > > + : (status == PASSED ? "PASS" : "FAIL"); > > > > spin_lock(&lock); > > > > @@ -96,27 +96,27 @@ static void va_report(const char *msg_fmt, > > puts("\n"); > > if (skip) > > skipped++; > > - else if (xfail && !pass) > > + else if (xfail && status == FAILED) > > xfailures++; > > - else if (xfail || !pass) > > + else if (xfail || status == FAILED) > > failures++; > > > > spin_unlock(&lock); > > } > > > > -void report(const char *msg_fmt, unsigned pass, ...) > > +void report_status(const char *msg_fmt, enum status status, ...) > > { > > va_list va; > > - va_start(va, pass); > > - va_report(msg_fmt, pass, false, false, va); > > + va_start(va, status); > > + va_report(msg_fmt, status, false, false, va); > > va_end(va); > > } > > > > -void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) > > +void report_xfail_status(const char *msg_fmt, bool xfail, enum status > > status, ...) > > { > > va_list va; > > - va_start(va, pass); > > - va_report(msg_fmt, pass, xfail, false, va); > > + va_start(va, status); > > + va_report(msg_fmt, status, xfail, false, va); > > va_end(va); > > } > > That's certainly a solution... but I wonder whether it might be easier > to simply fix the failing tests instead, to make sure that they do not > pass a value > sizeof(int) to report() and report_xfail_status() ? > It may be easier, but it won't stop future changes from encountering the same issue. > Another idea would be to swap the parameters of report() and > report_xfail_status() : > It's a bit non-standard, but I don't have much of a preference. It would take changing tons of places in the code base though. > diff --git a/lib/libcflat.h b/lib/libcflat.h > index b94d0ac..d6d1323 100644 > --- a/lib/libcflat.h > +++ b/lib/libcflat.h > @@ -99,10 +99,10 @@ 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, ...) > - __attribute__((format(printf, 1, > 3))); > -extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) > - __attribute__((format(printf, 1, > 4))); > +extern void report(bool pass, const char *msg_fmt, ...) > + __attribute__((format(printf, 2, > 3))); > +extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) > + __attribute__((format(printf, 3, > 4))); > extern void report_abort(const char *msg_fmt, ...) > __attribute__((format(printf, 1, > 2))) > __attribute__((noreturn)); > diff --git a/lib/report.c b/lib/report.c > index ca9b4fd..2255dc3 100644 > --- a/lib/report.c > +++ b/lib/report.c > @@ -104,18 +104,18 @@ static void va_report(const char *msg_fmt, > spin_unlock(&lock); > } > > -void report(const char *msg_fmt, bool pass, ...) > +void report(bool pass, const char *msg_fmt, ...) > { > va_list va; > - va_start(va, pass); > + va_start(va, msg_fmt); > va_report(msg_fmt, pass, false, false, va); > va_end(va); > } > > -void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) > +void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) > { > va_list va; > - va_start(va, pass); > + va_start(va, msg_fmt); > va_report(msg_fmt, pass, xfail, false, va); > va_end(va); > } > > ... then we can keep the "bool" - but we have to fix all calling sites, too. > > Paolo, any preferences? > > Thomas
On 14/10/2019 10.14, Bill Wendling wrote: > On Mon, Oct 14, 2019 at 12:57 AM Thomas Huth <thuth@redhat.com> wrote: >> >> On 11/10/2019 20.36, Bill Wendling wrote: >>> I apologize for the breakage. I'm not sure how this escaped me. Here's >>> a proposed fix. Thoughts? >>> >>> commit 5fa1940140fd75a97f3ac5ae2e4de9e1bef645d0 >>> Author: Bill Wendling <morbo@google.com> >>> Date: Fri Oct 11 11:26:03 2019 -0700 >>> >>> Use a status enum for reporting pass/fail >>> >>> Some values passed into "report" as "pass/fail" are larger than the >>> size of the parameter. Use instead a status enum so that the size of the >>> argument no longer matters. >>> >>> diff --git a/lib/libcflat.h b/lib/libcflat.h >>> index b6635d9..8f80a1c 100644 >>> --- a/lib/libcflat.h >>> +++ b/lib/libcflat.h >>> @@ -95,13 +95,22 @@ extern int vsnprintf(char *buf, int size, const >>> char *fmt, va_list va) >>> extern int vprintf(const char *fmt, va_list va) >>> __attribute__((format(printf, 1, 0))); >>> >>> +enum status { PASSED, FAILED }; >>> + >>> +#define STATUS(x) ((x) != 0 ? PASSED : FAILED) >>> + >>> +#define report(msg_fmt, status, ...) \ >>> + report_status(msg_fmt, STATUS(status) __VA_OPT__(,) __VA_ARGS__) >>> +#define report_xfail(msg_fmt, xfail, status, ...) \ >>> + report_xfail_status(msg_fmt, xfail, STATUS(status) __VA_OPT__(,) __VA_ARGS__) >>> + >>> 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, unsigned pass, ...) >>> +extern void report_status(const char *msg_fmt, unsigned pass, ...) >>> __attribute__((format(printf, 1, 3))); >>> -extern void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) >>> +extern void report_xfail_status(const char *msg_fmt, bool xfail, enum >>> status status, ...) >>> __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 2a5f549..4ba2ac0 100644 >>> --- a/lib/report.c >>> +++ b/lib/report.c >>> @@ -80,12 +80,12 @@ void report_prefix_pop(void) >>> spin_unlock(&lock); >>> } >>> >>> -static void va_report(const char *msg_fmt, >>> - bool pass, bool xfail, bool skip, va_list va) >>> +static void va_report(const char *msg_fmt, enum status status, bool xfail, >>> + bool skip, va_list va) >>> { >>> const char *prefix = skip ? "SKIP" >>> - : xfail ? (pass ? "XPASS" : "XFAIL") >>> - : (pass ? "PASS" : "FAIL"); >>> + : xfail ? (status == PASSED ? "XPASS" : "XFAIL") >>> + : (status == PASSED ? "PASS" : "FAIL"); >>> >>> spin_lock(&lock); >>> >>> @@ -96,27 +96,27 @@ static void va_report(const char *msg_fmt, >>> puts("\n"); >>> if (skip) >>> skipped++; >>> - else if (xfail && !pass) >>> + else if (xfail && status == FAILED) >>> xfailures++; >>> - else if (xfail || !pass) >>> + else if (xfail || status == FAILED) >>> failures++; >>> >>> spin_unlock(&lock); >>> } >>> >>> -void report(const char *msg_fmt, unsigned pass, ...) >>> +void report_status(const char *msg_fmt, enum status status, ...) >>> { >>> va_list va; >>> - va_start(va, pass); >>> - va_report(msg_fmt, pass, false, false, va); >>> + va_start(va, status); >>> + va_report(msg_fmt, status, false, false, va); >>> va_end(va); >>> } >>> >>> -void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...) >>> +void report_xfail_status(const char *msg_fmt, bool xfail, enum status >>> status, ...) >>> { >>> va_list va; >>> - va_start(va, pass); >>> - va_report(msg_fmt, pass, xfail, false, va); >>> + va_start(va, status); >>> + va_report(msg_fmt, status, xfail, false, va); >>> va_end(va); >>> } >> >> That's certainly a solution... but I wonder whether it might be easier >> to simply fix the failing tests instead, to make sure that they do not >> pass a value > sizeof(int) to report() and report_xfail_status() ? >> > It may be easier, but it won't stop future changes from encountering > the same issue. True. >> Another idea would be to swap the parameters of report() and >> report_xfail_status() : >> > It's a bit non-standard, but I don't have much of a preference. It > would take changing tons of places in the code base though. Yes, it's a bigger change now ... but with your approach, I'm a little bit afraid that we'll forget the reason over the years, so one day in the future somebody might wonder what's this "enum status" magic all about and more or less revert your patch again... so if we take your patch, I think there should either be some comments in the code as explanation, or we might want to add builds with clang to the .travis.yml and gitlab-ci.yml files to make sure that we keep building the kvm-unit-tests with clang, too. Thomas
On 14/10/19 09:57, Thomas Huth wrote: > -void report(const char *msg_fmt, bool pass, ...) > +void report(bool pass, const char *msg_fmt, ...) > { > va_list va; > - va_start(va, pass); > + va_start(va, msg_fmt); > va_report(msg_fmt, pass, false, false, va); > va_end(va); > } > > -void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) > +void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) > { > va_list va; > - va_start(va, pass); > + va_start(va, msg_fmt); > va_report(msg_fmt, pass, xfail, false, va); > va_end(va); > } > > ... then we can keep the "bool" - but we have to fix all calling sites, too. > > Paolo, any preferences? Actually I had already pushed Bill's patch. I also thought about reordering the arguments, but it's a big change. If someone wants to try his hands at doing it with Coccinelle, I'm happy to apply it. Paolo
On 14/10/2019 13.04, Paolo Bonzini wrote: > On 14/10/19 09:57, Thomas Huth wrote: >> -void report(const char *msg_fmt, bool pass, ...) >> +void report(bool pass, const char *msg_fmt, ...) >> { >> va_list va; >> - va_start(va, pass); >> + va_start(va, msg_fmt); >> va_report(msg_fmt, pass, false, false, va); >> va_end(va); >> } >> >> -void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...) >> +void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...) >> { >> va_list va; >> - va_start(va, pass); >> + va_start(va, msg_fmt); >> va_report(msg_fmt, pass, xfail, false, va); >> va_end(va); >> } >> >> ... then we can keep the "bool" - but we have to fix all calling sites, too. >> >> Paolo, any preferences? > > Actually I had already pushed Bill's patch. I also thought about > reordering the arguments, but it's a big change. If someone wants to > try his hands at doing it with Coccinelle, I'm happy to apply it. This coccinelle script seems to do the job: @@ expression fmt; expression pass; expression list args; @@ report( -fmt, pass +pass, fmt , args); @@ expression fmt; expression pass; expression list args; @@ report_xfail( -fmt, xfail, pass +xfail, pass, fmt , args); I'll try to cook a patch with that. Thomas
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);
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@google.com> --- lib/libcflat.h | 4 ++-- lib/report.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)