|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH v2] target/i386: Remove compilation errors when -Werror=maybe-uninitialized |
Date: | Thu, 22 Dec 2022 13:44:46 +0100 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 |
On 22/12/22 13:32, Stefan Weil wrote:
Am 22.12.22 um 12:51 schrieb Philippe Mathieu-Daudé:On 22/12/22 12:18, Eric Auger wrote:Hi All, On 12/22/22 12:09, Daniel P. Berrangé wrote:On Thu, Dec 22, 2022 at 11:07:31AM +0100, Eric Auger wrote:Hi Philippe, On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:On 22/12/22 09:18, Paolo Bonzini wrote:On 12/21/22 17:36, Eric Auger wrote:To avoid compilation errors when -Werror=maybe-uninitialized is used,replace 'case 3' by 'default'. Otherwise we get: ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’: ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 2495 | d->Q(3) = r3; | ~~~~~~~~^~~~ ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 2494 | d->Q(2) = r2; | ~~~~~~~~^~~~ ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 2493 | d->Q(1) = r1; | ~~~~~~~~^~~~ ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 2492 | d->Q(0) = r0; | ~~~~~~~~^~~~With what compiler? Is that a supported one?3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/">https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/I guess this won't fix the fact r0, r1, r2, r3 are not initialized, will it?Signed-off-by: Eric Auger <eric.auger@redhat.com> Suggested-by: Stefan Weil <sw@weilnetz.de> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX") --- target/i386/ops_sse.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h index 3cbc36a59d..c442c8c10c 100644 --- a/target/i386/ops_sse.h +++ b/target/i386/ops_sse.h @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order) r0 = s->Q(0); r1 = s->Q(1); break; - case 3: + default: r0 = s->Q(2); r1 = s->Q(3); break; @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order) r2 = s->Q(0); r3 = s->Q(1); break; - case 3: + default: r2 = s->Q(2); r3 = s->Q(3); break;Queued, but this compiler sucks. :)Can't we simply add a dumb 'default' case? So when reviewing we don't have to evaluate 'default' means 3 here. -- >8 -- --- a/target/i386/ops_sse.h +++ b/target/i386/ops_sse.h @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order) r0 = s->Q(2); r1 = s->Q(3); break; + default: + qemu_build_not_reached(); } switch ((order >> 4) & 3) { case 0: @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order) r2 = s->Q(2); r3 = s->Q(3); break; + default: + qemu_build_not_reached(); }This ultimately expands to assert() and the compiler should see that it terminates the control flow at this point, so shouldn't have a reason to warn.OK so with qemu_build_not_reached(); I get /home/augere/UPSTREAM/qemu/include/qemu/osdep.h:184:35: error: call to ‘qemu_build_not_reached_always’ declared with attribute error: code path is reachable184 | #define qemu_build_not_reached() qemu_build_not_reached_always()| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ However with g_assert_not_reached(), it does not complain and errors are removed. So I will respin with g_assert_not_reached() if nobody advises me against that.Thank you!As noted by Paolo a better compiler could know that 0, 1, 2 and 3 are the only possible cases. Such a better compiler might complain that an additional default case is never reached. Therefore the proposed code might cause future compiler warnings.But we could use this code pattern to make the intention of the code clearer:case 3: default: /* default case added to help the compiler to avoid warnings */ ...
I'm fine with that, as long as we don't obfuscate the code for clever compiler's sake. QEMU code base is already complex enough (the devil 😈 is in the details).
[Prev in Thread] | Current Thread | [Next in Thread] |