Re: [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue.

[Laszlo Ersek <lersek@redhat.com>]

On 03/31/17 19:05, Qin Long wrote:
> The compiler (visual studio) may optimize some explicit strcmp call
> to use the intrinsic memcmp call. In CrtLibSupport.h, we just use
>  #define to mapping memcmp to CompareMem API. So in Link phase, this
> kind of intrinsic optimization will cause the "unresolved external
> symbol" error. For example:
>     OpensslLib.lib(v3_utl.obj) : error LNK2001:
>                                unresolved external symbol _memcmp
> 
> This patch will keep the memcmp mapping, and provide extra Intrinsic
> memcmp wrapper to satisfy the symbol link.
> 
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
>  CryptoPkg/Include/CrtLibSupport.h                 | 1 +
>  CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/CryptoPkg/Include/CrtLibSupport.h b/CryptoPkg/Include/CrtLibSupport.h
> index ddf7784a37..7f1ec12302 100644
> --- a/CryptoPkg/Include/CrtLibSupport.h
> +++ b/CryptoPkg/Include/CrtLibSupport.h
> @@ -133,6 +133,7 @@ void           *malloc     (size_t);
>  void           *realloc    (void *, size_t);
>  void           free        (void *);
>  void           *memset     (void *, int, size_t);
> +int            memcmp      (const void *, const void *, size_t);
>  int            isdigit     (int);
>  int            isspace     (int);
>  int            isxdigit    (int);
> diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> index e8a76d07ff..33489aa6f4 100644
> --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> @@ -46,6 +46,12 @@ void * memset (void *dest, char ch, unsigned int count)
>    return dest;
>  }
>  
> +/* Compare characters in two buffers. */
> +int memcmp (const void *buf1, const void *buf2, unsigned int count)
> +{
> +  return (int)(CompareMem(buf1,buf2,(UINTN)(count)));
> +}
> +
>  int strcmp (const char *s1, const char *s2)
>  {
>    return (int)AsciiStrCmp(s1, s2);
> 

Can we just suppress the optimization to the intrinsic with VS?

If we can't, then:

- The prototype and the function definition don't match. The last
argument should be "size_t" in the function definition too, not
"unsigned int".

- No need for the explicit cast to UINTN for "count", because size_t
will match UINTN exactly.

- No need for the parens around "count".

- Please add spaces between the arguments, after the commas.

- the comment should say, "compare bytes", not "compare characters".

- In general, INTN cannot be safely cast to "int" (for the return
value). In practice, it is likely safe, because the CompareMem
implementations in edk2 return byte differences, which can be
represented as "int". So I guess this is safe. (I had the same thought
when I reviewed the strcmp() wrapper.)

- Given that we are adding an actual memcmp() function, can we remove
the #define for it? In commit 420e508397c7 ("CryptoPkg: Fix handling of
&strcmp function pointers", 2017-03-23), we did the same for strcmp():
added the function, removed the macro.

Thanks,
Laszlo