diff mbox

[edk2,4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

Message ID 1467120338-12587-5-git-send-email-lersek@redhat.com
State Accepted
Commit 4a7518d31a0254e4065d308f091bd7bc16dc8dba
Headers show

Commit Message

Laszlo Ersek June 28, 2016, 1:25 p.m. UTC 
When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
the status checking should be removed; the function calls should stay.

Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Shumin Qiu <shumin.qiu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>

---

Notes:
    build tested

 ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 ++++++++--
 ShellPkg/Library/UefiShellLib/UefiShellLib.c     |  5 ++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

-- 
1.8.3.1


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek June 29, 2016, 11:49 a.m. UTC | #1 
On 06/29/16 08:36, Ni, Ruiyu wrote:
> Laszlo,

> Thanks for fixing such a big bug.

> 

> I am curious how you detect such buggy code? By code review?


Yes.

I described this in the 0/6 message
<http://thread.gmane.org/gmane.comp.bios.edk2.devel/13736>, but I'm
happy to elaborate. :)

So, Gerd has a Jenkins Continuous Integration environment that regularly
fetches new edk2 commits and build OvmfPkg and ArmVirtPkg platforms.
Gerd has recently updated his Jenkins instance to gcc-6, and gcc-6 flags
some C language constructs -- with new warnings -- that used to "stay
under the radar" with earlier gcc releases.

So, one of the erroneous constructs that gcc-6 finds is:

  ASSERT_EFI_ERROR (BooleanExpression)

Clearly, in this case the programmer meant

  ASSERT (BooleanExpression)

and ASSERT_EFI_ERROR() is just a typo.

gcc-6 finds this issue because:
- ASSERT_EFI_ERROR() expands to EFI_ERROR(),
- EFI_ERROR() expands to RETURN_ERROR(),
- and RETURN_ERROR() checks if the argument, first converted to
  RETURN_STATUS (== UINTN), then converted to INTN, is negative,
- when a boolean expression is converted to UINTN, then INTN, the
  result is always 0 or 1 -- it can never be negative.

Therefore the incorrect form

  ASSERT_EFI_ERROR (BooleanExpression)

never fires, and gcc-6 finds it with the "-Wbool-compare".

Now, Gerd's build stopped with the first such error, and because I
hadn't set up gcc-6 yet, I decided to audit all ASSERT_EFI_ERROR()
applications in edk2. I used the following command as first step:

  git grep -Enw ASSERT_EFI_ERROR \
  | grep -E -v 'ASSERT_EFI_ERROR( )?\(Status\)'

because I wanted to see all invocations of this macro, *except* the
following two form:

  ASSERT_EFI_ERROR(Status)
  ASSERT_EFI_ERROR (Status)

Those two are by far the most frequent ones, and I trusted that a
variable called "Status" would always have type RETURN_STATUS or EFI_STATUS.

The rest of the macro invocations I audited one by one. Most of the
errors that I found were of the kind

  ASSERT_EFI_ERROR (BooleanExpression)

which I simply fixed by removing the "_EFI_ERROR" part.

The code fixed by this patch had a different kind of error, but the way
I found those locations was the same: just looking at ASSERT_EFI_ERROR
macro invocations.

Thanks!
Laszlo

>> -----Original Message-----

>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek

>> Sent: Tuesday, June 28, 2016 9:26 PM

>> To: edk2-devel-01 <edk2-devel@ml01.01.org>

>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Qiu, Shumin <shumin.qiu@intel.com>

>> Subject: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

>>

>> When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only

>> the status checking should be removed; the function calls should stay.

>>

>> Cc: Jaben Carsey <jaben.carsey@intel.com>

>> Cc: Shumin Qiu <shumin.qiu@intel.com>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

>> ---

>>

>> Notes:

>>    build tested

>>

>> ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 ++++++++--

>> ShellPkg/Library/UefiShellLib/UefiShellLib.c     |  5 ++++-

>> 2 files changed, 12 insertions(+), 3 deletions(-)

>>

>> diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c

>> index 7abfd8944b92..dc96bffde7d3 100644

>> --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c

>> +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c

>> @@ -991,8 +991,11 @@ ShellCommandRunElse (

>>   IN EFI_SYSTEM_TABLE  *SystemTable

>>   )

>> {

>> +  EFI_STATUS  Status;

>>   SCRIPT_FILE *CurrentScriptFile;

>> -  ASSERT_EFI_ERROR(CommandInit());

>> +

>> +  Status = CommandInit ();

>> +  ASSERT_EFI_ERROR (Status);

>>

>>   if (gEfiShellParametersProtocol->Argc > 1) {

>>     ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), gShellLevel1HiiHandle, L"if");

>> @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (

>>   IN EFI_SYSTEM_TABLE  *SystemTable

>>   )

