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 <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
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 :-/
tools/testing/selftests/kvm/memslot_perf_test.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 20eb2e730800..8698d1ab60d0 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -1033,9 +1033,8 @@ static bool test_loop(const struct test_data *data,
struct test_result *rbestruntime)
{
uint64_t maxslots;
- struct test_result result;
+ struct test_result result = {};
- result.nloops = 0;
if (!test_execute(targs->nslots, &maxslots, targs->seconds, data,
&result.nloops,
&result.slot_runtime, &result.guest_runtime)) {
@@ -1089,7 +1088,7 @@ int main(int argc, char *argv[])
.seconds = 5,
.runs = 1,
};
- struct test_result rbestslottime;
+ struct test_result rbestslottime = {};
int tctr;
if (!check_memory_sizes())
@@ -1098,11 +1097,10 @@ int main(int argc, char *argv[])
if (!parse_args(argc, argv, &targs))
return -1;
- rbestslottime.slottimens = 0;
for (tctr = targs.tfirst; tctr <= targs.tlast; tctr++) {
const struct test_data *data = &tests[tctr];
unsigned int runctr;
- struct test_result rbestruntime;
+ struct test_result rbestruntime = {};
if (tctr > targs.tfirst)
pr_info("\n");
@@ -1110,7 +1108,6 @@ int main(int argc, char *argv[])
pr_info("Testing %s performance with %i runs, %d seconds each\n",
data->name, targs.runs, targs.seconds);
- rbestruntime.runtimens = 0;
for (runctr = 0; runctr < targs.runs; runctr++)
if (!test_loop(data, &targs,
&rbestslottime, &rbestruntime))
base-commit: 013858c6fc2491c70640556385d5af123d1596c5
--
2.42.0.582.g8ccd20d70d-goog
On 5/10/23 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 <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
>
> 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 :-/
>
> tools/testing/selftests/kvm/memslot_perf_test.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
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 <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
>
> 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 <[email protected]>
Thanks,
Maciej
On Wed, 04 Oct 2023 17:29:54 -0700, 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.
Applied to kvm-x86 selftests, thanks much for the quick reviews!
[1/1] KVM: selftests: Zero-initialize entire test_result in memslot perf test
https://github.com/kvm-x86/linux/commit/6313e096dbfa
--
https://github.com/kvm-x86/linux/tree/next