From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 12B0221A0483D for ; Fri, 31 Mar 2017 18:10:55 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP; 31 Mar 2017 18:10:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,254,1486454400"; d="scan'208";a="840614645" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by FMSMGA003.fm.intel.com with ESMTP; 31 Mar 2017 18:10:55 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 31 Mar 2017 18:10:54 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.253]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.42]) with mapi id 14.03.0248.002; Sat, 1 Apr 2017 09:10:53 +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 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source Thread-Index: AQHSqkvwLBoa1eda0kSJHt+oNebV6aGvs5Dw Date: Sat, 1 Apr 2017 01:10:53 +0000 Message-ID: References: <20170331170517.4672-1-qin.long@intel.com> <20170331170517.4672-2-qin.long@intel.com> <1da75385-b1b8-c10f-e0a1-2925bf3a92f1@redhat.com> In-Reply-To: <1da75385-b1b8-c10f-e0a1-2925bf3a92f1@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 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source 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 01:10:55 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Exactly. Thanks, Laszlo. Since we will keep the openssl source original after this upgrade, I have t= o leverage extra build option to suppress these two warnings this time. Yes, I prefer to keep /Werror as possible, I will follow-up the next opens= sl PR and a TianoCore BZ.=20 Best Regards & Thanks, LONG, Qin > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Saturday, April 01, 2017 2:24 AM > To: Long, Qin; edk2-devel@lists.01.org > Cc: Ye, Ting; Wu, Hao A; Tian, Feng; Dong, Eric > Subject: Re: [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warni= ngs > in openssl source >=20 > On 03/31/17 19:05, Qin Long wrote: > > This patch added some extra build options to suppress possible > > warnings when building openssl source under GCC48 and VS2010. Including= : > > > > Adding "-Wno-error=3Dmaybe-uninitialized" to suppress the following > > GCC48 build warning: > > OpensslLib/openssl/ssl/statem/statem_clnt.c:2543:9: error: "len" may > > be used uninitialized in this function [-Werror=3Dmaybe-uninitiali= zed] > > len +=3D pskhdrlen; > > ^ >=20 > This happens in the tls_construct_client_key_exchange() function. After > reviewing the function, the warning is indeed incorrect. I agree with the= fix > (adding -Wno-error=3Dmaybe-uninitialized), but I propose the following to= o: >=20 > - file an upstream OpenSSL bug report about this (the existent "len =3D 0= " > assignment can be moved to the unconditional part of the code), > - file a TianoCore BZ as well, so that we don't forget removing -Wno- > error=3Dmaybe-uninitialized once the OpenSSL bug is fixed and we adopt th= e > next stable release with the OpenSSL change. >=20 > The point is, I think -Werror=3Dmaybe-uninitialized is a valuable warning= , and it > might help us catch an actual bug in OpenSSL later on. So I don't think i= t > should be turned off permanently. >=20 > > And adding "/wd4306" to suppress the following VS2010 build warning: > > openssl\crypto\asn1\tasn_dec.c(795) : warning C4306: 'type cast' : > > conversion from 'int' to 'ASN1_VALUE *' of greater size >=20 > That again seems like a valid warning, the C standard doesn't guarantee t= hat > (int) can be converted to a pointer type and vice versa. The code should = be > using intptr_t or uintptr_t (in edk2 we use UINTN for that). >=20 > This is the code in question: >=20 > case V_ASN1_NULL: > if (len) { > ASN1err(ASN1_F_ASN1_EX_C2I, ASN1_R_NULL_IS_WRONG_LENGTH); > goto err; > } > *pval =3D (ASN1_VALUE *)1; > break; >=20 > This is really awful. But, if it is necessary, it should be >=20 > *pval =3D (ASN1_VALUE *)(uintptr_t)1; >=20 > Again, I think it's OK to suppress the warnings for now, but I think we s= hould > have a longer term reminder to reenable them. >=20 > > > > Cc: Ting Ye > > Cc: Hao Wu > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Qin Long > > --- > > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 ++++++++++----- > > CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 > > ++++++++++----- > > 2 files changed, 20 insertions(+), 10 deletions(-) > > > > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > b/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > index 42f72f4f1f..cbabb34bdd 100644 > > --- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > +++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > @@ -535,21 +535,26 @@ > > # C4244: conversion from type1 to type2, possible loss of data > > # C4245: conversion from type1 to type2, signed/unsigned mismatch > > # C4267: conversion from size_t to type, possible loss of data > > + # C4306: 'identifier' : conversion from 'type1' to 'type2' of grea= ter size > > # C4389: 'operator' : signed/unsigned mismatch (xxxx) > > # C4702: unreachable code > > # C4706: assignment within conditional expression > > # > > MSFT:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 > /wd4706 > > - MSFT:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 > /wd4706 > > - MSFT:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 > /wd4706 > > + MSFT:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 > /wd4702 /wd4706 > > + MSFT:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 > /wd4702 /wd4706 > > > > INTEL:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER - > U__ICC $(OPENSSL_FLAGS) /w > > INTEL:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER - > U__ICC $(OPENSSL_FLAGS) /w > > INTEL:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER - > U__ICC $(OPENSSL_FLAGS) /w > > > > - GCC:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > > - GCC:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > DNO_MSABI_VA_FUNCS > > - GCC:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > > + # > > + # Suppress the following build warnings in openssl so we don't break= the > build with -Werror > > + # -Werror=3Dmaybe-uninitialized: there exist some other paths for = which > the variable is not initialized. >=20 > If that was true, that would be an actual bug in the OpenSSL code base, a= nd > suppressing the warning in such a case would also be a bug. We may only > suppress the warning because it is *invalid* -- there is no such problema= tic > code path, only the compiler (incorrectly) thinks there is. >=20 > So please update the comment. I would even go as far as including the > TianoCore BZ number in this comment, for the BZ which tracks the longer- > term re-enablement of this warning. >=20 > Looks good to me otherwise. >=20 > (Note: personally I can't reproduce the build issues that I reported, at = least > with GCC48 that I use.) >=20 > Thanks! > Laszlo >=20 > > + # > > + GCC:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > Wno-error=3Dmaybe-uninitialized > > + GCC:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > Wno-error=3Dmaybe-uninitialized -DNO_MSABI_VA_FUNCS > > + GCC:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > Wno-error=3Dmaybe-uninitialized > > GCC:*_*_ARM_CC_FLAGS =3D $(OPENSSL_FLAGS) > > GCC:*_*_AARCH64_CC_FLAGS =3D $(OPENSSL_FLAGS) > > > > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > index cbbb456d70..026b551bca 100644 > > --- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > +++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > @@ -496,21 +496,26 @@ > > # C4244: conversion from type1 to type2, possible loss of data > > # C4245: conversion from type1 to type2, signed/unsigned mismatch > > # C4267: conversion from size_t to type, possible loss of data > > + # C4306: 'identifier' : conversion from 'type1' to 'type2' of grea= ter size > > # C4389: 'operator' : signed/unsigned mismatch (xxxx) > > # C4702: unreachable code > > # C4706: assignment within conditional expression > > # > > MSFT:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 > /wd4706 > > - MSFT:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 > /wd4706 > > - MSFT:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 > /wd4706 > > + MSFT:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 > /wd4702 /wd4706 > > + MSFT:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER > $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 > /wd4702 /wd4706 > > > > INTEL:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER - > U__ICC $(OPENSSL_FLAGS) /w > > INTEL:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER - > U__ICC $(OPENSSL_FLAGS) /w > > INTEL:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 -U_MSC_VER - > U__ICC $(OPENSSL_FLAGS) /w > > > > - GCC:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > > - GCC:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > DNO_MSABI_VA_FUNCS > > - GCC:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > > + # > > + # Suppress the following build warnings in openssl so we don't break= the > build with -Werror > > + # -Werror=3Dmaybe-uninitialized: there exist some other paths for = which > the variable is not initialized. > > + # > > + GCC:*_*_IA32_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > Wno-error=3Dmaybe-uninitialized > > + GCC:*_*_X64_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > Wno-error=3Dmaybe-uninitialized -DNO_MSABI_VA_FUNCS > > + GCC:*_*_IPF_CC_FLAGS =3D -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) - > Wno-error=3Dmaybe-uninitialized > > GCC:*_*_ARM_CC_FLAGS =3D $(OPENSSL_FLAGS) > > GCC:*_*_AARCH64_CC_FLAGS =3D $(OPENSSL_FLAGS) > > > >