>> {

>> +  EFI_STATUS  Status;

>>   SCRIPT_FILE *CurrentScriptFile;

>> -  ASSERT_EFI_ERROR(CommandInit());

>> +

>> +  Status = CommandInit ();

>> +  ASSERT_EFI_ERROR (Status);

>>

>>   if (gEfiShellParametersProtocol->Argc > 1) {

>>     ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), gShellLevel1HiiHandle, L"if");

>> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c

>> index cf89a4ac87ed..35a1a7169c8b 100644

>> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c

>> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c

>> @@ -373,6 +373,8 @@ EFIAPI

>> ShellInitialize (

>>   )

>> {

>> +  EFI_STATUS Status;

>> +

>>   //

>>   // if auto initialize is not false then skip

>>   //

>> @@ -383,7 +385,8 @@ ShellInitialize (

>>   //

>>   // deinit the current stuff

>>   //

>> -  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));

>> +  Status = ShellLibDestructor (gImageHandle, gST);

>> +  ASSERT_EFI_ERROR (Status);

>>

>>   //

>>   // init the new stuff

>> --

>> 1.8.3.1

>>

>>

>> _______________________________________________

>> 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
Laszlo Ersek June 30, 2016, 11:07 a.m. UTC | #2 
Jaben, Shumin,

On 06/28/16 15:25, Laszlo Ersek wrote:
> When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only

> the status checking should be removed; the function calls should stay.

> 

> Cc: Jaben Carsey <jaben.carsey@intel.com>

> Cc: Shumin Qiu <shumin.qiu@intel.com>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

> ---

> 

> Notes:

>     build tested

> 

>  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 ++++++++--

>  ShellPkg/Library/UefiShellLib/UefiShellLib.c     |  5 ++++-

>  2 files changed, 12 insertions(+), 3 deletions(-)


Can I please get a maintainer review for this patch?

Thanks
Laszlo

> diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c

> index 7abfd8944b92..dc96bffde7d3 100644

> --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c

> +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c

> @@ -991,8 +991,11 @@ ShellCommandRunElse (

>    IN EFI_SYSTEM_TABLE  *SystemTable

>    )

>  {

> +  EFI_STATUS  Status;

>    SCRIPT_FILE *CurrentScriptFile;

> -  ASSERT_EFI_ERROR(CommandInit());

> +

> +  Status = CommandInit ();

> +  ASSERT_EFI_ERROR (Status);

>  

>    if (gEfiShellParametersProtocol->Argc > 1) {

>      ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), gShellLevel1HiiHandle, L"if");  

> @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (

>    IN EFI_SYSTEM_TABLE  *SystemTable

>    )

>  {

> +  EFI_STATUS  Status;

>    SCRIPT_FILE *CurrentScriptFile;

> -  ASSERT_EFI_ERROR(CommandInit());

> +

> +  Status = CommandInit ();

> +  ASSERT_EFI_ERROR (Status);

>  

>    if (gEfiShellParametersProtocol->Argc > 1) {

>      ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), gShellLevel1HiiHandle, L"if");  

> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c

> index cf89a4ac87ed..35a1a7169c8b 100644

> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c

> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c

> @@ -373,6 +373,8 @@ EFIAPI

>  ShellInitialize (

>    )

>  {

> +  EFI_STATUS Status;

> +

>    //

>    // if auto initialize is not false then skip

>    //

> @@ -383,7 +385,8 @@ ShellInitialize (

>    //

>    // deinit the current stuff

>    //

> -  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));

> +  Status = ShellLibDestructor (gImageHandle, gST);

> +  ASSERT_EFI_ERROR (Status);

>  

>    //

>    // init the new stuff

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
index 7abfd8944b92..dc96bffde7d3 100644
--- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
+++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
@@ -991,8 +991,11 @@  ShellCommandRunElse (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  EFI_STATUS  Status;
   SCRIPT_FILE *CurrentScriptFile;
-  ASSERT_EFI_ERROR(CommandInit());
+
+  Status = CommandInit ();
+  ASSERT_EFI_ERROR (Status);
 
   if (gEfiShellParametersProtocol->Argc > 1) {
     ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), gShellLevel1HiiHandle, L"if");  
@@ -1066,8 +1069,11 @@  ShellCommandRunEndIf (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  EFI_STATUS  Status;
   SCRIPT_FILE *CurrentScriptFile;
-  ASSERT_EFI_ERROR(CommandInit());
+
+  Status = CommandInit ();
+  ASSERT_EFI_ERROR (Status);
 
   if (gEfiShellParametersProtocol->Argc > 1) {
     ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), gShellLevel1HiiHandle, L"if");  
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index cf89a4ac87ed..35a1a7169c8b 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -373,6 +373,8 @@  EFIAPI
 ShellInitialize (
   )
 {
+  EFI_STATUS Status;
+
   //
   // if auto initialize is not false then skip
   //
@@ -383,7 +385,8 @@  ShellInitialize (
   //
   // deinit the current stuff
   //
-  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));
+  Status = ShellLibDestructor (gImageHandle, gST);
+  ASSERT_EFI_ERROR (Status);
 
   //
   // init the new stuff