From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 57FE921A0483B for ; Fri, 31 Mar 2017 11:23:44 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9E96280F6C; Fri, 31 Mar 2017 18:23:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9E96280F6C Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9E96280F6C Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-153.phx2.redhat.com [10.3.116.153]) by smtp.corp.redhat.com (Postfix) with ESMTP id D172653; Fri, 31 Mar 2017 18:23:41 +0000 (UTC) To: Qin Long , edk2-devel@lists.01.org References: <20170331170517.4672-1-qin.long@intel.com> <20170331170517.4672-2-qin.long@intel.com> Cc: ting.ye@intel.com, hao.a.wu@intel.com, feng.tian@intel.com, eric.dong@intel.com From: Laszlo Ersek Message-ID: <1da75385-b1b8-c10f-e0a1-2925bf3a92f1@redhat.com> Date: Fri, 31 Mar 2017 20:23:37 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170331170517.4672-2-qin.long@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 31 Mar 2017 18:23:43 +0000 (UTC) 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: Fri, 31 Mar 2017 18:23:44 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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=maybe-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=maybe-uninitialized] > len += pskhdrlen; > ^ 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=maybe-uninitialized), but I propose the following too: - file an upstream OpenSSL bug report about this (the existent "len = 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=maybe-uninitialized once the OpenSSL bug is fixed and we adopt the next stable release with the OpenSSL change. The point is, I think -Werror=maybe-uninitialized is a valuable warning, and it might help us catch an actual bug in OpenSSL later on. So I don't think it should be turned off permanently. > 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 That again seems like a valid warning, the C standard doesn't guarantee that (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). This is the code in question: case V_ASN1_NULL: if (len) { ASN1err(ASN1_F_ASN1_EX_C2I, ASN1_R_NULL_IS_WRONG_LENGTH); goto err; } *pval = (ASN1_VALUE *)1; break; This is really awful. But, if it is necessary, it should be *pval = (ASN1_VALUE *)(uintptr_t)1; Again, I think it's OK to suppress the warnings for now, but I think we should have a longer term reminder to reenable them. > > 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 greater size > # C4389: 'operator' : signed/unsigned mismatch (xxxx) > # C4702: unreachable code > # C4706: assignment within conditional expression > # > MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706 > - MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706 > - MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706 > + MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706 > + MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706 > > INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w > INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w > INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w > > - GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > - GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS > - GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > + # > + # Suppress the following build warnings in openssl so we don't break the build with -Werror > + # -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized. If that was true, that would be an actual bug in the OpenSSL code base, and 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 problematic code path, only the compiler (incorrectly) thinks there is. 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. Looks good to me otherwise. (Note: personally I can't reproduce the build issues that I reported, at least with GCC48 that I use.) Thanks! Laszlo > + # > + GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized > + GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS > + GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized > GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS) > GCC:*_*_AARCH64_CC_FLAGS = $(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 greater size > # C4389: 'operator' : signed/unsigned mismatch (xxxx) > # C4702: unreachable code > # C4706: assignment within conditional expression > # > MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706 > - MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706 > - MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706 > + MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706 > + MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706 > > INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w > INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w > INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w > > - GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > - GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS > - GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) > + # > + # Suppress the following build warnings in openssl so we don't break the build with -Werror > + # -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized. > + # > + GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized > + GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS > + GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized > GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS) > GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS) > >