Laszlo, Thank you for the sharing. Your patches really help to improve the EDKII code quality.
Regards, Ray >-----Original Message----- >From: Laszlo Ersek [mailto:[email protected]] >Sent: Wednesday, June 29, 2016 7:50 PM >To: Ni, Ruiyu <[email protected]>; edk2-devel-01 <[email protected]> >Cc: Carsey, Jaben <[email protected]>; Qiu, Shumin <[email protected]> >Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side >effects in ASSERT_EFI_ERROR() > >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
