Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled
On Mon, May 20, 2019 at 12:03:08PM +0200, Paolo Bonzini wrote: > On 17/05/19 11:04, Thomas Huth wrote: > > So far the KVM selftests are compiled without any compiler warnings > > enabled. That's quite bad, since we miss a lot of possible bugs this > > way. Let's enable at least "-Wall" and some other useful warning flags > > now, and fix at least the trivial problems in the code (like unused > > variables). > > > > Signed-off-by: Thomas Huth > > --- > > v2: > > - Rebased to kvm/queue > > - Fix warnings in state_test.c and evmcs_test.c, too > > > > tools/testing/selftests/kvm/Makefile | 4 +++- > > tools/testing/selftests/kvm/dirty_log_test.c | 6 +- > > tools/testing/selftests/kvm/lib/kvm_util.c | 3 --- > > tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +--- > > tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c | 1 + > > tools/testing/selftests/kvm/x86_64/evmcs_test.c| 7 +-- > > tools/testing/selftests/kvm/x86_64/platform_info_test.c| 1 - > > tools/testing/selftests/kvm/x86_64/smm_test.c | 3 +-- > > tools/testing/selftests/kvm/x86_64/state_test.c| 7 +-- > > .../selftests/kvm/x86_64/vmx_close_while_nested_test.c | 5 + > > tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c | 5 ++--- > > 11 files changed, 16 insertions(+), 30 deletions(-) > > Queued, with a squashed fix to kvm_get_supported_hv_cpuid. > I've done the fixups needed to keep aarch64 compiling and will send the patch shortly. drew
Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled
On 17/05/19 12:07, Thomas Huth wrote: > > lib/ucall.c: In function ‘get_ucall’: > lib/ucall.c:145:3: warning: dereferencing type-punned pointer will break > strict-aliasing rules [-Wstrict-aliasing] >gva = *(vm_vaddr_t *)run->mmio.data; > > x86_64/vmx_set_nested_state_test.c: In function > ‘set_revision_id_for_vmcs12’: > x86_64/vmx_set_nested_state_test.c:78:2: warning: dereferencing > type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] > *(u32 *)(state->data) = vmcs12_revision; > > ... how do we want to handle such spots in the kvm selftest code? > Compile with -fno-strict-aliasing? Or fix it with type-punning through > unions? I would use memcpy. I'll send a patch shortly. Paolo
Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled
On 17/05/19 11:04, Thomas Huth wrote: > So far the KVM selftests are compiled without any compiler warnings > enabled. That's quite bad, since we miss a lot of possible bugs this > way. Let's enable at least "-Wall" and some other useful warning flags > now, and fix at least the trivial problems in the code (like unused > variables). > > Signed-off-by: Thomas Huth > --- > v2: > - Rebased to kvm/queue > - Fix warnings in state_test.c and evmcs_test.c, too > > tools/testing/selftests/kvm/Makefile | 4 +++- > tools/testing/selftests/kvm/dirty_log_test.c | 6 +- > tools/testing/selftests/kvm/lib/kvm_util.c | 3 --- > tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +--- > tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c | 1 + > tools/testing/selftests/kvm/x86_64/evmcs_test.c| 7 +-- > tools/testing/selftests/kvm/x86_64/platform_info_test.c| 1 - > tools/testing/selftests/kvm/x86_64/smm_test.c | 3 +-- > tools/testing/selftests/kvm/x86_64/state_test.c| 7 +-- > .../selftests/kvm/x86_64/vmx_close_while_nested_test.c | 5 + > tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c | 5 ++--- > 11 files changed, 16 insertions(+), 30 deletions(-) Queued, with a squashed fix to kvm_get_supported_hv_cpuid. Paolo
Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled
Thomas Huth writes: > > x86_64/vmx_set_nested_state_test.c: In function > ‘set_revision_id_for_vmcs12’: > x86_64/vmx_set_nested_state_test.c:78:2: warning: dereferencing > type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] > *(u32 *)(state->data) = vmcs12_revision; > > ... how do we want to handle such spots in the kvm selftest code? > Compile with -fno-strict-aliasing? Or fix it with type-punning through > unions? These are tests, let's keep code simple. Casting my vote for the former option) -- Vitaly
Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled
On 17/05/2019 11.41, Vitaly Kuznetsov wrote: > Peter Xu writes: > >> On Fri, May 17, 2019 at 11:04:45AM +0200, Thomas Huth wrote: >>> So far the KVM selftests are compiled without any compiler warnings >>> enabled. That's quite bad, since we miss a lot of possible bugs this >>> way. Let's enable at least "-Wall" and some other useful warning flags >>> now, and fix at least the trivial problems in the code (like unused >>> variables). >>> >>> Signed-off-by: Thomas Huth >>> --- >>> v2: >>> - Rebased to kvm/queue >>> - Fix warnings in state_test.c and evmcs_test.c, too >> >> I still see these warnings (probably because the hyperv_cpuid.c is a >> new test): >> >> In file included from x86_64/hyperv_cpuid.c:18: >> x86_64/hyperv_cpuid.c: In function ‘test_hv_cpuid’: >> x86_64/hyperv_cpuid.c:61:33: warning: suggest parentheses around comparison >> in operand of ‘==’ [-Wparentheses] >>TEST_ASSERT(entry->padding[0] == entry->padding[1] >>~~^~~~ >> include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’ >> test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__) >>^ >> x86_64/hyperv_cpuid.c:62:8: warning: suggest parentheses around comparison >> in operand of ‘==’ [-Wparentheses] >>TEST_ASSERT(entry->padding[0] == entry->padding[1] >>~~ >> == entry->padding[2] == 0, >> ^~~~ >> include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’ >> test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__) > > There's a fix from Dan Carpenter on the list: > https://marc.info/?l=kernel-janitors=155783012012532=2 Right, that fix should preferably be committed first. >> x86_64/hyperv_cpuid.c: In function ‘kvm_get_supported_hv_cpuid’: >> x86_64/hyperv_cpuid.c:93:6: warning: unused variable ‘ret’ >> [-Wunused-variable] >> int ret; >> ^~~ ... but I obviously missed this one here again :-/ There are also these compiler warnings left: lib/ucall.c: In function ‘get_ucall’: lib/ucall.c:145:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] gva = *(vm_vaddr_t *)run->mmio.data; x86_64/vmx_set_nested_state_test.c: In function ‘set_revision_id_for_vmcs12’: x86_64/vmx_set_nested_state_test.c:78:2: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] *(u32 *)(state->data) = vmcs12_revision; ... how do we want to handle such spots in the kvm selftest code? Compile with -fno-strict-aliasing? Or fix it with type-punning through unions? Thomas
Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled
Peter Xu writes: > On Fri, May 17, 2019 at 11:04:45AM +0200, Thomas Huth wrote: >> So far the KVM selftests are compiled without any compiler warnings >> enabled. That's quite bad, since we miss a lot of possible bugs this >> way. Let's enable at least "-Wall" and some other useful warning flags >> now, and fix at least the trivial problems in the code (like unused >> variables). >> >> Signed-off-by: Thomas Huth >> --- >> v2: >> - Rebased to kvm/queue >> - Fix warnings in state_test.c and evmcs_test.c, too > > I still see these warnings (probably because the hyperv_cpuid.c is a > new test): > > In file included from x86_64/hyperv_cpuid.c:18: > x86_64/hyperv_cpuid.c: In function ‘test_hv_cpuid’: > x86_64/hyperv_cpuid.c:61:33: warning: suggest parentheses around comparison > in operand of ‘==’ [-Wparentheses] >TEST_ASSERT(entry->padding[0] == entry->padding[1] >~~^~~~ > include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’ > test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__) >^ > x86_64/hyperv_cpuid.c:62:8: warning: suggest parentheses around comparison in > operand of ‘==’ [-Wparentheses] >TEST_ASSERT(entry->padding[0] == entry->padding[1] >~~ > == entry->padding[2] == 0, > ^~~~ > include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’ > test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__) There's a fix from Dan Carpenter on the list: https://marc.info/?l=kernel-janitors=155783012012532=2 >^ > x86_64/hyperv_cpuid.c: In function ‘kvm_get_supported_hv_cpuid’: > x86_64/hyperv_cpuid.c:93:6: warning: unused variable ‘ret’ [-Wunused-variable] > int ret; > ^~~ > > The first two seem to be real bugs in the test code, and the 3rd one > might need a cleanup too. -- Vitaly
Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled
On Fri, May 17, 2019 at 11:04:45AM +0200, Thomas Huth wrote: > So far the KVM selftests are compiled without any compiler warnings > enabled. That's quite bad, since we miss a lot of possible bugs this > way. Let's enable at least "-Wall" and some other useful warning flags > now, and fix at least the trivial problems in the code (like unused > variables). > > Signed-off-by: Thomas Huth > --- > v2: > - Rebased to kvm/queue > - Fix warnings in state_test.c and evmcs_test.c, too I still see these warnings (probably because the hyperv_cpuid.c is a new test): In file included from x86_64/hyperv_cpuid.c:18: x86_64/hyperv_cpuid.c: In function ‘test_hv_cpuid’: x86_64/hyperv_cpuid.c:61:33: warning: suggest parentheses around comparison in operand of ‘==’ [-Wparentheses] TEST_ASSERT(entry->padding[0] == entry->padding[1] ~~^~~~ include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’ test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__) ^ x86_64/hyperv_cpuid.c:62:8: warning: suggest parentheses around comparison in operand of ‘==’ [-Wparentheses] TEST_ASSERT(entry->padding[0] == entry->padding[1] ~~ == entry->padding[2] == 0, ^~~~ include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’ test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__) ^ x86_64/hyperv_cpuid.c: In function ‘kvm_get_supported_hv_cpuid’: x86_64/hyperv_cpuid.c:93:6: warning: unused variable ‘ret’ [-Wunused-variable] int ret; ^~~ The first two seem to be real bugs in the test code, and the 3rd one might need a cleanup too. Thanks, -- Peter Xu
