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@xxxxxxxxxx>
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@xxxxxxxxxx> 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@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
