1. Patchew
  2. EDK2
  3. View series

[edk2] [PATCH] MdePkg: resolve bug 741

Zenith432 posted 1 patch 7 years ago
Failed in applying to current master (apply log)
MdePkg/Include/Library/UefiLib.h | 2 +-
MdePkg/Library/UefiLib/UefiLib.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[edk2] [PATCH] MdePkg: resolve bug 741
Posted by Zenith432 7 years ago 
This is a proposal to resolve bug 741: UefiLib.c compilation failure with clang
error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs]"

Rationale:
1. Default argument promotion causes the sizeof a function's argument to be different than the corresponding
parameter's sizeof.  This may break a permissible implemenation of stdarg.h, which is why it is considered
undefined behavior in C.  The condition should be repaired rather than silenced with -Wno-varargs.
2. The warning is due to the last non-variadic parameter of GetBestLanguage having type BOOLEAN.
3. BOOLEAN is typedef'd to 'unsigned char' in all ProcessorBind.h.
4. 'unsigned char' goes default argument promotion to int.
5. All ProcessorBind.h typedef the type INT32 to either 'int' or some 32-bit equivalent.
6. As a result it is safe to replace the type of the parameter from BOOLEAN to INT32 in all
current supported architectures.
7. The change is consistent with the BOOLEAN argument being converted to INT32 at the caller site. The
function GetBestLanguage currently converts the argument from INT32 back to BOOLEAN, however
the function's logic is not disturbed by treating the argument as an INT32.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zenith432 <zenith432@users.sourceforge.net>
---
 MdePkg/Include/Library/UefiLib.h | 2 +-
 MdePkg/Library/UefiLib/UefiLib.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 0b14792a..5d98eb26 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -818,7 +818,7 @@ CHAR8 *
 EFIAPI
 GetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages, 
-  IN BOOLEAN      Iso639Language,
+  IN INT32        Iso639Language,
   ...
   );
 
diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
index a7eee012..57236511 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -1514,7 +1514,7 @@ CHAR8 *
 EFIAPI
 GetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages, 
