Re: [kvm-unit-tests PATCH 4/4] x86: fix printf format warnings

[Thomas Huth <thuth@redhat.com>]

On 29.02.2016 21:19, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/x86/desc.c            |  6 +++---
>  lib/x86/vm.c              |  6 +++---
>  x86/access.c              |  4 ++--
>  x86/eventinj.c            |  8 ++++----
>  x86/kvmclock.c            |  2 +-
>  x86/kvmclock_test.c       |  6 +++---
>  x86/msr.c                 |  2 +-
>  x86/s3.c                  |  6 +++---
>  x86/svm.c                 | 12 ++++++------
>  x86/taskswitch.c          |  2 +-
>  x86/taskswitch2.c         |  2 +-
>  x86/tsc.c                 |  4 ++--
>  x86/tscdeadline_latency.c |  2 +-
>  x86/vmx.c                 | 18 +++++++++---------
>  x86/vmx_tests.c           | 20 ++++++++++----------
>  x86/xsave.c               |  2 +-
>  16 files changed, 51 insertions(+), 51 deletions(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 1a80c01283853..0a79a41b56cb1 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -53,7 +53,7 @@ static void check_exception_table(struct ex_regs *regs)
>              return;
>          }
>      }
> -    printf("unhandled exception %d\n", regs->vector);
> +    printf("unhandled exception %ld\n", regs->vector);

That could also be %lu instead, I think, but it likely does not really
matter here.

>      exit(7);
>  }
>  
> @@ -75,9 +75,9 @@ void do_handle_exception(struct ex_regs *regs)
>  		exception_handlers[regs->vector](regs);
>  		return;
>  	}
> -	printf("unhandled cpu exception %d\n", regs->vector);
> +	printf("unhandled cpu exception %ld\n", regs->vector);
>  	if (regs->vector == 14)
> -		printf("PF at %p addr %p\n", regs->rip, read_cr2());
> +		printf("PF at 0x%lx addr 0x%lx\n", regs->rip, read_cr2());
>  	exit(7);
>  }
>  
...
> diff --git a/x86/kvmclock_test.c b/x86/kvmclock_test.c
> index b6da5ebbe1097..c8ffa126803d5 100644
> --- a/x86/kvmclock_test.c
> +++ b/x86/kvmclock_test.c
> @@ -67,7 +67,7 @@ static void kvm_clock_test(void *data)
>                          ++hv_test_info->warps;
>                          if (delta < hv_test_info->worst){
>                                  hv_test_info->worst = delta;
> -                                printf("Worst warp %lld %\n", hv_test_info->worst);
> +                                printf("Worst warp %lld\n", hv_test_info->worst);

Ah, nice, so this addition of __attribute__((format)) indeed revealed
also some real problematic format strings :-)

>                          }
>                          spin_unlock(&hv_test_info->lock);
>                  }
> @@ -102,8 +102,8 @@ static int cycle_test(int ncpus, int check, struct test_info *ti)
>          printf("Total vcpus: %d\n", ncpus);
>          printf("Test  loops: %ld\n", loops);
>          if (check == 1) {
> -                printf("Total warps:  %lld\n", ti->warps);
> -                printf("Total stalls: %lld\n", ti->stalls);
> +                printf("Total warps:  %" PRId64 "\n", ti->warps);
> +                printf("Total stalls: %" PRId64 "\n", ti->stalls);
>                  printf("Worst warp:   %lld\n", ti->worst);
>          } else
>                  printf("TSC cycles:  %lld\n", end - begin);
> diff --git a/x86/msr.c b/x86/msr.c
> index ec4710ed478b9..50b13840ed946 100644
> --- a/x86/msr.c
> +++ b/x86/msr.c
> @@ -88,7 +88,7 @@ static void test_msr_rw(int msr_index, unsigned long long input, unsigned long l
>      wrmsr(msr_index, input);
>      r = rdmsr(msr_index);
>      if (expected != r) {
> -        printf("testing %s: output = 0x%x:0x%x expected = 0x%x:0x%x\n", sptr, r >> 32, r, expected >> 32, expected);
> +        printf("testing %s: output = 0x%llx:0x%llx expected = 0x%llx:0x%llx\n", sptr, r >> 32, r, expected >> 32, expected);

That code looks wrong (before and after your change, but in different
ways ;-)):
r is long long, so let's assume its value is 0x1234567890ABCDEF. You
would then get the following output:

 output = 0x12345678:0x1234567890ABCDEF

But the author likely intended:

 output = 0x12345678:0x90ABCDEF

Same for the "expected" value.

So either the lower part has explicitely be casted to (uint32_t), or
simply print the whole value only once with llx, without the colon
inbetween.

>      }
>      report(sptr, expected == r);
>  }
...

