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 <[email protected]>
> Cc: Michael Kinney <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <[email protected]>
> ---
> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
