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-boun...@lists.01.org] On Behalf Of Jordan > Justen > Sent: Wednesday, February 8, 2017 2:49 PM > To: Bi, Dandan <dandan...@intel.com>; edk2-devel@lists.01.org > Cc: Laszlo Ersek <ler...@redhat.com>; Gao, Liming <liming....@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.jus...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Liming Gao <liming....@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Dandan Bi <dandan...@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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
