From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 41A72820A1 for ; Wed, 8 Feb 2017 01:12:43 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D1C6667EDC; Wed, 8 Feb 2017 09:12:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-31.phx2.redhat.com [10.3.116.31]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v189Cf6Y011952; Wed, 8 Feb 2017 04:12:42 -0500 To: "Bi, Dandan" , "Justen, Jordan L" , "edk2-devel@lists.01.org" References: <1486533352-214148-1-git-send-email-dandan.bi@intel.com> <148653651702.19501.2422588150276024393@jljusten-ivb> <3C0D5C461C9E904E8F62152F6274C0BB3B894002@shsmsx102.ccr.corp.intel.com> Cc: "Gao, Liming" From: Laszlo Ersek Message-ID: <7c1e5f57-5d12-ef3a-4107-958d82b285a1@redhat.com> Date: Wed, 8 Feb 2017 10:12:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <3C0D5C461C9E904E8F62152F6274C0BB3B894002@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 08 Feb 2017 09:12:43 +0000 (UTC) Subject: Re: [patch] OvmfPkg/QemuBootOrderLib: Fix NOOPT build failure X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Feb 2017 09:12:43 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 ; edk2-devel@lists.01.org > Cc: Laszlo Ersek ; Gao, Liming > Subject: Re: [edk2] [patch] OvmfPkg/QemuBootOrderLib: Fix NOOPT build failure > > On 2017-02-07 21:55:52, Dandan Bi wrote: >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Liming Gao >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Dandan Bi >> --- >> 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 >