* [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
@ 2017-08-23 7:01 Shi Steven
2017-08-28 7:19 ` Gao, Liming
0 siblings, 1 reply; 8+ messages in thread
From: Shi Steven @ 2017-08-23 7:01 UTC (permalink / raw)
To: edk2-devel, liming.gao
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
2017-08-23 7:01 [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain Shi Steven
@ 2017-08-28 7:19 ` Gao, Liming
2017-09-22 15:53 ` Marvin H?user
0 siblings, 1 reply; 8+ messages in thread
From: Gao, Liming @ 2017-08-28 7:19 UTC (permalink / raw)
To: Shi, Steven, edk2-devel@lists.01.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
2017-08-28 7:19 ` Gao, Liming
@ 2017-09-22 15:53 ` Marvin H?user
2017-09-25 4:59 ` Gao, Liming
0 siblings, 1 reply; 8+ messages in thread
From: Marvin H?user @ 2017-09-22 15:53 UTC (permalink / raw)
To: edk2-devel@lists.01.org
Cc: Gao, Liming, steven.shi@intel.com, yonghong.zhu@intel.com
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
2017-09-22 15:53 ` Marvin H?user
@ 2017-09-25 4:59 ` Gao, Liming
2017-09-25 8:13 ` Laszlo Ersek
0 siblings, 1 reply; 8+ messages in thread
From: Gao, Liming @ 2017-09-25 4:59 UTC (permalink / raw)
To: Marvin H?user, edk2-devel@lists.01.org
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.
Thanks
Liming
>-----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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
2017-09-25 4:59 ` Gao, Liming
@ 2017-09-25 8:13 ` Laszlo Ersek
2017-09-25 9:57 ` Gao, Liming
0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2017-09-25 8:13 UTC (permalink / raw)
To: Gao, Liming, Marvin Häuser, edk2-devel@lists.01.org
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
2017-09-25 8:13 ` Laszlo Ersek
@ 2017-09-25 9:57 ` Gao, Liming
2017-09-25 12:29 ` Marvin Häuser
0 siblings, 1 reply; 8+ messages in thread
From: Gao, Liming @ 2017-09-25 9:57 UTC (permalink / raw)
To: Laszlo Ersek, Marvin H?user, edk2-devel@lists.01.org
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
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
2017-09-25 9:57 ` Gao, Liming
@ 2017-09-25 12:29 ` Marvin Häuser
2017-09-26 3:14 ` Shi, Steven
0 siblings, 1 reply; 8+ messages in thread
From: Marvin Häuser @ 2017-09-25 12:29 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Gao, Liming, Laszlo Ersek
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
> >>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain
2017-09-25 12:29 ` Marvin Häuser
@ 2017-09-26 3:14 ` Shi, Steven
0 siblings, 0 replies; 8+ messages in thread
From: Shi, Steven @ 2017-09-26 3:14 UTC (permalink / raw)
To: Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org
Cc: Laszlo Ersek, Gao, Liming
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-26 3:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-23 7:01 [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain Shi Steven
2017-08-28 7:19 ` Gao, Liming
2017-09-22 15:53 ` Marvin H?user
2017-09-25 4:59 ` Gao, Liming
2017-09-25 8:13 ` Laszlo Ersek
2017-09-25 9:57 ` Gao, Liming
2017-09-25 12:29 ` Marvin Häuser
2017-09-26 3:14 ` Shi, Steven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox