On Tue, Nov 11, 2014 at 07:54:30PM -0800, Jordan Justen wrote:
> > > When I build IA32 (OvmfPkg/build.sh -a IA32) I get warnings about
> > > possibly uninitialized variables.
> > >
> > > It seems to happen basically everwhere that you switch on
> > > HostBridgeDevId.
> >
> > I wonder why it's only warning on IA32, not like its different on X64 :)
> >
> > > We should default to setting variables to the PIIX4 values in this
> > > case.
> > >
> > > I'm not sure if we should just skip the assert and use the PIIX4
> > > values.
> >
> > With the understanding that I will obviously defer to your judgment
> > regarding style, I actually *like* the switch statement, complete with
> > ASSERT(FALSE) if something "surprising" happens that causes the world
> > to end... :)
> >
> > Assuming we're still on the same page, the question becomes how to
> > shut up gcc. We could initialize PmCmd, Pmba, and PmRegMisc to 0 when
> > we declare them:
> >
> > UINTN PmCmd = 0;
> > UINTN Pmba = 0;
> > UINTN PmRegMisc = 0;
>
> Unfortunately, it looks like the Coding Style document states:
> "Initializing a variable as part of its declaration is illegal."
> http://sourceforge.net/projects/edk2/files/General%20Documentation/EDK%20II%20C%20Coding%20Standards%20Specification.pdf/download
>
> > or right before the switch statement, but that feels cheesy because
> > it's gratuitous, so I don't like either alternative.
> >
> > Right now, I'm very tempted to initialize them to 0 *after* the
> > ASSERT(FALSE) with a comment:
> >
> > HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> > switch (HostBridgeDevId) {
> > case INTEL_82441_DEVICE_ID:
> > PmCmd = POWER_MGMT_REGISTER_PIIX4 (PCI_COMMAND_OFFSET);
> > Pmba = POWER_MGMT_REGISTER_PIIX4 (0x40);
> > PmRegMisc = POWER_MGMT_REGISTER_PIIX4 (0x80);
> > break;
> > case INTEL_Q35_MCH_DEVICE_ID:
> > PmCmd = POWER_MGMT_REGISTER_Q35 (PCI_COMMAND_OFFSET);
> > Pmba = POWER_MGMT_REGISTER_Q35 (0x40);
> > PmRegMisc = POWER_MGMT_REGISTER_Q35 (0x80);
> > break;
> > default:
> > DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: x%04x\n",
> > __FUNCTION__, HostBridgeDevId));
> > ASSERT (FALSE);
> > + PmCmd = Pmba = PmRegMisc = 0; // suppress uninitialized use gcc
> > warning
>
> But, in this case, I think I prefer to allow PIIX4 to be used as a
> default. Well, only for release builds, but... how about this? :
>
> switch (HostBridgeDevId) {
> case INTEL_Q35_MCH_DEVICE_ID:
> PmCmd = POWER_MGMT_REGISTER_Q35 (PCI_COMMAND_OFFSET);
> Pmba = POWER_MGMT_REGISTER_Q35 (0x40);
> PmRegMisc = POWER_MGMT_REGISTER_Q35 (0x80);
> break;
> default:
> DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: x%04x\n",
> __FUNCTION__, HostBridgeDevId));
> ASSERT (FALSE);
> //
> // Fall through to use PIIX4 locations by default
> //
> case INTEL_82441_DEVICE_ID:
> PmCmd = POWER_MGMT_REGISTER_PIIX4 (PCI_COMMAND_OFFSET);
> Pmba = POWER_MGMT_REGISTER_PIIX4 (0x40);
> PmRegMisc = POWER_MGMT_REGISTER_PIIX4 (0x80);
> break;
> }
>
Hmmm, if I looked at this a few years down the road, I'll most likely
end up wondering if there was a clever reason to word it like that,
and simply shutting up compiler warnings would be low on my list of
suspects :)
I think I'll just go for this instead:
> UINTN PmCmd;
> UINTN Pmba;
> UINTN PmRegMisc;
>
//
// suppress uninitialized use gcc warning
//
> PmCmd = 0;
> Pmba = 0;
> PmRegMisc = 0;
>
if it's still an option :)
Thanks,
--Gabriel
------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