> diff --git a/x86/svm.c b/x86/svm.c
> index 1046ddf73732f..934b2ae91fa81 100644
> --- a/x86/svm.c
> +++ b/x86/svm.c
> @@ -934,9 +934,9 @@ static bool latency_finished(struct test *test)
>  
>  static bool latency_check(struct test *test)
>  {
> -    printf("    Latency VMRUN : max: %d min: %d avg: %d\n", latvmrun_max,
> +    printf("    Latency VMRUN : max: %ld min: %ld avg: %ld\n", latvmrun_max,
>              latvmrun_min, vmrun_sum / LATENCY_RUNS);
> -    printf("    Latency VMEXIT: max: %d min: %d avg: %d\n", latvmexit_max,
> +    printf("    Latency VMEXIT: max: %ld min: %ld avg: %ld\n", latvmexit_max,
>              latvmexit_min, vmexit_sum / LATENCY_RUNS);
>      return true;
>  }
> @@ -998,13 +998,13 @@ static bool lat_svm_insn_finished(struct test *test)
>  
>  static bool lat_svm_insn_check(struct test *test)
>  {
> -    printf("    Latency VMLOAD: max: %d min: %d avg: %d\n", latvmload_max,
> +    printf("    Latency VMLOAD: max: %ld min: %ld avg: %ld\n", latvmload_max,
>              latvmload_min, vmload_sum / LATENCY_RUNS);
> -    printf("    Latency VMSAVE: max: %d min: %d avg: %d\n", latvmsave_max,
> +    printf("    Latency VMSAVE: max: %ld min: %ld avg: %ld\n", latvmsave_max,
>              latvmsave_min, vmsave_sum / LATENCY_RUNS);
> -    printf("    Latency STGI:   max: %d min: %d avg: %d\n", latstgi_max,
> +    printf("    Latency STGI:   max: %ld min: %ld avg: %ld\n", latstgi_max,
>              latstgi_min, stgi_sum / LATENCY_RUNS);
> -    printf("    Latency CLGI:   max: %d min: %d avg: %d\n", latclgi_max,
> +    printf("    Latency CLGI:   max: %ld min: %ld avg: %ld\n", latclgi_max,
>              latclgi_min, clgi_sum / LATENCY_RUNS);
>      return true;
>  }

The latxxx_min/max variables seem to be declared with u64 ... so maybe
better use PRIu64 here instead?

> diff --git a/x86/taskswitch2.c b/x86/taskswitch2.c
> index db3e41aa82186..dbc9a2525d01a 100644
> --- a/x86/taskswitch2.c
> +++ b/x86/taskswitch2.c
> @@ -62,7 +62,7 @@ start:
>  
>  void do_pf_tss(ulong *error_code)
>  {
> -	printf("PF task is running %x %x\n", error_code, *(ulong*)error_code);
> +	printf("PF task is running %p %lx\n", error_code, *(ulong*)error_code);

While you're at it, you could remove the (ulong*) typecast here, too.

>  	print_current_tss_info();
>  	if (*(ulong*)error_code == 0x2) /* write access, not present */
>  		test_count++;
...
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 3fa1a735881f7..e047a59ab1129 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -144,17 +144,17 @@ void print_vmexit_info()
>  	guest_rip = vmcs_read(GUEST_RIP);
>  	guest_rsp = vmcs_read(GUEST_RSP);
>  	printf("VMEXIT info:\n");
> -	printf("\tvmexit reason = %d\n", reason);
> -	printf("\texit qualification = 0x%x\n", exit_qual);
> -	printf("\tBit 31 of reason = %x\n", (vmcs_read(EXI_REASON) >> 31) & 1);
> -	printf("\tguest_rip = 0x%llx\n", guest_rip);
> -	printf("\tRAX=0x%llx    RBX=0x%llx    RCX=0x%llx    RDX=0x%llx\n",
> +	printf("\tvmexit reason = %ld\n", reason);
> +	printf("\texit qualification = 0x%lx\n", exit_qual);
> +	printf("\tBit 31 of reason = %lx\n", (vmcs_read(EXI_REASON) >> 31) & 1);
> +	printf("\tguest_rip = 0x%lx\n", guest_rip);
> +	printf("\tRAX=0x%lx    RBX=0x%lx    RCX=0x%lx    RDX=0x%lx\n",
>  		regs.rax, regs.rbx, regs.rcx, regs.rdx);
> -	printf("\tRSP=0x%llx    RBP=0x%llx    RSI=0x%llx    RDI=0x%llx\n",
> +	printf("\tRSP=0x%lx    RBP=0x%lx    RSI=0x%lx    RDI=0x%lx\n",
>  		guest_rsp, regs.rbp, regs.rsi, regs.rdi);
> -	printf("\tR8 =0x%llx    R9 =0x%llx    R10=0x%llx    R11=0x%llx\n",
> +	printf("\tR8 =0x%lx    R9 =0x%lx    R10=0x%lx    R11=0x%lx\n",
>  		regs.r8, regs.r9, regs.r10, regs.r11);
> -	printf("\tR12=0x%llx    R13=0x%llx    R14=0x%llx    R15=0x%llx\n",
> +	printf("\tR12=0x%lx    R13=0x%lx    R14=0x%lx    R15=0x%lx\n",
>  		regs.r12, regs.r13, regs.r14, regs.r15);
>  }

The registers are declared as u64 ... so use PRIx64 for them instead?

> @@ -842,7 +842,7 @@ static int handle_hypercall()
>  	case HYPERCALL_VMEXIT:
>  		return VMX_TEST_VMEXIT;
>  	default:
> -		printf("ERROR : Invalid hypercall number : %d\n", hypercall_no);
> +		printf("ERROR : Invalid hypercall number : %ld\n", hypercall_no);
>  	}
>  	return VMX_TEST_EXIT;
>  }
...
> diff --git a/x86/xsave.c b/x86/xsave.c
> index e471835b42fd9..52142d2cdb93b 100644
> --- a/x86/xsave.c
> +++ b/x86/xsave.c
> @@ -75,7 +75,7 @@ void test_xsave(void)
>      printf("Legal instruction testing:\n");
>  
>      supported_xcr0 = get_supported_xcr0();
> -    printf("Supported XCR0 bits: 0x%x\n", supported_xcr0);
> +    printf("Supported XCR0 bits: 0x%lx\n", supported_xcr0);

Also PRIx64 here?

 Thomas