Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <631a4966-982b-8d7c-1ad5-e9358f98fcf4@linaro.org>
Date:   Thu, 5 Oct 2023 08:41:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: [PATCH] KVM: selftests: Zero-initialize entire test_result in
 memslot perf test
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        "Maciej S . Szmigiero" <maciej.szmigiero@oracle.com>
References: <20231005002954.2887098-1-seanjc@google.com>
From:   =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= <philmd@linaro.org>
In-Reply-To: <20231005002954.2887098-1-seanjc@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 <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 :-/
> 
>   tools/testing/selftests/kvm/memslot_perf_test.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>