Re: [patch] OvmfPkg/QemuBootOrderLib: Fix NOOPT build failure

[Laszlo Ersek <lersek@redhat.com>]

On 02/08/17 08:15, Bi, Dandan wrote:
> Hi  Jordan,
> 
> Yes, it fails on IA32.
> As far as I know, it impacts VS2012/2013/2015. 

Thank you for confirming.

(Unfortunately, a few months back I lost the virtual machine in which I
had installed VS2015, and since then I haven't found the energy to spend
several days again on setting up VS2015. Last time it was an exercise in
frustration -- trial and error.)

> I will create a new patch and add the impacted toolchain and arch info to the commit message.

That's good, yes.

I have more comments below:

> 
> Thanks,
> Dandan
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jordan Justen
> Sent: Wednesday, February 8, 2017 2:49 PM
> To: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [patch] OvmfPkg/QemuBootOrderLib: Fix NOOPT build failure
> 
> On 2017-02-07 21:55:52, Dandan Bi wrote:
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
>> ---
>>  OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c 
>> b/OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c
>> index ec42214..1bb89d4 100644
>> --- a/OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c
>> +++ b/OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c
>> @@ -306,8 +306,8 @@ MapRootBusPosToBusNr (
>>      return EFI_INVALID_PARAMETER;
>>    }
>>    if (RootBusPos > ExtraRootBusMap->Count) {
>>      return EFI_NOT_FOUND;
>>    }
>> -  *RootBusNr = ExtraRootBusMap->BusNumbers[RootBusPos - 1];
>> +  *RootBusNr = ExtraRootBusMap->BusNumbers[(UINT32)RootBusPos - 1];
> 
> Is this failing on IA32? If so, think UINTN would be better.

UINTN is infinitely better, because just before the line that's being
modified, we have the following check (see it in the context):

  if (RootBusPos > ExtraRootBusMap->Count) {
    return EFI_NOT_FOUND;
  }

The "ExtraRootBusMap->Count" field has type UINTN. Therefore, if the
check passes, we can be sure that "RootBusPos" fits in a UINTN.

(I guess this is also why *only* the NOOPT build fails with VS -- with
optimization enabled (DEBUG or RELEASE), the compiler can deduce the
upper bound on RootBusPos, and sees that RootBusPos will fit in the
natural word size for indexing.)

> 
> You might also want to add the impacted toolchain and arch to the commit message.

Right, good idea. Even in NOOPT mode, I firmly believe that VS is wrong
to warn about this, so the least we should do here is capture the lame
excuse that VS makes for breaking the build. :/

(NB: I'm not saying that gcc's warnings are much better. Its warnings
for 100% valid C code can be super infuriating too!)

I have an actual argument why the warning is useless. Namely, even if
"RootBusPos" is cast to UINTN before passing it to the subscript
operator [], the element type of the array "ExtraRootBusMap->BusNumbers"
is UINT32.

Which means that at some point, the processor will perform a
multiplication by 4. The product (i.e., the *byte* offset) can overflow
the UINTN representation just as well, if the programmer was careless
and omitted the bounds check.

Hence, the warning does not catch a missed bounds check, but at least it
breaks valid code that *does* perform the bounds check. Yay?

... Or is the warning about the internal multiplication being 64-bit in
the first place, which cannot be done implicitly in an IA32 build? That
sounds more in-line with the edk2 coding style. I'm curious.

Thanks,
Laszlo

> 
> -Jordan
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>