On 11/13/14 18:40, Gabriel Somlo wrote: > On Thu, 13 Nov 2014 09:27:09 -0600, Scott Duplichan wrote: >> Gabriel Somlo [mailto:[email protected]] wrote: >> ]On Sun, Nov 09, 2014 at 05:00:38PM -0800, Jordan Justen wrote: >> ]> On 2014-11-08 14:01:56, Gabriel L. Somlo wrote: >> ]> > @@ -228,6 +229,11 @@ MiscInitialization ( >> ]> > VOID >> ]> > ) >> ]> > { >> ]> > + UINT16 HostBridgeDevId; >> ]> > + UINTN PmCmd; >> ]> > + UINTN Pmba; >> ]> > + UINTN PmRegMisc; >> ]> > + >> ]> > // >> ]> > // Disable A20 Mask >> ]> > // >> ]> > @@ -239,33 +245,47 @@ MiscInitialization ( >> ]> > BuildCpuHob (36, 16); >> ]> > >> ]> > // >> ]> > + // Query Host Bridge DID to determine platform type >> ]> > + // >> ]> > + 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: >> 0x%04x\n", >> ]> > + __FUNCTION__, HostBridgeDevId)); >> ]> > + ASSERT (FALSE); >> ]> >> ]> 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 :) >> >> Because GCC49 IA32 is an optimized build (-Os) and GCC49 X64 is not. > > OK, thanks, that makes sense :) > >> ]Assuming we're still on the same page, the question becomes how to >> ]shut up gcc. >> >> Using a phrase like 'shut up gcc' sort of implies the warning is in >> the bogus category, like so many from the Microsoft compilers. But >> this warning is valid, right? For example, if I run Ovmf in some >> environment where PIIX4 is not present, then the code will fail >> to find the ACPI timer I/O address. Why not return an error if >> no supported device ID is found. That will eliminate the warning, >> and prevent the code from running on the unsupported chipset. > > I'm using this type of switch statement from several places, some > of which return VOID. Right now, that's > > OvmfPkg/PlatformPei/Platform.c MiscInitialization () > OvmfPkg/Csm/CsmSupportLib/LegacyInterrupt.c LegacyInterruptInstall () > OvmfPkg/Library/AcpiTimerLib/*AcpiTimerLib.c AcpiTimerLibConstructor () > OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c PciAcpiInitialization () > > I could add a "return EFI_UNSUPPORTED" for those of them which return > a status code, or simply a "return" for those of type VOID, however: > > I'd still basically be just "shutting up gcc", because I'd place these > return statements after the ASSERT (FALSE). > > Once neither host bridge type we expect has been detected, the world > ends and returning "for real" makes no sense, as we're in an > unexpected/undefined state. Hence the ASSERT. > > Jordan, what do you think ? Adding a "return [EFI_UNSUPPORTED]" after > the ASSERT (FALSE), is that stylistically preferable over zeroing > out the values before the switch() ?
(chiming in:) both have been employed in edk2. I *slightly* prefer the return statement after the ASSERT(), because that one makes the purpose obvious even without comments. just my two cents Thanks Laszlo ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
