[kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Bill Wendling]

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);

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Jim Mattson]

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>

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Bill Wendling]

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>

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[David Hildenbrand]

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)

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Bill Wendling]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[David Hildenbrand]

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


-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Bill Wendling]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Thomas Huth]

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