On 7/17/25 9:56 AM, Peter Maydell wrote:
On Mon, 14 Jul 2025 at 16:41, Pierrick Bouvier
<pierrick.bouv...@linaro.org> wrote:
As folding is not guaranteed by C standard, I'm not sure it's really
possible to file a bug. However, since we rely on this behaviour in
other parts, maybe it would be better to rewrite the condition on our side.
By changing the code to this, the folding happens as expected.
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 26cf7e6dfa2..af5788dafab 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -318,9 +318,11 @@ static void cpu_arm_set_sve(Object *obj, bool
value, Error **errp)
{
ARMCPU *cpu = ARM_CPU(obj);
- if (value && kvm_enabled() && !kvm_arm_sve_supported()) {
- error_setg(errp, "'sve' feature not supported by KVM on this
host");
- return;
+ if (value) {
+ if (kvm_enabled() && !kvm_arm_sve_supported()) {
+ error_setg(errp, "'sve' feature not supported by KVM on
this host");
+ return;
+ }
}
FIELD_DP64_IDREG(&cpu->isar, ID_AA64PFR0, SVE, value);
If you prefer keeping your patch, I'm ok, but fixing the condition looks
better to me (as we already rely on constant folding in other places).
I'm not really a fan of relying on the compiler to fold stuff
away -- it's fragile and there's no guarantee the compiler
will actually do it. In this example it would be really easy
for somebody coming along to tidy this up later to put the
nested if()s back into a single if() condition and reintroduce
the problem, for instance.
There are various places where we already relied on that, including
before the single-binary work ({accel}_allowed).
For the fragile aspect of it, that's why CI exists. Building all
binaries with clang --enable-debug should ensure no regression can be
introduced.
I've applied this patch to target-arm.next; thanks for the review.
Thank you Peter.
-- PMM
