Re: [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source

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

Exactly. Thanks, Laszlo.
Since we will keep the openssl source original after this upgrade, I have to 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 openssl PR and a TianoCore BZ. 


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 warnings
> in openssl source
> 
> 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 <ting.ye@intel.com>
> > Cc: Hao Wu <hao.a.wu@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/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)
> >
> >