Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain

["Shi, Steven" <steven.shi@intel.com>]

Besides the GetBestLanguage() , there are several other lib functions have the same problem which are listed in attachment https://bugzilla.tianocore.org/attachment.cgi?id=97 of the Bug410. These functions can also cause the build fails with CLANG38 in my side with clang version 4.0 or clang 5.0.

I'm OK to enable the -Wvarargs again if we think these lib ABI updates are acceptable.

Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Monday, September 25, 2017 8:30 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in
> CLANG38 toolchain
> 
> Hey Liming and Laszlo,
> 
> Thanks for your answers!
> 
> I rather thought of this (I didn't check whether Language1 should be CONST,
> but doesn't matter for now):
> 
> CHAR8 *
> EFIAPI
> GetBestLanguage (
>   IN CONST CHAR8  *SupportedLanguages,
>   IN BOOLEAN          Iso639Language,
>   IN CHAR8               *Language1,
>   ...
>   );
> 
> This would be compatible with all existing code because the VA list solely
> consists of CHAR8 pointers.
> The only difference would be that one cannot pass just NULL after
> Iso639Language, but this scenario makes no sense anyway.
> One would just use Langauge1 on the first iteration and, as part of a do-
> while-loop, assign the VA args via VA_ARG().
> 
> Best regards,
> Marvin.
> 
> > -----Original Message-----
> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > Sent: Monday, September 25, 2017 11:58 AM
> > To: Laszlo Ersek <lersek@redhat.com>; Marvin H?user
> > <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40
> in
> > CLANG38 toolchain
> >
> > Laszlo:
> >   This is a better way. The bug
> > https://bugzilla.tianocore.org/show_bug.cgi?id=410 only lists
> > GetBestLanguage() API. I am not sure whether they are similar cases in
> edk2.
> > If have, we had better fix them together.
> >
> > Thanks
> > Liming
> > >-----Original Message-----
> > >From: Laszlo Ersek [mailto:lersek@redhat.com]
> > >Sent: Monday, September 25, 2017 4:13 PM
> > >To: Gao, Liming <liming.gao@intel.com>; Marvin Häuser
> > ><Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org
> > >Subject: Re: [edk2] [PATCH] BaseTools/Conf: Support LLVM39 and
> LLVM40
> > >in
> > >CLANG38 toolchain
> > >
> > >On 09/25/17 06:59, Gao, Liming wrote:
> > >> Marvin:
> > >>   My concern is that the fix is an incompatible change. It requires
> > >> to modify
> > >library API. In fact, this is an undefined behavior. But, current
> > >compiler (VS, GCC and Clang) makes it work.  So, I prefer to keep the
> > >code as-is, and disable this warning first. If you find any real issue,
> > >we can return back and figure out the solution.
> > >
> > >I think we can draw a parallel here to GetVariable() and GetVariable2().
> > >GetVariable() is now unavailable if
> > DISABLE_NEW_DEPRECATED_INTERFACES
> > >is defined.
> > >
> > >I think the right solution would be to
> > >- introduce GetBestLanguage2(),
> > >- migrate all the current call sites to GetBestLanguage2() -- I counted
> > >  18, and the updates should be easy --,
> > >- and then make GetVariable() conditional on
> > >  not-DISABLE_NEW_DEPRECATED_INTERFACES.
> > >
> > >
> > >CHAR8 *
> > >EFIAPI
> > >GetBestLanguage2 (
> > >  IN CONST CHAR8  *SupportedLanguages,
> > >  IN INTN         Iso639Language,
> > >  ...
> > >  );
> > >
> > >I don't feel strongly about this question, I just think technically
> > >this would be best. CLANG warnings are valuable.
> > >
> > >Thanks
> > >Laszlo
> > >
> > >>> -----Original Message-----
> > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > >>> Of Marvin H?user
> > >>> Sent: Friday, September 22, 2017 11:53 PM
> > >>> To: edk2-devel@lists.01.org
> > >>> Cc: Gao, Liming <liming.gao@intel.com>
> > >>> Subject: Re: [edk2] [PATCH] BaseTools/Conf: Support LLVM39 and
> > >>> LLVM40
> > >in
> > >>> CLANG38 toolchain
> > >>>
> > >>> Hey,
> > >>>
> > >>> I just noticed this patch as it recently has been pushed. I found
> > >>> this has
> > >been a
> > >>> reaction to https://bugzilla.tianocore.org/show_bug.cgi?id=410
> > >>> Though as Clang correctly detected, this is Undefined Behavior per
> > >>> the C specification, so why was the warning hidden?
> > >>> In context of the issue in UefiLib, providing the first element of
> > >>> the VA list
> > >as a
> > >>> prototyped argument, would have solved the issue without UB.
> > >>>
> > >>> Do you wish such a patch?
> > >>>
> > >>> Thanks,
> > >>> Marvin.
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf
> > >>>> Of Gao, Liming
> > >>>> Sent: Monday, August 28, 2017 9:19 AM
> > >>>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel@lists.01.org
> > >>>> Subject: Re: [edk2] [PATCH] BaseTools/Conf: Support LLVM39 and
> > >LLVM40
> > >>> in
> > >>>> CLANG38 toolchain
> > >>>>
> > >>>> Reviewed-by: Liming Gao <liming.gao@intel.com>
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: Shi, Steven
> > >>>>> Sent: Wednesday, August 23, 2017 3:01 PM
> > >>>>> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
> > >>>>> Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Shi, Steven
> > >>>>> <steven.shi@intel.com>
> > >>>>> Subject: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in
> > >>> CLANG38
> > >>>>> toolchain
> > >>>>>
> > >>>>> From: "Shi, Steven" <steven.shi@intel.com>
> > >>>>>
> > >>>>> Add LLVM39 and LLVM40 support in CLANG38 toolchain
> > >>>>>
> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.0
> > >>>>> Signed-off-by: Steven Shi <steven.shi@intel.com>
> > >>>>> ---
> > >>>>> BaseTools/Conf/tools_def.template | 5 +++--
> > >>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/BaseTools/Conf/tools_def.template
> > >>>>> b/BaseTools/Conf/tools_def.template
> > >>>>> index 1fa3ca3..2f83341 100755
> > >>>>> --- a/BaseTools/Conf/tools_def.template
> > >>>>> +++ b/BaseTools/Conf/tools_def.template
> > >>>>> @@ -380,7 +380,8 @@ DEFINE SOURCERY_CYGWIN_TOOLS =
> > >>>> /cygdrive/c/Program
> > >>>>> Files/CodeSourcery/Sourcery G
> > >>>>> #                               Intel(r) ACPI Compiler from
> > >>>>> #                               https://acpica.org/downloads
> > >>>>> #   CLANG38  -Linux-  Requires:
> > >>>>> -#                             Clang v3.8 or later, LLVMgold plugin and GNU
> binutils
> > >2.26
> > >>>>> targeting x86_64-linux-gnu
> > >>>>> +#                             Clang v3.8, LLVMgold plugin and GNU binutils
> 2.26
> > >>>> targeting
> > >>>>> x86_64-linux-gnu
> > >>>>> +#                             Clang v3.9 or later, LLVMgold plugin and GNU
> binutils
> > >2.28
> > >>>>> targeting x86_64-linux-gnu
> > >>>>> #                        Optional:
> > >>>>> #                             Required to build platforms or ACPI tables:
> > >>>>> #                               Intel(r) ACPI Compiler from
> > >>>>> @@ -5512,7 +5513,7 @@ DEFINE CLANG38_X64_PREFIX           =
> > >>>>> ENV(CLANG38_BIN)
> > >>>>> DEFINE CLANG38_IA32_TARGET          = -target i686-pc-linux-gnu
> > >>>>> DEFINE CLANG38_X64_TARGET           = -target x86_64-pc-linux-gnu
> > >>>>>
> > >>>>> -DEFINE CLANG38_ALL_CC_FLAGS         = DEF(GCC44_ALL_CC_FLAGS)
> > -
> > >>> Wno-
> > >>>>> empty-body -fno-stack-protector -mms-bitfields -Wno-address -
> > Wno-
> > >>> shift-
> > >>>>> negative-value -Wno-parentheses-equality -Wno-unknown-pragmas
> > -
> > >>>> Wno-
> > >>>>> tautological-constant-out-of-range-compare
> > >>>>> -Wno-incompatible-library- redeclaration
> > >>>>> -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -
> > >>>> msoft-
> > >>>>> float -mno-implicit-float  -ftrap-
> > >>>>> function=undefined_behavior_has_been_optimized_away_by_clang
> > -
> > >>>>> funsigned-char -fno-ms-extensions -Wno-null-dereference -Wno-
> > >>>>> tautological-compare -Wno-unknown-warning-option
> > >>>>> +DEFINE CLANG38_ALL_CC_FLAGS         =
> DEF(GCC44_ALL_CC_FLAGS)
> > -
> > >>>> Wno-
> > >>>>> empty-body -fno-stack-protector -mms-bitfields -Wno-address -
> > Wno-
> > >>> shift-
> > >>>>> negative-value -Wno-parentheses-equality -Wno-unknown-pragmas
> > -
> > >>>> Wno-
> > >>>>> tautological-constant-out-of-range-compare
> > >>>>> -Wno-incompatible-library- redeclaration
> > >>>>> -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -
> > >>>> msoft-
> > >>>>> float -mno-implicit-float  -ftrap-
> > >>>>> function=undefined_behavior_has_been_optimized_away_by_clang
> > -
> > >>>>> funsigned-char -fno-ms-extensions -Wno-null-dereference -Wno-
> > >>>>> tautological-compare -Wno-unknown-warning-option -Wno-varargs
> > >>>>>
> > >>>>> ###########################
> > >>>>> # CLANG38 IA32 definitions
> > >>>>> --
> > >>>>> 2.7.4
> > >>>>
> > >>>> _______________________________________________
> > >>>> edk2-devel mailing list
> > >>>> edk2-devel@lists.01.org
> > >>>> https://lists.01.org/mailman/listinfo/edk2-devel
> > >>> _______________________________________________
> > >>> edk2-devel mailing list
> > >>> edk2-devel@lists.01.org
> > >>> https://lists.01.org/mailman/listinfo/edk2-devel
> > >> _______________________________________________
> > >> edk2-devel mailing list
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
> > >>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel