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};
struct kvm_one_reg reg = {
.id = id,
.addr = (uintptr_t) &val,
--
2.35.1On 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.
© 2016 - 2026 Red Hat, Inc.