| Message ID | 20220330210443.597500-2-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: valgrind "uninitialized values" fixes | expand |
On 30/3/22 23:04, Daniel Henrique Barboza wrote: > Valgrind isn't convinced that we are initializing the values we assign > to env->spr[spr] because it doesn't understand that the 'reg_val' union > is being written by the kvm_vcpu_ioctl() that follows (via struct > kvm_one_reg). > > This results in Valgrind complaining about uninitialized values every > time we use env->spr in a conditional, like this instance: > > ==707578== Thread 1: > ==707578== Conditional jump or move depends on uninitialised value(s) > ==707578== at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106) > ==707578== by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173) > ==707578== by 0xA110F7: hreg_store_msr (helper_regs.c:262) > ==707578== by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168) > ==707578== by 0xD4730F: device_transitional_reset (qdev.c:799) > ==707578== by 0xD4A11B: resettable_phase_hold (resettable.c:182) > ==707578== by 0xD49A77: resettable_assert_reset (resettable.c:60) > ==707578== by 0xD4994B: resettable_reset (resettable.c:45) > ==707578== by 0xD458BB: device_cold_reset (qdev.c:296) > ==707578== by 0x48FBC7: cpu_reset (cpu-common.c:114) > ==707578== by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38) > ==707578== by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209) > ==707578== Uninitialised value was created by a stack allocation > ==707578== at 0xB11F08: kvm_get_one_spr (kvm.c:543) > > Initializing 'reg_val' has no impact in the logic and makes Valgrind > output more bearable. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > target/ppc/kvm.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index dc93b99189..ce1b926e8c 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr) > { > PowerPCCPU *cpu = POWERPC_CPU(cs); > CPUPPCState *env = &cpu->env; > - union { > + union reg_val { > uint32_t u32; > uint64_t u64; > - } val; > + }; > + /* Init reg_val to avoid "uninitialised value" Valgrind warnings */ > + union reg_val val = {0}; This should also work: -- >8 -- @@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr) union { uint32_t u32; uint64_t u64; - } val; + } val = { 0 }; ---
On 3/30/22 18:22, Philippe Mathieu-Daudé wrote: > On 30/3/22 23:04, Daniel Henrique Barboza wrote: >> Valgrind isn't convinced that we are initializing the values we assign >> to env->spr[spr] because it doesn't understand that the 'reg_val' union >> is being written by the kvm_vcpu_ioctl() that follows (via struct >> kvm_one_reg). >> >> This results in Valgrind complaining about uninitialized values every >> time we use env->spr in a conditional, like this instance: >> >> ==707578== Thread 1: >> ==707578== Conditional jump or move depends on uninitialised value(s) >> ==707578== at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106) >> ==707578== by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173) >> ==707578== by 0xA110F7: hreg_store_msr (helper_regs.c:262) >> ==707578== by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168) >> ==707578== by 0xD4730F: device_transitional_reset (qdev.c:799) >> ==707578== by 0xD4A11B: resettable_phase_hold (resettable.c:182) >> ==707578== by 0xD49A77: resettable_assert_reset (resettable.c:60) >> ==707578== by 0xD4994B: resettable_reset (resettable.c:45) >> ==707578== by 0xD458BB: device_cold_reset (qdev.c:296) >> ==707578== by 0x48FBC7: cpu_reset (cpu-common.c:114) >> ==707578== by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38) >> ==707578== by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209) >> ==707578== Uninitialised value was created by a stack allocation >> ==707578== at 0xB11F08: kvm_get_one_spr (kvm.c:543) >> >> Initializing 'reg_val' has no impact in the logic and makes Valgrind >> output more bearable. >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> target/ppc/kvm.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >> index dc93b99189..ce1b926e8c 100644 >> --- a/target/ppc/kvm.c >> +++ b/target/ppc/kvm.c >> @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr) >> { >> PowerPCCPU *cpu = POWERPC_CPU(cs); >> CPUPPCState *env = &cpu->env; >> - union { >> + union reg_val { >> uint32_t u32; >> uint64_t u64; >> - } val; >> + }; >> + /* Init reg_val to avoid "uninitialised value" Valgrind warnings */ >> + union reg_val val = {0}; > > This should also work: > > -- >8 -- > @@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr) > union { > uint32_t u32; > uint64_t u64; > - } val; > + } val = { 0 }; Apparently it does work. I'll make a few tests and re-send. Also, do we have an official way of spelling this zeroed struct initialization? I see several instances of {0} and { 0 } in the code. In this series I used {0}. ./scripts/checkpatch.pl seems ok with both formats. Thanks, Daniel > ---
On 30/3/22 23:34, Daniel Henrique Barboza wrote: > On 3/30/22 18:22, Philippe Mathieu-Daudé wrote: >> On 30/3/22 23:04, Daniel Henrique Barboza wrote: >>> Valgrind isn't convinced that we are initializing the values we assign >>> to env->spr[spr] because it doesn't understand that the 'reg_val' union >>> is being written by the kvm_vcpu_ioctl() that follows (via struct >>> kvm_one_reg). >>> >>> This results in Valgrind complaining about uninitialized values every >>> time we use env->spr in a conditional, like this instance: >>> >>> ==707578== Thread 1: >>> ==707578== Conditional jump or move depends on uninitialised value(s) >>> ==707578== at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106) >>> ==707578== by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173) >>> ==707578== by 0xA110F7: hreg_store_msr (helper_regs.c:262) >>> ==707578== by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168) >>> ==707578== by 0xD4730F: device_transitional_reset (qdev.c:799) >>> ==707578== by 0xD4A11B: resettable_phase_hold (resettable.c:182) >>> ==707578== by 0xD49A77: resettable_assert_reset (resettable.c:60) >>> ==707578== by 0xD4994B: resettable_reset (resettable.c:45) >>> ==707578== by 0xD458BB: device_cold_reset (qdev.c:296) >>> ==707578== by 0x48FBC7: cpu_reset (cpu-common.c:114) >>> ==707578== by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38) >>> ==707578== by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209) >>> ==707578== Uninitialised value was created by a stack allocation >>> ==707578== at 0xB11F08: kvm_get_one_spr (kvm.c:543) >>> >>> Initializing 'reg_val' has no impact in the logic and makes Valgrind >>> output more bearable. >>> >>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> --- >>> target/ppc/kvm.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>> index dc93b99189..ce1b926e8c 100644 >>> --- a/target/ppc/kvm.c >>> +++ b/target/ppc/kvm.c >>> @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, >>> uint64_t id, int spr) >>> { >>> PowerPCCPU *cpu = POWERPC_CPU(cs); >>> CPUPPCState *env = &cpu->env; >>> - union { >>> + union reg_val { >>> uint32_t u32; >>> uint64_t u64; >>> - } val; >>> + }; >>> + /* Init reg_val to avoid "uninitialised value" Valgrind warnings */ >>> + union reg_val val = {0}; >> >> This should also work: >> >> -- >8 -- >> @@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t >> id, int spr) >> union { >> uint32_t u32; >> uint64_t u64; >> - } val; >> + } val = { 0 }; > > Apparently it does work. I'll make a few tests and re-send. > > > Also, do we have an official way of spelling this zeroed struct > initialization? I > see several instances of {0} and { 0 } in the code. In this series I > used {0}. > ./scripts/checkpatch.pl seems ok with both formats. $ git grep -F '= {0}' | wc -l 81 $ git grep -F '= { 0 }' | wc -l 112 $ git grep -F '= { }' | wc -l 61 $ git grep -F '= {}' | wc -l 368 I am not aware of an official way. Apparently '{ }' is the way to go.
On 30/3/22 23:38, Philippe Mathieu-Daudé wrote: > On 30/3/22 23:34, Daniel Henrique Barboza wrote: >> On 3/30/22 18:22, Philippe Mathieu-Daudé wrote: >>> On 30/3/22 23:04, Daniel Henrique Barboza wrote: >>>> Valgrind isn't convinced that we are initializing the values we assign >>>> to env->spr[spr] because it doesn't understand that the 'reg_val' union >>>> is being written by the kvm_vcpu_ioctl() that follows (via struct >>>> kvm_one_reg). >>>> >>>> This results in Valgrind complaining about uninitialized values every >>>> time we use env->spr in a conditional, like this instance: >>>> >>>> ==707578== Thread 1: >>>> ==707578== Conditional jump or move depends on uninitialised value(s) >>>> ==707578== at 0xA10A40: hreg_compute_hflags_value >>>> (helper_regs.c:106) >>>> ==707578== by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173) >>>> ==707578== by 0xA110F7: hreg_store_msr (helper_regs.c:262) >>>> ==707578== by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168) >>>> ==707578== by 0xD4730F: device_transitional_reset (qdev.c:799) >>>> ==707578== by 0xD4A11B: resettable_phase_hold (resettable.c:182) >>>> ==707578== by 0xD49A77: resettable_assert_reset (resettable.c:60) >>>> ==707578== by 0xD4994B: resettable_reset (resettable.c:45) >>>> ==707578== by 0xD458BB: device_cold_reset (qdev.c:296) >>>> ==707578== by 0x48FBC7: cpu_reset (cpu-common.c:114) >>>> ==707578== by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38) >>>> ==707578== by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209) >>>> ==707578== Uninitialised value was created by a stack allocation >>>> ==707578== at 0xB11F08: kvm_get_one_spr (kvm.c:543) >>>> >>>> Initializing 'reg_val' has no impact in the logic and makes Valgrind >>>> output more bearable. >>>> >>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>> --- >>>> target/ppc/kvm.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >>>> index dc93b99189..ce1b926e8c 100644 >>>> --- a/target/ppc/kvm.c >>>> +++ b/target/ppc/kvm.c >>>> @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, >>>> uint64_t id, int spr) >>>> { >>>> PowerPCCPU *cpu = POWERPC_CPU(cs); >>>> CPUPPCState *env = &cpu->env; >>>> - union { >>>> + union reg_val { >>>> uint32_t u32; >>>> uint64_t u64; >>>> - } val; >>>> + }; >>>> + /* Init reg_val to avoid "uninitialised value" Valgrind >>>> warnings */ >>>> + union reg_val val = {0}; >>> >>> This should also work: >>> >>> -- >8 -- >>> @@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, >>> uint64_t id, int spr) >>> union { >>> uint32_t u32; >>> uint64_t u64; >>> - } val; >>> + } val = { 0 }; >> >> Apparently it does work. I'll make a few tests and re-send. >> >> >> Also, do we have an official way of spelling this zeroed struct >> initialization? I >> see several instances of {0} and { 0 } in the code. In this series I >> used {0}. >> ./scripts/checkpatch.pl seems ok with both formats. > > $ git grep -F '= {0}' | wc -l > 81 > $ git grep -F '= { 0 }' | wc -l > 112 > $ git grep -F '= { }' | wc -l > 61 > $ git grep -F '= {}' | wc -l > 368 > > I am not aware of an official way. Apparently '{ }' is the way to go. I meant '{}' but personally I prefer '{ }' for readability.
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index dc93b99189..ce1b926e8c 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; - union { + union reg_val { uint32_t u32; uint64_t u64; - } val; + }; + /* Init reg_val to avoid "uninitialised value" Valgrind warnings */ + union reg_val val = {0}; struct kvm_one_reg reg = { .id = id, .addr = (uintptr_t) &val,
Valgrind isn't convinced that we are initializing the values we assign to env->spr[spr] because it doesn't understand that the 'reg_val' union is being written by the kvm_vcpu_ioctl() that follows (via struct kvm_one_reg). This results in Valgrind complaining about uninitialized values every time we use env->spr in a conditional, like this instance: ==707578== Thread 1: ==707578== Conditional jump or move depends on uninitialised value(s) ==707578== at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106) ==707578== by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173) ==707578== by 0xA110F7: hreg_store_msr (helper_regs.c:262) ==707578== by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168) ==707578== by 0xD4730F: device_transitional_reset (qdev.c:799) ==707578== by 0xD4A11B: resettable_phase_hold (resettable.c:182) ==707578== by 0xD49A77: resettable_assert_reset (resettable.c:60) ==707578== by 0xD4994B: resettable_reset (resettable.c:45) ==707578== by 0xD458BB: device_cold_reset (qdev.c:296) ==707578== by 0x48FBC7: cpu_reset (cpu-common.c:114) ==707578== by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38) ==707578== by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209) ==707578== Uninitialised value was created by a stack allocation ==707578== at 0xB11F08: kvm_get_one_spr (kvm.c:543) Initializing 'reg_val' has no impact in the logic and makes Valgrind output more bearable. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- target/ppc/kvm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)