* [PATCH] MdePkg: resolve bug 741 [not found] <541697813.1899563.1512848632833.ref@mail.yahoo.com> @ 2017-12-09 19:43 ` Zenith432 2017-12-09 20:22 ` Marvin H?user 2017-12-10 13:52 ` Gao, Liming 0 siblings, 2 replies; 8+ messages in thread From: Zenith432 @ 2017-12-09 19:43 UTC (permalink / raw) To: edk2-devel 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) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] MdePkg: resolve bug 741 2017-12-09 19:43 ` [PATCH] MdePkg: resolve bug 741 Zenith432 @ 2017-12-09 20:22 ` Marvin H?user 2017-12-10 13:52 ` Gao, Liming 1 sibling, 0 replies; 8+ messages in thread From: Marvin H?user @ 2017-12-09 20:22 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Zenith432, michael.d.kinney@intel.com, liming.gao@intel.com 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdePkg: resolve bug 741 2017-12-09 19:43 ` [PATCH] MdePkg: resolve bug 741 Zenith432 2017-12-09 20:22 ` Marvin H?user @ 2017-12-10 13:52 ` Gao, Liming 2017-12-10 14:41 ` Zenith432 1 sibling, 1 reply; 8+ messages in thread From: Gao, Liming @ 2017-12-10 13:52 UTC (permalink / raw) To: Zenith432, edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdePkg: resolve bug 741 2017-12-10 13:52 ` Gao, Liming @ 2017-12-10 14:41 ` Zenith432 0 siblings, 0 replies; 8+ messages in thread From: Zenith432 @ 2017-12-10 14:41 UTC (permalink / raw) To: Gao, Liming, edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <1822489314.1922023.1512852000221.ref@mail.yahoo.com>]
* Re: [PATCH] MdePkg: resolve bug 741 [not found] <1822489314.1922023.1512852000221.ref@mail.yahoo.com> @ 2017-12-09 20:40 ` Zenith432 2017-12-09 20:57 ` Marvin Häuser 0 siblings, 1 reply; 8+ messages in thread From: Zenith432 @ 2017-12-09 20:40 UTC (permalink / raw) To: edk2-devel@lists.01.org, Marvin H?user Cc: michael.d.kinney@intel.com, liming.gao@intel.com FWIW, I tried another solution by replacing the statement VA_START (Args, Iso639Language); with VA_START (Args, *(int*)&Iso639Language); in an attempt to get an lvalue with the same address-of as Iso639Language and correct sizeof the promoted argument, but clang doesn't like that either error: second argument to 'va_start' is not the last named parameter [-Werror,-Wvarargs] -------------------------------------------- On Sat, 12/9/17, Marvin H?user <Marvin.Haeuser@outlook.com> wrote: Subject: RE: [edk2] [PATCH] MdePkg: resolve bug 741 To: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org> Cc: "Zenith432" <zenith432@users.sourceforge.net>, "michael.d.kinney@intel.com" <michael.d.kinney@intel.com>, "liming.gao@intel.com" <liming.gao@intel.com> Date: Saturday, December 9, 2017, 10:22 PM 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdePkg: resolve bug 741 2017-12-09 20:40 ` Zenith432 @ 2017-12-09 20:57 ` Marvin Häuser 0 siblings, 0 replies; 8+ messages in thread From: Marvin Häuser @ 2017-12-09 20:57 UTC (permalink / raw) To: edk2-devel@lists.01.org, Zenith432 Cc: michael.d.kinney@intel.com, liming.gao@intel.com I prefer my route because it preserves BOOLEAN (descriptive) and fixes the issue obviously (the last named parameter is what is passed). It's your choice of course. By the way, all of your other patch e-mails do not CC the package maintainer. They may not see them this way, so I suggest resending CC'ing the package maintainers. Regards, Marvin. > -----Original Message----- > From: Zenith432 [mailto:zenith432@users.sourceforge.net] > Sent: Saturday, December 9, 2017 9:40 PM > To: edk2-devel@lists.01.org; Marvin H?user > <Marvin.Haeuser@outlook.com> > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > Subject: RE: [edk2] [PATCH] MdePkg: resolve bug 741 > > FWIW, I tried another solution by replacing the statement > > VA_START (Args, Iso639Language); > > with > > VA_START (Args, *(int*)&Iso639Language); > > in an attempt to get an lvalue with the same address-of as Iso639Language > and correct sizeof the promoted argument, but clang doesn't like that either > > error: second argument to 'va_start' is not the last named parameter [- > Werror,-Wvarargs] > > -------------------------------------------- > On Sat, 12/9/17, Marvin H?user <Marvin.Haeuser@outlook.com> wrote: > > Subject: RE: [edk2] [PATCH] MdePkg: resolve bug 741 > To: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org> > Cc: "Zenith432" <zenith432@users.sourceforge.net>, > "michael.d.kinney@intel.com" <michael.d.kinney@intel.com>, > "liming.gao@intel.com" <liming.gao@intel.com> > Date: Saturday, December 9, 2017, 10:22 PM > > 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. > ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <617598899.1906935.1512855198518.ref@mail.yahoo.com>]
* Re: [PATCH] MdePkg: resolve bug 741 [not found] <617598899.1906935.1512855198518.ref@mail.yahoo.com> @ 2017-12-09 21:33 ` Zenith432 2018-01-03 2:06 ` Gao, Liming 0 siblings, 1 reply; 8+ messages in thread From: Zenith432 @ 2017-12-09 21:33 UTC (permalink / raw) To: edk2-devel@lists.01.org, Marvin Häuser Cc: michael.d.kinney@intel.com, liming.gao@intel.com It's the package maintainer's choice. As a practical matter, silencing the warning also works because... 1. clang is the only compiler that complains. Even though it complains, it generates correct code because it has __builtin implementation of va_start that takes register argument and stack granularity into account. 2. In MdePkg/Include/Base.h there are __builtin implementations of VA_START for __CC_ARM, GCC and clang which should all work despite the argument promotion. 3. For the other architectures (i.e. Windows) there's an implementation of VA_START in Base.h that uses _INT_SIZE_OF for the parameter always a multiple of UINTN. The Windows compilers also have builtin forms of va_start, but this non-builtin implementation looks ok for all arguments actually passed as variadic in EDK2. -------------------------------------------- On Sat, 12/9/17, Marvin Häuser <Marvin.Haeuser@outlook.com> wrote: ... It's your choice of course. ... Regards, Marvin. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdePkg: resolve bug 741 2017-12-09 21:33 ` Zenith432 @ 2018-01-03 2:06 ` Gao, Liming 0 siblings, 0 replies; 8+ messages in thread From: Gao, Liming @ 2018-01-03 2:06 UTC (permalink / raw) To: Zenith432, edk2-devel@lists.01.org, Marvin H?user; +Cc: Kinney, Michael D I choose to disable this warning first, because there are other cases in edk2 project. One is PI S3SaveState protocol EFI_S3_SAVE_STATE_WRITE API. This case needs to update PI spec. It may take more time. typedef EFI_STATUS (EFIAPI *EFI_S3_SAVE_STATE_WRITE)( IN CONST EFI_S3_SAVE_STATE_PROTOCOL *This, IN UINT16 OpCode, ... ); Thanks Liming > -----Original Message----- > From: Zenith432 [mailto:zenith432@users.sourceforge.net] > Sent: Sunday, December 10, 2017 5:33 AM > To: edk2-devel@lists.01.org; Marvin Häuser <Marvin.Haeuser@outlook.com> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com> > Subject: RE: [edk2] [PATCH] MdePkg: resolve bug 741 > > It's the package maintainer's choice. As a practical matter, silencing the warning also works because... > > 1. clang is the only compiler that complains. Even though it complains, it generates correct code because it has __builtin > implementation of va_start that takes register argument and stack granularity into account. > 2. In MdePkg/Include/Base.h there are __builtin implementations of VA_START for __CC_ARM, GCC and clang which should all work > despite the argument promotion. > 3. For the other architectures (i.e. Windows) there's an implementation of VA_START in Base.h that uses _INT_SIZE_OF for the > parameter always a multiple of UINTN. The Windows compilers also have builtin forms of va_start, but this non-builtin > implementation looks ok for all arguments actually passed as variadic in EDK2. > > -------------------------------------------- > On Sat, 12/9/17, Marvin Häuser <Marvin.Haeuser@outlook.com> wrote: > > ... > It's your choice of course. > ... > > Regards, > Marvin. > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-01-03 2:01 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <541697813.1899563.1512848632833.ref@mail.yahoo.com> 2017-12-09 19:43 ` [PATCH] MdePkg: resolve bug 741 Zenith432 2017-12-09 20:22 ` Marvin H?user 2017-12-10 13:52 ` Gao, Liming 2017-12-10 14:41 ` Zenith432 [not found] <1822489314.1922023.1512852000221.ref@mail.yahoo.com> 2017-12-09 20:40 ` Zenith432 2017-12-09 20:57 ` Marvin Häuser [not found] <617598899.1906935.1512855198518.ref@mail.yahoo.com> 2017-12-09 21:33 ` Zenith432 2018-01-03 2:06 ` Gao, Liming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox