Messages in this thread | | | Date | Thu, 5 Oct 2023 12:45:00 +0200 | Subject | Re: [PATCH] KVM: selftests: Zero-initialize entire test_result in memslot perf test | From | "Maciej S. Szmigiero" <> |
| |
On 5.10.2023 02:29, Sean Christopherson wrote: > Zero-initialize the entire test_result structure used by memslot_perf_test > instead of zeroing only the fields used to guard the pr_info() calls. > > gcc 13.2.0 is a bit overzealous and incorrectly thinks that rbestslottim's > slot_runtime may be used uninitialized. > > In file included from memslot_perf_test.c:25: > memslot_perf_test.c: In function ‘main’: > include/test_util.h:31:22: error: ‘rbestslottime.slot_runtime.tv_nsec’ may be used uninitialized [-Werror=maybe-uninitialized] > 31 | #define pr_info(...) printf(__VA_ARGS__) > | ^~~~~~~~~~~~~~~~~~~ > memslot_perf_test.c:1127:17: note: in expansion of macro ‘pr_info’ > 1127 | pr_info("Best slot setup time for the whole test area was %ld.%.9lds\n", > | ^~~~~~~ > memslot_perf_test.c:1092:28: note: ‘rbestslottime.slot_runtime.tv_nsec’ was declared here > 1092 | struct test_result rbestslottime; > | ^~~~~~~~~~~~~ > include/test_util.h:31:22: error: ‘rbestslottime.slot_runtime.tv_sec’ may be used uninitialized [-Werror=maybe-uninitialized] > 31 | #define pr_info(...) printf(__VA_ARGS__) > | ^~~~~~~~~~~~~~~~~~~ > memslot_perf_test.c:1127:17: note: in expansion of macro ‘pr_info’ > 1127 | pr_info("Best slot setup time for the whole test area was %ld.%.9lds\n", > | ^~~~~~~ > memslot_perf_test.c:1092:28: note: ‘rbestslottime.slot_runtime.tv_sec’ was declared here > 1092 | struct test_result rbestslottime; > | ^~~~~~~~~~~~~ > > That can't actually happen, at least not without the "result" structure in > test_loop() also being used uninitialized, which gcc doesn't complain > about, as writes to rbestslottime are all-or-nothing, i.e. slottimens can't > be non-zero without slot_runtime being written. > > if (!data->mem_size && > (!rbestslottime->slottimens || > result.slottimens < rbestslottime->slottimens)) > *rbestslottime = result; > > Zero-initialize the structures to make gcc happy even though this is > likely a compiler bug. The cost to do so is negligible, both in terms of > code and runtime overhead. The only downside is that the compiler won't > warn about legitimate usage of "uninitialized" data, e.g. the test could > end up consuming zeros instead of useful data. However, given that the > test is quite mature and unlikely to see substantial changes, the odds of > introducing such bugs are relatively low, whereas being able to compile > KVM selftests with -Werror detects issues on a regular basis. > > Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > > I don't like papering over compiler bugs, but this is causing me quite a bit of > pain, and IMO the long-term downsides are quite minimal. And I already spent > way too much time trying to figure out if there is some bizarre edge case that > gcc is detecting :-/ >
Weird, but as you say, the downsides of papering over this (probable) compiler issue are small, so: Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Thanks, Maciej
| |