[kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail

[kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail

[Bill Wendling]

Some values passed into "report" as "pass/fail" are larger than the
size of the parameter. Instead use a status enum so that the size of the
argument no longer matters.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/libcflat.h | 13 +++++++++++--
 lib/report.c   | 24 ++++++++++++------------
 2 files changed, 23 insertions(+), 14 deletions(-)

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);
 }
 
-- 
2.23.0.700.g56cf767bdb-goog

[kvm-unit-tests PATCH 2/2] x86: use pointer for end of exception table

[Bill Wendling]

Two global objects can't have the same address in C. Clang uses this
fact to omit the check on the first iteration of the loop in
check_exception_table.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/x86/desc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 451f504..bfe8651 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -41,7 +41,7 @@ struct ex_record {
     unsigned long handler;
 };
 
-extern struct ex_record exception_table_start, exception_table_end;
+extern struct ex_record exception_table_start, *exception_table_end;
 
 static const char* exception_mnemonic(int vector)
 {
@@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
 		(((regs->rflags >> 16) & 1) << 8);
     asm("mov %0, %%gs:4" : : "r"(ex_val));
 
-    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
+    for (ex = &exception_table_start; ex != exception_table_end; ++ex) {
         if (ex->rip == regs->rip) {
             regs->rip = ex->handler;
             return;
-- 
2.23.0.700.g56cf767bdb-goog

[kvm-unit-tests PATCH 1/1] x86: use pointer for end of exception table

[Bill Wendling]

Two global objects can't have the same address in C. Clang uses this
fact to omit the check on the first iteration of the loop in
check_exception_table.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/x86/desc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 451f504..cfc449f 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -41,7 +41,7 @@ struct ex_record {
     unsigned long handler;
 };
 
-extern struct ex_record exception_table_start, exception_table_end;
+extern struct ex_record exception_table_start, *exception_table_end;
 
 static const char* exception_mnemonic(int vector)
 {
@@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
 		(((regs->rflags >> 16) & 1) << 8);
     asm("mov %0, %%gs:4" : : "r"(ex_val));
 
-    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
+    for (ex = &exception_table_start; ex != (void*)&exception_table_end; ++ex) {
         if (ex->rip == regs->rip) {
             regs->rip = ex->handler;
             return;
-- 
2.23.0.700.g56cf767bdb-goog

Re: [kvm-unit-tests PATCH 1/1] x86: use pointer for end of exception table

[Thomas Huth]

On 13/10/2019 09.18, Bill Wendling wrote:
> Two global objects can't have the same address in C. Clang uses this
> fact to omit the check on the first iteration of the loop in
> check_exception_table.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/x86/desc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 451f504..cfc449f 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -41,7 +41,7 @@ struct ex_record {
>      unsigned long handler;
>  };
>  
> -extern struct ex_record exception_table_start, exception_table_end;
> +extern struct ex_record exception_table_start, *exception_table_end;
>  
>  static const char* exception_mnemonic(int vector)
>  {
> @@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
>  		(((regs->rflags >> 16) & 1) << 8);
>      asm("mov %0, %%gs:4" : : "r"(ex_val));
>  
> -    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
> +    for (ex = &exception_table_start; ex != (void*)&exception_table_end; ++ex) {
>          if (ex->rip == regs->rip) {
>              regs->rip = ex->handler;
>              return;
> 

That looks like quite an ugly hack to me - and if clang gets a little
bit smarter, I'm pretty sure it will optimize this away, too.

Could you please check if something like this works, too:

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 451f504..0e9779b 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -41,7 +41,8 @@ struct ex_record {
     unsigned long handler;
 };

-extern struct ex_record exception_table_start, exception_table_end;
+extern struct ex_record exception_table_start;
+extern long exception_table_size;

 static const char* exception_mnemonic(int vector)
 {
@@ -108,12 +109,13 @@ static void check_exception_table(struct ex_regs
*regs)
 {
     struct ex_record *ex;
     unsigned ex_val;
+    int cnt = exception_table_size / sizeof(struct ex_record);

     ex_val = regs->vector | (regs->error_code << 16) |
                (((regs->rflags >> 16) & 1) << 8);
     asm("mov %0, %%gs:4" : : "r"(ex_val));

-    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
+    for (ex = &exception_table_start; cnt-- > 0; ++ex) {
         if (ex->rip == regs->rip) {
             regs->rip = ex->handler;
             return;
diff --git a/x86/flat.lds b/x86/flat.lds
index a278b56..108fad5 100644
--- a/x86/flat.lds
+++ b/x86/flat.lds
@@ -9,6 +9,7 @@ SECTIONS
           exception_table_start = .;
           *(.data.ex)
          exception_table_end = .;
+          exception_table_size = exception_table_end -
exception_table_start;
          }
     . = ALIGN(16);
     .rodata : { *(.rodata) }

 Thomas

Re: [kvm-unit-tests PATCH 1/1] x86: use pointer for end of exception table

[Bill Wendling]

On Tue, Oct 15, 2019 at 12:41 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 13/10/2019 09.18, Bill Wendling wrote:
> > Two global objects can't have the same address in C. Clang uses this
> > fact to omit the check on the first iteration of the loop in
> > check_exception_table.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
> > ---
> >  lib/x86/desc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> > index 451f504..cfc449f 100644
> > --- a/lib/x86/desc.c
> > +++ b/lib/x86/desc.c
> > @@ -41,7 +41,7 @@ struct ex_record {
> >      unsigned long handler;
> >  };
> >
> > -extern struct ex_record exception_table_start, exception_table_end;
> > +extern struct ex_record exception_table_start, *exception_table_end;
> >
> >  static const char* exception_mnemonic(int vector)
> >  {
> > @@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
> >               (((regs->rflags >> 16) & 1) << 8);
> >      asm("mov %0, %%gs:4" : : "r"(ex_val));
> >
> > -    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
> > +    for (ex = &exception_table_start; ex != (void*)&exception_table_end; ++ex) {
> >          if (ex->rip == regs->rip) {
> >              regs->rip = ex->handler;
> >              return;
> >
>
> That looks like quite an ugly hack to me - and if clang gets a little
> bit smarter, I'm pretty sure it will optimize this away, too.
>
I agree. I was thinking about this actually and this patch looks like
it'll work. I'll generate a new version of the patches.

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 451f504..4002203 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
                (((regs->rflags >> 16) & 1) << 8);
     asm("mov %0, %%gs:4" : : "r"(ex_val));

-    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
+    for (ex = &exception_table_start; ex < &exception_table_end; ++ex) {
         if (ex->rip == regs->rip) {
             regs->rip = ex->handler;
             return;


> Could you please check if something like this works, too:
>
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 451f504..0e9779b 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -41,7 +41,8 @@ struct ex_record {
>      unsigned long handler;
>  };
>
> -extern struct ex_record exception_table_start, exception_table_end;
> +extern struct ex_record exception_table_start;
> +extern long exception_table_size;
>
>  static const char* exception_mnemonic(int vector)
>  {
> @@ -108,12 +109,13 @@ static void check_exception_table(struct ex_regs
> *regs)
>  {
>      struct ex_record *ex;
>      unsigned ex_val;
> +    int cnt = exception_table_size / sizeof(struct ex_record);
>
>      ex_val = regs->vector | (regs->error_code << 16) |
>                 (((regs->rflags >> 16) & 1) << 8);
>      asm("mov %0, %%gs:4" : : "r"(ex_val));
>
> -    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
> +    for (ex = &exception_table_start; cnt-- > 0; ++ex) {
>          if (ex->rip == regs->rip) {
>              regs->rip = ex->handler;
>              return;
> diff --git a/x86/flat.lds b/x86/flat.lds
> index a278b56..108fad5 100644
> --- a/x86/flat.lds
> +++ b/x86/flat.lds
> @@ -9,6 +9,7 @@ SECTIONS
>            exception_table_start = .;
>            *(.data.ex)
>           exception_table_end = .;
> +          exception_table_size = exception_table_end -
> exception_table_start;
>           }
>      . = ALIGN(16);
>      .rodata : { *(.rodata) }
>
>  Thomas

[kvm-unit-tests PATCH 2/2] Use a status enum for reporting pass/fail

[Bill Wendling]

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.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/libcflat.h | 14 ++++++++++++--
 lib/report.c   | 24 ++++++++++++------------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/lib/libcflat.h b/lib/libcflat.h
index b6635d9..847c24f 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -95,13 +95,23 @@ 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 VA_ARGS(...) , ##__VA_ARGS__
+#define report(msg_fmt, status, ...) \
+	report_status(msg_fmt, STATUS(status) VA_ARGS(__VA_ARGS__))
+#define report_xfail(msg_fmt, xfail, status, ...) \
+	report_xfail_status(msg_fmt, xfail, STATUS(status) VA_ARGS(__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);
 }
 
-- 
2.23.0.700.g56cf767bdb-goog

Re: [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail

[Sean Christopherson]

On Sat, Oct 12, 2019 at 12:44:53AM -0700, Bill Wendling wrote:
> Some values passed into "report" as "pass/fail" are larger than the
> size of the parameter. Instead use a status enum so that the size of the
> argument no longer matters.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>

The threading of these mails has me all kinds of confused.  What is the
relationship between all these patches?  Did you perhaps intend to send
some of these as v2?

  [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail
  [kvm-unit-tests PATCH 2/2] Use a status enum for reporting pass/fail
  [kvm-unit-tests PATCH 1/1] x86: use pointer for end of exception table
  [kvm-unit-tests PATCH 2/2] x86: use pointer for end of exception table

Re: [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail

[Bill Wendling]

On Mon, Oct 14, 2019 at 9:27 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Sat, Oct 12, 2019 at 12:44:53AM -0700, Bill Wendling wrote:
> > Some values passed into "report" as "pass/fail" are larger than the
> > size of the parameter. Instead use a status enum so that the size of the
> > argument no longer matters.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
>
> The threading of these mails has me all kinds of confused.  What is the
> relationship between all these patches?  Did you perhaps intend to send
> some of these as v2?
>
>   [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail
>   [kvm-unit-tests PATCH 2/2] Use a status enum for reporting pass/fail
>   [kvm-unit-tests PATCH 1/1] x86: use pointer for end of exception table
>   [kvm-unit-tests PATCH 2/2] x86: use pointer for end of exception table

Yes, the later ones are meant as v2. I'm not familiar with the patch
submission policy via emails. :-(

-bw

[kvm-unit-tests v2 PATCH 0/2] Clang compilation fixes

[Bill Wendling]

The attached patches have fixes for clang compilation and some failures
seen from the previous "report()" fix.

The first, changing the "report()" function to accept an "enum status"
instead of "unsigned pass" is under discussion, with others making
alternative suggestions. I add the patch here for completeness, but you
may want to wait until the discussion has settled.

The second, using "less than" to compare two global objects' addresses
for inequality, should be less controversial. 

This replaces the previous two patches I sent. I apologize for the
spammage.

    [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail
    [kvm-unit-tests PATCH 2/2] x86: use pointer for end of exception table

Bill Wendling (2):
  lib: use a status enum for reporting pass/fail
  x86: don't compare two global objects' addrs for inequality

 lib/libcflat.h | 13 +++++++++++--
 lib/report.c   | 24 ++++++++++++------------
 lib/x86/desc.c |  2 +-
 3 files changed, 24 insertions(+), 15 deletions(-)

-- 
2.23.0.700.g56cf767bdb-goog

[kvm-unit-tests v2 PATCH 1/2] lib: use a status enum for reporting pass/fail

[Bill Wendling]

Some values passed into "report" as "pass/fail" are larger than the
size of the parameter. Instead use a status enum so that the size of the
argument no longer matters.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/libcflat.h | 13 +++++++++++--
 lib/report.c   | 24 ++++++++++++------------
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/lib/libcflat.h b/lib/libcflat.h
index b6635d9..8765f67 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, enum status status, ...)
 					__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);
 }
 
-- 
2.23.0.700.g56cf767bdb-goog

[kvm-unit-tests v2 PATCH 2/2] x86: don't compare two global objects' addrs for inequality

[Bill Wendling]

Two global objects can't have the same address in C. Clang uses this
fact to omit the check on the first iteration of the loop in
check_exception_table. Avoid compariting inequality by using less-than.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/x86/desc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 451f504..4002203 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
 		(((regs->rflags >> 16) & 1) << 8);
     asm("mov %0, %%gs:4" : : "r"(ex_val));
 
-    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
+    for (ex = &exception_table_start; ex < &exception_table_end; ++ex) {
         if (ex->rip == regs->rip) {
             regs->rip = ex->handler;
             return;
-- 
2.23.0.700.g56cf767bdb-goog

Re: [kvm-unit-tests v2 PATCH 2/2] x86: don't compare two global objects' addrs for inequality

[Thomas Huth]

On 15/10/2019 22.46, Bill Wendling wrote:
> Two global objects can't have the same address in C. Clang uses this
> fact to omit the check on the first iteration of the loop in
> check_exception_table. Avoid compariting inequality by using less-than.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/x86/desc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 451f504..4002203 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
>  		(((regs->rflags >> 16) & 1) << 8);
>      asm("mov %0, %%gs:4" : : "r"(ex_val));
>  
> -    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
> +    for (ex = &exception_table_start; ex < &exception_table_end; ++ex) {
>          if (ex->rip == regs->rip) {
>              regs->rip = ex->handler;
>              return;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests v2 PATCH 2/2] x86: don't compare two global objects' addrs for inequality

[Paolo Bonzini]

On 15/10/19 22:46, Bill Wendling wrote:
> Two global objects can't have the same address in C. Clang uses this
> fact to omit the check on the first iteration of the loop in
> check_exception_table. Avoid compariting inequality by using less-than.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/x86/desc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 451f504..4002203 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -113,7 +113,7 @@ static void check_exception_table(struct ex_regs *regs)
>  		(((regs->rflags >> 16) & 1) << 8);
>      asm("mov %0, %%gs:4" : : "r"(ex_val));
>  
> -    for (ex = &exception_table_start; ex != &exception_table_end; ++ex) {
> +    for (ex = &exception_table_start; ex < &exception_table_end; ++ex) {
>          if (ex->rip == regs->rip) {
>              regs->rip = ex->handler;
>              return;
> 

Queued, thanks.

Paolo

Re: [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail

[Paolo Bonzini]

On 12/10/19 09:44, Bill Wendling wrote:
> Some values passed into "report" as "pass/fail" are larger than the
> size of the parameter. Instead use a status enum so that the size of the
> argument no longer matters.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>

I'm going to apply Thomas's argument reversal patch.  Note that the
commit message doesn't describe very well what is it that you're fixing
(or rather, why it needs fixing).

Paolo

> ---
>  lib/libcflat.h | 13 +++++++++++--
>  lib/report.c   | 24 ++++++++++++------------
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> 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);
>  }
>  
> 

Re: [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail

[Bill Wendling]

On Mon, Oct 21, 2019 at 8:33 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 12/10/19 09:44, Bill Wendling wrote:
> > Some values passed into "report" as "pass/fail" are larger than the
> > size of the parameter. Instead use a status enum so that the size of the
> > argument no longer matters.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
>
> I'm going to apply Thomas's argument reversal patch.

Okay.

> Note that the
> commit message doesn't describe very well what is it that you're fixing
> (or rather, why it needs fixing).
>
My apologies for that. I'll keep that in mind for future patches.

> Paolo
>
> > ---
> >  lib/libcflat.h | 13 +++++++++++--
> >  lib/report.c   | 24 ++++++++++++------------
> >  2 files changed, 23 insertions(+), 14 deletions(-)
> >
> > 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);
> >  }
> >
> >
>