On 11/16/14 04:31, Gabriel L. Somlo wrote:
> On Sat, Nov 15, 2014 at 12:01:18PM -0600, Scott Duplichan wrote:
>> I am still unclear on philosophy of ASSERT use in EDK2. A complicating
>> factor is that the choice of whether or not to disable ASSERT for release
>> builds is left up to the project. For example, OvmfPkg and ArmPkg disable
>> ASSERT for release builds by defining MDEPKG_NDEBUG in the .dsc file. In
>> addition, ASSERT can be turned off by use of a PCD. So depending on the
>> project, the type of build, and the PcdDebugPropertyMask, ASSERT may or
>> may not do anything. If the ASSERT is used purely as a debug aid, then
>> it doesn't matter.
>>
>> Thanks,
>> Scott
>>
>> + // Compute index into PciHostIrqs[] table by walking
>> + // the device path and adding up all device numbers
>> + //
>> + Status = EFI_NOT_FOUND;
>> + RootSlot = 0;
>> + Idx = PciHdr->Device.InterruptPin - 1;
>> + while (!IsDevicePathEnd (DevPathNode)) {
>> + if (DevicePathType (DevPathNode) == HARDWARE_DEVICE_PATH &&
>> + DevicePathSubType (DevPathNode) == HW_PCI_DP) {
>> +
>> + Idx += ((PCI_DEVICE_PATH *)DevPathNode)->Device;
>> +
>> + //
>> + // Unlike SeaBIOS, which starts climbing from the leaf device
>> + // up toward the root, we traverse the device path starting at
>> + // the root moving toward the leaf node.
>> + // The slot number of the top-level parent bridge is needed for
>> + // Q35 cases with more than 24 slots on the root bus.
>> + //
>> + if (Status != EFI_SUCCESS) {
>> + Status = EFI_SUCCESS;
>> + RootSlot = ((PCI_DEVICE_PATH *)DevPathNode)->Device;
>> + }
>> + }
>> +
>> + DevPathNode = NextDevicePathNode (DevPathNode);
>> + }
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>
> As Laszlo pointed out, at this point we've clearly found a PCI device
> with a non-zero InterruptPin. If its slot on the root PCI bus is 0,
> we're in "uncharted waters" -- as Paolo mentioned, device 0 on the
> root bus (the host bridge) should never be in this situation. SeaBIOS
> makes this assumption implicitly, but I agree it'd be nice if we
> actually checked and "did something" if the unexpected happens.
>
>> + if (RootSlot == 0) {
>> + DEBUG((
>> + EFI_D_ERROR,
>> + "%a: PCI host bridge (00:00.0) should have no interrupts!\n",
>> + __FUNCTION__
>> + ));
>> + ASSERT (FALSE);
>> + }
>
> I was thinking about maybe just replacing the above with
>
> ASSERT (RootSlot > 0);
>
> and relying on whatever macro ASSERT is written on top of to print out
> something useful.
>
> But now that you mention it, I prefer an explicit DEBUG printout if
> ASSERT can be made a NOP during build...
>
> If we continue instead of halting when RootSlot == 0, we just pick an
> irq from the table and set it, which is no better or worse than
> SeaBIOS, which is what I'm using as a reference.
>
> OTOH, if ASSERT() is turned off with the assumption that "we tested it
> and never ran into that case, so we assume it *really* can't ever
> happen", then maybe my "if (RootSlot == 0) " block above really should
> be replaced with "ASSERT (RootSlot > 0);" and nop-ed out in released
> binaries once it's clear it never got triggered during testing.
>
> I can send out v9 with the one-liner ASSERT, or Jordan/Laszlo could
> swap out the block when applying the patch, or leave it as is, or
> maybe we get more ideas from others ? I'm not well enough versed in
> the edk2/ovmf environment to feel strongly either way :)
Here's my thoughts about using ASSERT() in edk2, and assert() in POSIX
just the same.
ASSERT(x) means that the rest of the code takes "x" for granted, because
"!x" is either not possible, as proven by the programmer, or "!x" should
never happen by design. If "!x" happens, then the world must be stopped,
because the program faces an environment that it has never been designed
for (or a proof underlying its operation turns out to be flawed), and it
simply doesn't make sense to contine.
ASSERT(x) also communicates a great deal of information to the human
reader of the code, in two ways: first, code following ASSERT(x) is
allowed to assume "x", second, the human reader might think "aha! I
didn't realize this. Why is 'x' guaranteed here?", and track back, and
reach enlightenment.
Correspondingly, I use ASSERT()s liberally, and I *never ever* disable
them. If there was a standardized macro that behaved exactly as ASSERT()
(or assert()) behaves, without the possibility to ever disable it, I
would use that macro. No such macro has been standardized. Hence, I use
the standard macro, with the caveat that if you ever set MDEPKG_NDEBUG
(or NDEBUG in POSIX), you get to keep both pieces when stuff breaks.
(I make exceptions for predicates whose verification would be so slow as
to defeat the purpose of the software. Those asserts usually depend on
PCDs and such..)
So, I'm okay with this version, and my R-b stands.
Thanks,
Laszlo
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