-  IN BOOLEAN      Iso639Language,
+  IN INT32        Iso639Language,
   ...
   )
 {
-- 
2.14.3 (Apple Git-98)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdePkg: resolve bug 741
Posted by Marvin H?user 7 years ago 
For code style purposes, I suggest to declare the first member of the language list as a third argument, process it, and then use it to get the following from the VA.
Also, I think you forgot to CC the MdePkg maintainers, so I did with this mail.

Regards,
Marvin.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Zenith432
> Sent: Saturday, December 9, 2017 8:44 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH] MdePkg: resolve bug 741
> 
> This is a proposal to resolve bug 741: UefiLib.c compilation failure with clang
> error: passing an object that undergoes default argument promotion to
> 'va_start' has undefined behavior [-Werror,-Wvarargs]"
> 
> Rationale:
> 1. Default argument promotion causes the sizeof a function's argument to be
> different than the corresponding parameter's sizeof.  This may break a
> permissible implemenation of stdarg.h, which is why it is considered
> undefined behavior in C.  The condition should be repaired rather than
> silenced with -Wno-varargs.
> 2. The warning is due to the last non-variadic parameter of GetBestLanguage
> having type BOOLEAN.
> 3. BOOLEAN is typedef'd to 'unsigned char' in all ProcessorBind.h.
> 4. 'unsigned char' goes default argument promotion to int.
> 5. All ProcessorBind.h typedef the type INT32 to either 'int' or some 32-bit
> equivalent.
> 6. As a result it is safe to replace the type of the parameter from BOOLEAN to
> INT32 in all current supported architectures.
> 7. The change is consistent with the BOOLEAN argument being converted to
> INT32 at the caller site. The function GetBestLanguage currently converts the
> argument from INT32 back to BOOLEAN, however the function's logic is not
> disturbed by treating the argument as an INT32.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Zenith432 <zenith432@users.sourceforge.net>
> ---
>  MdePkg/Include/Library/UefiLib.h | 2 +-  MdePkg/Library/UefiLib/UefiLib.c
> | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h
> b/MdePkg/Include/Library/UefiLib.h
> index 0b14792a..5d98eb26 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -818,7 +818,7 @@ CHAR8 *
>  EFIAPI
>  GetBestLanguage (
>    IN CONST CHAR8  *SupportedLanguages,
> -  IN BOOLEAN      Iso639Language,
> +  IN INT32        Iso639Language,
>    ...
>    );
> 
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
> b/MdePkg/Library/UefiLib/UefiLib.c
> index a7eee012..57236511 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1514,7 +1514,7 @@ CHAR8 *
>  EFIAPI
>  GetBestLanguage (
>    IN CONST CHAR8  *SupportedLanguages,
> -  IN BOOLEAN      Iso639Language,
> +  IN INT32        Iso639Language,
>    ...
>    )
>  {
> --
> 2.14.3 (Apple Git-98)
> _______________________________________________
> 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
Re: [edk2] [PATCH] MdePkg: resolve bug 741
Posted by Gao, Liming 7 years ago 
For 4, 'unsigned char' goes default argument promotion to int. This is CLANG compiler behavior. Does GCC and VS compiler follow this rule?

Disable varargs warning is the temp solution. For long term, we expect to figure out the compatible solution. If all supported compilers follow this rule, I think this is a compatible change. 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Zenith432
> Sent: Sunday, December 10, 2017 3:44 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH] MdePkg: resolve bug 741
> 
> This is a proposal to resolve bug 741: UefiLib.c compilation failure with clang
> error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs]"
> 
> Rationale:
> 1. Default argument promotion causes the sizeof a function's argument to be different than the corresponding
> parameter's sizeof.  This may break a permissible implemenation of stdarg.h, which is why it is considered
> undefined behavior in C.  The condition should be repaired rather than silenced with -Wno-varargs.
> 2. The warning is due to the last non-variadic parameter of GetBestLanguage having type BOOLEAN.
> 3. BOOLEAN is typedef'd to 'unsigned char' in all ProcessorBind.h.
> 4. 'unsigned char' goes default argument promotion to int.
> 5. All ProcessorBind.h typedef the type INT32 to either 'int' or some 32-bit equivalent.
> 6. As a result it is safe to replace the type of the parameter from BOOLEAN to INT32 in all
> current supported architectures.
> 7. The change is consistent with the BOOLEAN argument being converted to INT32 at the caller site. The
> function GetBestLanguage currently converts the argument from INT32 back to BOOLEAN, however
> the function's logic is not disturbed by treating the argument as an INT32.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Zenith432 <zenith432@users.sourceforge.net>
> ---
>  MdePkg/Include/Library/UefiLib.h | 2 +-
>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
> index 0b14792a..5d98eb26 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -818,7 +818,7 @@ CHAR8 *
>  EFIAPI
>  GetBestLanguage (
>    IN CONST CHAR8  *SupportedLanguages,
> -  IN BOOLEAN      Iso639Language,
> +  IN INT32        Iso639Language,
>    ...
>    );
> 
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
> index a7eee012..57236511 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1514,7 +1514,7 @@ CHAR8 *
>  EFIAPI
>  GetBestLanguage (
>    IN CONST CHAR8  *SupportedLanguages,
> -  IN BOOLEAN      Iso639Language,
> +  IN INT32        Iso639Language,
>    ...
>    )
>  {
> --
> 2.14.3 (Apple Git-98)
> _______________________________________________
> 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
Re: [edk2] [PATCH] MdePkg: resolve bug 741
Posted by Zenith432 7 years ago 
On 10/12/2017 15:52, Gao, Liming wrote:
 > For 4, 'unsigned char' goes default argument promotion to int. This 
is CLANG compiler behavior. Does GCC and VS compiler follow this rule?
 >
 > Disable varargs warning is the temp solution. For long term, we 
expect to figure out the compatible solution. If all supported compilers 
follow this rule, I think this is a compatible change.

It is STD C default argument promotion.  For example, C99 document n1570

section 6.5.2.2 (Function Calls)
"If the expression that denotes the called function has a type that does 
not include a prototype, the integer promotions are performed on each 
argument, and arguments that have type float are promoted to double. 
These are called the default argument promotions."

section 6.3.1.1 (Booleans, characters and integers)
"If an int can represent all values of the original type (as restricted 
by the width, for a bit-field), the value is converted to an int; 
otherwise, it is converted to an unsigned int. These are called the 
integer promotions."

section 7.16.1.4 (The va_start macro)
"If the parameter parmN is declared with the register storage class, 
with a function or array type, or with a type that is not compatible 
with the type that results after application of the default argument 
promotions, the behavior is undefined."

Technically, when there's a prototype that says the parameter is BOOLEAN 
(i.e. unsigned char), there is no "promotion" from the point of view of 
STDC.  Instead, it tries to convert the argument to BOOLEAN (if 
possible.)  Then the BOOLEAN is passed using whatever mechanism the 
implementation has to pass BOOLEANs (which is to fit them in a 4 or 
8-byte granular stack).  However, va_start wants a parameter type that 
is the outcome of a default argument promotion, and the outcome of a 
default argument promotion of 'unsigned char' is always int.

There's more discussion with longer quotes from the STD here
https://stackoverflow.com/questions/1255775/default-argument-promotions-in-c-function-calls
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.