Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

[Eric Blake <eblake@redhat.com>]

On 1/10/20 3:50 AM, Greg Kurz wrote:

>>> I guess a decent compiler can be smart enough detect that the initialization
>>> isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
>>> Anyway, reducing scope isn't bad. The only hitch I could see is that some
>>> people do prefer to have all variables declared upfront, but there's a nested
>>> param_val variable already so I guess it's okay.
>>
>> I don't want to outsmart compilers :)

Or conversely play the game of which compilers will warn about an 
atypical construct.

>>
>> The MACHINE() macro is not a simple cast, it does object introspection
>> with OBJECT_CHECK(), thus is not free. Since
> 
> Sure, I understand the motivation in avoiding an unneeded call
> to calling object_dynamic_cast_assert().
> 
>> object_dynamic_cast_assert() argument is not const, I'm not sure the
>> compiler can remove the call.
>>
> 
> Not remove the call, but delay it to the branch that uses it,
> ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS.
> 
>> Richard, Eric, do you know?
>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>    hw/ppc/spapr_rtas.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>> index 6f06e9d7fe..7237e5ebf2 100644
>>>> --- a/hw/ppc/spapr_rtas.c
>>>> +++ b/hw/ppc/spapr_rtas.c
>>>> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>>>>                                              uint32_t nret, target_ulong rets)
>>>>    {
>>>>        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>>> -    MachineState *ms = MACHINE(spapr);
>>>> -    unsigned int max_cpus = ms->smp.max_cpus;
>>>>        target_ulong parameter = rtas_ld(args, 0);
>>>>        target_ulong buffer = rtas_ld(args, 1);
>>>>        target_ulong length = rtas_ld(args, 2);
>>>> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>>>>    
>>>>        switch (parameter) {
>>>>        case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
>>>> +        MachineState *ms = MACHINE(spapr);
>>>> +        unsigned int max_cpus = ms->smp.max_cpus;

Declaring an initializer inside a switch statement can trigger warnings 
under some compilation scenarios (particularly if the variable is 
referenced outside of the scope where it was introduced).  But here, you 
are using 'case label: { ...' to create a scope, which presumably ends 
before the next case label, and is thus not going to trigger compiler 
warnings.

An alternative is indeed leaving the declaration up front but deferring 
the (possibly expensive) initializer to the case label that needs it:

MachineState *ms;
switch (parameter) {
case ...:
   ms = MACHINE(spapr);

and done that way, you might not even need the extra {} behind the case 
label (I didn't read the file to see if there is already some other 
reason for having introduced that sub-scope).

As for whether compilers are smart enough to defer non-trivial 
initialization to the one case label that uses the variable, I wouldn't 
count on it.  If the non-trivial initialization includes a function call 
(which the MACHINE() macro does), it's much harder to prove whether that 
function call has side effects that may be needed prior to the switch 
statement.  So limiting the scope of the initialization (whether by 
dropping the declaration, or just deferring the call) DOES make sense.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org