Re: [PATCH v2 07/12] Fix some -Werror=maybe-uninitialized

On 2026/07/27 6:07, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 27, 2026 at 12:53 AM Michael S. Tsirkin <[email protected]> wrote:
>>
>> On Mon, Jul 27, 2026 at 12:44:06AM +0400, Marc-André Lureau wrote:
>>> When compiled with -Og, gcc produces many false-positives
>>> gcc (GCC) 16.1.1 20260515 (Red Hat 16.1.1-2).
>>
>> it hurts if you do it? so don't do it then?
> 
> We are not far from getting it working, we can accommodate a bit of
> code while making it a bit clearer for the reader too.
> 
>>
>>> We already use auto-var-init=zero, but better be explicit.
>>
>> explicit about false positives?
> 
> Explicit initialization

These are the same concerns I raised in my previous review:

https://lore.kernel.org/qemu-devel/[email protected]/

I do not think this patch make the code clearer. They add values that 
are never consumed, which obscures rather than clarifies the data flow. 
Leaving a variable uninitialized until its actual value is assigned 
makes that flow more explicit.

Still, I think this change makes sense. Failure with -Og shows that
whether GCC emits the warning depends on subtle optimization details.
Avoiding that dependency is worthwhile.

> 
>>
>>> Signed-off-by: Marc-André Lureau <[email protected]>
>>> ---
>>>   target/i386/cpu.c             | 3 ++-
>>>   target/i386/emulate/x86_mmu.c | 4 ++--
>>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index 5805d33ab92d..28e4435df23f 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -7734,7 +7734,7 @@ static void x86_cpuid_get_avx10_version(Object *obj, Visitor *v,
>>>   static bool x86_cpu_apply_avx10_features(X86CPU *cpu, uint8_t version,
>>>                                            Error **errp)
>>>   {
>>> -    const AVX10VersionDefinition *def;
>>> +    const AVX10VersionDefinition *def = NULL;
>>>       CPUX86State *env = &cpu->env;
>>>
>>>       if (!version) {
>>> @@ -7757,6 +7757,7 @@ static bool x86_cpu_apply_avx10_features(X86CPU *cpu, uint8_t version,
>>>               break;
>>>           }
>>>       }
>>> +    assert(def != NULL);
>>>
>>>       if (def->version < version) {
>>>           error_setg(errp, "avx10-version can be at most %d", def->version);
>>
>>
>> what does assert have to do with gcc warnings?
> 
> That's what is expected at this point, unfortunately the compiler
> doesn't care we help it here. I can drop it

I would keep it. It reduces the downside of introducing an otherwise 
unused initial value by documenting the invariant.

> 
>>> diff --git a/target/i386/emulate/x86_mmu.c b/target/i386/emulate/x86_mmu.c
>>> index 8d4371467fd7..65bcccd4751e 100644
>>> --- a/target/i386/emulate/x86_mmu.c
>>> +++ b/target/i386/emulate/x86_mmu.c
>>> @@ -185,8 +185,8 @@ static MMUTranslateResult walk_gpt(CPUState *cpu, target_ulong addr, MMUTranslat
>>>       int largeness = 0;
>>>       target_ulong cr3 = x86_read_cr(cpu, 3);
>>>       uint64_t page_mask = pae ? PAE_PTE_PAGE_MASK : LEGACY_PTE_PAGE_MASK;
>>> -    MMUTranslateResult res;
>>> -
>>> +    MMUTranslateResult res = MMU_TRANSLATE_PAGE_NOT_MAPPED;
>>> +
>>
>>
>> This is not 0 as the commit log implies.
> 
> Initializing with 0 implies defaulting to MMU_TRANSLATE_SUCCESS - not
> the best choice here.

Since the value is ultimately overwritten anyway, the specific 
placeholder chosen doesn't strictly matter for execution, but 
MMU_TRANSLATE_SUCCESS feels like a more natural default behavior.

walk_gpt() checks each page-table level until one rejects the mapping or
all levels have been traversed. If top_level == 0 (i.e., there is zero 
levels to traverse), nothing would have rejected the mapping, so success 
is more natural than MMU_TRANSLATE_PAGE_NOT_MAPPED.

Regards,
Akihiko Odaki