public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Dandan Bi <dandan.bi@intel.com>, edk2-devel@lists.01.org
Cc: Michael Kinney <michael.d.kinney@intel.com>,
	Liming Gao <liming.gao@intel.com>
Subject: Re: [patch] MdePkg/BaseSafeIntLib: Fix VS2015 IA32 NOOPT build failure
Date: Mon, 26 Feb 2018 16:29:55 +0100	[thread overview]
Message-ID: <70bbebe4-cacc-deb5-8153-8a870e37f844@redhat.com> (raw)
In-Reply-To: <1519626137-88164-1-git-send-email-dandan.bi@intel.com>

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 <liming.gao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  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 <Base.h>
>  #include <Library/SafeIntLib.h>
> +#include <Library/BaseLib.h>

(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 <Base.h>
>  #include <Library/SafeIntLib.h>
> +#include <Library/BaseLib.h>
>  
>  /**
>    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


      reply	other threads:[~2018-02-26 15:23 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-26  6:22 [patch] MdePkg/BaseSafeIntLib: Fix VS2015 IA32 NOOPT build failure Dandan Bi
2018-02-26 15:29 ` Laszlo Ersek [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=70bbebe4-cacc-deb5-8153-8a870e37f844@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox