From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 D207D21A0483C for ; Fri, 31 Mar 2017 19:31:40 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga105.jf.intel.com with ESMTP; 31 Mar 2017 19:31:40 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,255,1486454400"; d="scan'208";a="67641932" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 31 Mar 2017 19:31:40 -0700 Received: from fmsmsx116.amr.corp.intel.com (10.18.116.20) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 31 Mar 2017 19:31:40 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx116.amr.corp.intel.com (10.18.116.20) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 31 Mar 2017 19:31:39 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.253]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.82]) with mapi id 14.03.0248.002; Sat, 1 Apr 2017 10:31:36 +0800 From: "Long, Qin" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Ye, Ting" , "Wu, Hao A" , "Tian, Feng" , "Dong, Eric" Thread-Topic: [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue. Thread-Index: AQHSqk7b/76477O1UE6asKKyKz7tiKGvtQtg Date: Sat, 1 Apr 2017 02:31:36 +0000 Message-ID: References: <20170331170517.4672-1-qin.long@intel.com> <20170331170517.4672-3-qin.long@intel.com> <2a380b37-ca12-6d00-5c83-9c77bda8d8a3@redhat.com> In-Reply-To: <2a380b37-ca12-6d00-5c83-9c77bda8d8a3@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Apr 2017 02:31:41 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 symb= ol > issue. >=20 > 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 > > Cc: Feng Tian > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Qin Long > > --- > > 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); > > >=20 > Can we just suppress the optimization to the intrinsic with VS? Noop.=20 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. :-( >=20 > If we can't, then: >=20 > - The prototype and the function definition don't match. The last argumen= t > should be "size_t" in the function definition too, not "unsigned int". >=20 > - No need for the explicit cast to UINTN for "count", because size_t will > match UINTN exactly. >=20 > - No need for the parens around "count". >=20 > - Please add spaces between the arguments, after the commas. >=20 > - the comment should say, "compare bytes", not "compare characters". Make sense for me. Will reproduce the patch to refine these. >=20 > - In general, INTN cannot be safely cast to "int" (for the return value).= In > practice, it is likely safe, because the CompareMem implementations in ed= k2 > return byte differences, which can be represented as "int". So I guess th= is is > safe. (I had the same thought when I reviewed the strcmp() wrapper.) >=20 > - Given that we are adding an actual memcmp() function, can we remove the > #define for it? In commit 420e508397c7 ("CryptoPkg: Fix handling of &strc= mp > 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 memcm= p calls (several places in openssl source) to UEFI functions directly.=20 The intrinsic memcmp wrapper is just to link those optimized _memcmp symbol= . >=20 > Thanks, > Laszlo