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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
   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





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux