| Message ID | 20230221153006.20300-5-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Adds support for running QEMU natively on windows-arm64 | expand |
On 21/2/23 16:30, Pierrick Bouvier wrote: > When compiling for windows-arm64 using clang-15, it reports a sometimes > uninitialized variable. This seems to be a false positive, as a default > case guards switch expressions, preventing to return an uninitialized > value, but clang seems unhappy with assert(0) definition. $ git grep 'assert(0)' | wc -l 96 Should we mass-update and forbid 'assert(0)' adding a check in scripts/checkpatch.pl? Otherwise we'll keep getting similar clang warnings... > Change code to g_assert_not_reached() fix the warning. > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/ppc/dfp_helper.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c > index cc024316d5..5967ea07a9 100644 > --- a/target/ppc/dfp_helper.c > +++ b/target/ppc/dfp_helper.c > @@ -121,7 +121,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc, > case 3: /* use FPSCR rounding mode */ > return; > default: > - assert(0); /* cannot get here */ > + g_assert_not_reached(); Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 2/21/23 12:30, Philippe Mathieu-Daudé wrote: > On 21/2/23 16:30, Pierrick Bouvier wrote: >> When compiling for windows-arm64 using clang-15, it reports a sometimes >> uninitialized variable. This seems to be a false positive, as a default >> case guards switch expressions, preventing to return an uninitialized >> value, but clang seems unhappy with assert(0) definition. > > $ git grep 'assert(0)' | wc -l > 96 > > Should we mass-update and forbid 'assert(0)' adding a check in > scripts/checkpatch.pl? Otherwise we'll keep getting similar clang > warnings... I just think assert(0) produces a less clean error message, so on that basis yes, we should replace them all. Perhaps abort() as well, unless there's an error_report immediately preceding. The fact that assert(0) was seen to fall through is a system header bug. I see we have a workaround in include/qemu/osdep.h for __MINGW32__, but I guess this doesn't trigger for arm64? Pierrick, would you mind testing a change there? r~
On 2/22/23 00:43, Richard Henderson wrote: > On 2/21/23 12:30, Philippe Mathieu-Daudé wrote: >> On 21/2/23 16:30, Pierrick Bouvier wrote: >>> When compiling for windows-arm64 using clang-15, it reports a sometimes >>> uninitialized variable. This seems to be a false positive, as a default >>> case guards switch expressions, preventing to return an uninitialized >>> value, but clang seems unhappy with assert(0) definition. >> >> $ git grep 'assert(0)' | wc -l >> 96 >> >> Should we mass-update and forbid 'assert(0)' adding a check in >> scripts/checkpatch.pl? Otherwise we'll keep getting similar clang >> warnings... > > I just think assert(0) produces a less clean error message, so on that basis yes, we > should replace them all. Perhaps abort() as well, unless there's an error_report > immediately preceding. > If we start exploring this way, why not define explicit noreturn functions for this: unreachable() and todo()/unimplemented(). Names are inspired from the same Rust macros [1]. IMHO, all assert(0) fall into one of those two categories. It's easy to grep, and explicit. The advantage, besides clarity, would be to guarantee it is never removed. I noticed qemu can't be compiled with NDEBUG for now, but with this, it would remove all assert invocations, including assert(0). [1] https://doc.rust-lang.org/std/index.html#macros > The fact that assert(0) was seen to fall through is a system header bug. I see we have a > workaround in include/qemu/osdep.h for __MINGW32__, but I guess this doesn't trigger for > arm64? Pierrick, would you mind testing a change there? > I can confirm this workaround is enabled for windows-arm64. Indeed, from msys2 clangarm64 env: $ echo | cc -dM -E - | grep MINGW #define __MINGW32__ 1 #define __MINGW64__ 1 $ cc --version clang version 15.0.7 Target: aarch64-w64-windows-gnu In more, it is needed. Without the workaround in osdep.h, compilation fails with this message: qemu/include/exec/exec-all.h:619:5: error: call to qemu_build_not_reached_always declared with 'error' attribute: code path is reachable qemu_build_not_reached(); ^ qemu/include/qemu/osdep.h:242:35: note: expanded from macro 'qemu_build_not_reached' #define qemu_build_not_reached() qemu_build_not_reached_always() ^ 1 error generated. > > r~
diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c index cc024316d5..5967ea07a9 100644 --- a/target/ppc/dfp_helper.c +++ b/target/ppc/dfp_helper.c @@ -121,7 +121,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc, case 3: /* use FPSCR rounding mode */ return; default: - assert(0); /* cannot get here */ + g_assert_not_reached(); } } else { /* r == 1 */ switch (rmc & 3) { @@ -138,7 +138,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc, rnd = DEC_ROUND_HALF_DOWN; break; default: - assert(0); /* cannot get here */ + g_assert_not_reached(); } } decContextSetRounding(&dfp->context, rnd);