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

["Long, Qin" <qin.long@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, April 01, 2017 2:45 AM
> To: Long, Qin; edk2-devel@lists.01.org
> Cc: Ye, Ting; Wu, Hao A; Tian, Feng; Dong, Eric
> Subject: Re: [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol
> issue.
> 
> 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?

Noop. 
We leverage the global optimization option (/GL) under VS.  So the compiler
may still produce the intrinsic replacement even if we use some kind of
no-instrinsic options. :-(

> 
> 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".

Make sense for me. Will reproduce the patch to refine these.

> 
> - 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.

I prefer to keep the memcmp mapping which will replace those explicit memcmp
calls (several places in openssl source) to UEFI functions directly. 
The intrinsic memcmp wrapper is just to link those optimized _memcmp symbol.

> 
> Thanks,
> Laszlo