From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 CBCA7209574F0 for ; Mon, 26 Feb 2018 07:23:54 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1B1D4EAEA6; Mon, 26 Feb 2018 15:29:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-149.rdu2.redhat.com [10.10.120.149]) by smtp.corp.redhat.com (Postfix) with ESMTP id ADA6D2026DFD; Mon, 26 Feb 2018 15:29:56 +0000 (UTC) To: Dandan Bi , edk2-devel@lists.01.org Cc: Michael Kinney , Liming Gao References: <1519626137-88164-1-git-send-email-dandan.bi@intel.com> From: Laszlo Ersek Message-ID: <70bbebe4-cacc-deb5-8153-8a870e37f844@redhat.com> Date: Mon, 26 Feb 2018 16:29:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1519626137-88164-1-git-send-email-dandan.bi@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Mon, 26 Feb 2018 15:29:59 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Mon, 26 Feb 2018 15:29:59 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [patch] MdePkg/BaseSafeIntLib: Fix VS2015 IA32 NOOPT build failure X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Feb 2018 15:23:55 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/26/18 07:22, Dandan Bi wrote: > There are VS2015 NOOPT IA32 build failure like below in BaseSafeIntLib. > XXX.lib(XXX.obj): error LNK2001: unresolved external symbol __allmul > XXX.lib(XXX.obj): error LNK2001: unresolved external symbol __allshl > XXX.lib(XXX.obj): error LNK2001: unresolved external symbol __aullshr > > This patch replaces direct shift/multiplication of 64-bit integer > with related function call to fix these failure. > > Cc: Liming Gao > Cc: Michael Kinney > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Dandan Bi > --- > MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 9 +++++---- > MdePkg/Library/BaseSafeIntLib/SafeIntLib32.c | 3 ++- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c > index c5f13d7..ccffbb6 100644 > --- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c > +++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c > @@ -26,10 +26,11 @@ > > **/ > > #include > #include > +#include (1) Can you introduce a [LibraryClasses] section to the INF file, and list BaseLib there? > > > // > // Magnitude of MIN_INT64 as expressed by a UINT64 number. > // > @@ -3371,12 +3372,12 @@ SafeUint64Mult ( > // a * c must be 0 or there would be bits in the high 64-bits > // a * d must be less than 2^32 or there would be bits in the high 64-bits > // b * c must be less than 2^32 or there would be bits in the high 64-bits > // then there must be no overflow of the resulting values summed up. > // > - DwordA = (UINT32)(Multiplicand >> 32); > - DwordC = (UINT32)(Multiplier >> 32); > + DwordA = (UINT32)RShiftU64 (Multiplicand, 32); > + DwordC = (UINT32)RShiftU64 (Multiplier, 32); > > // > // common case -- if high dwords are both zero, no chance for overflow > // > if ((DwordA == 0) && (DwordC == 0)) { > @@ -3407,11 +3408,11 @@ SafeUint64Mult ( > if ((ProductBC & 0xffffffff00000000) == 0) { > // > // now sum them all up checking for overflow. > // shifting is safe because we already checked for overflow above > // > - if (!RETURN_ERROR (SafeUint64Add (ProductBC << 32, ProductAD << 32, &UnsignedResult))) { > + if (!RETURN_ERROR (SafeUint64Add (LShiftU64(ProductBC, 32), LShiftU64(ProductAD, 32), &UnsignedResult))) { (2) This change is correct, but the style is not perfect. Please insert a space character right after each "LShiftU64" identifier. > // > // b * d > // > ProductBD = (((UINT64)DwordB) *(UINT64)DwordD); > > @@ -4073,11 +4074,11 @@ SafeInt32Mult ( > IN INT32 Multiplicand, > IN INT32 Multiplier, > OUT INT32 *Result > ) > { > - return SafeInt64ToInt32 (((INT64)Multiplicand) *((INT64)Multiplier), Result); > + return SafeInt64ToInt32 (MultS64x64((INT64)Multiplicand, (INT64)Multiplier), Result); > } (3) This can be simplified. The prototype of MultS64x64() is visible here, so you can drop the explicit INT64 casts. (4) Also, please insert a space character after "MultS64x64". (5) Yet another comment: I'm surprised that, while this 64-bit multiplication is flagged, several 64-bit (unsigned) multiplications are (apparently) not, in SafeUint64Mult(): 3386 *Result = (((UINT64)DwordB) *(UINT64)DwordD); 3399 ProductAD = (((UINT64)DwordA) *(UINT64)DwordD); 3406 ProductBC = (((UINT64)DwordB) *(UINT64)DwordC); 3416 ProductBD = (((UINT64)DwordB) *(UINT64)DwordD); > > /** > INT64 multiplication > > diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib32.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib32.c > index 18bfb9e..5e30db4 100644 > --- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib32.c > +++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib32.c > @@ -26,10 +26,11 @@ > > **/ > > #include > #include > +#include > > /** > INT32 -> UINTN conversion > > Converts the value specified by Operand to a value specified by Result type > @@ -547,8 +548,8 @@ SafeIntnMult ( > IN INTN Multiplicand, > IN INTN Multiplier, > OUT INTN *Result > ) > { > - return SafeInt64ToIntn (((INT64)Multiplicand) *((INT64)Multiplier), Result); > + return SafeInt64ToIntn (MultS64x64((INT64)Multiplicand, (INT64)Multiplier), Result); > } > > (6) Comments (3) and (4) apply here. Thanks! Laszlo