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:[email protected]] On Behalf Of >> Laszlo Ersek >> Sent: Tuesday, June 28, 2016 9:26 PM >> To: edk2-devel-01 <[email protected]> >> Cc: Carsey, Jaben <[email protected]>; Qiu, Shumin >> <[email protected]> >> 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 <[email protected]> >> Cc: Shumin Qiu <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> >> 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 >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
