New subject: [PATCH] Comment out ASSERTs testing non-NULL vars for NULL

7 Jul 2011

On Wed, Jul 06, 2011, Olivier Martin wrote:
...
Anyway, these are the build instructions to build the Tianocore project
(UEFI Open Source implementation) and to test on qEmu:
http://edk2.svn.sourceforge.net/viewvc/edk2/trunk/edk2/BeagleBoardPkg/readme
.txt?revision=11997
I had started following these, but ran into some issues with my setup;
 I will try with the alternate build-next.sh approach too, to see
 whether I get other issues or not.
The two classes of issues I ran into was:
 a) variables being written to, but never read
 b) conditions being always true
I indeed had to apply the patch you mention; what's the state of that
 patch?  Are these things which will get merged soonish?  Without the
 patch, my toolchain wouldn't be properly picked up (it'd try running
 c:/program files/ stuff and calling a CS toolchain) and I had some
 error conditions like '-(' in linker invocations due to missing ''
 escapes.  These go away with the patch and my toolchain is picked up
 and works.
I'm running Ubuntu oneiric (which will become 11.10) and the toolchain
 I've used is our packaged Linaro GCC based cross-compiler
 (arm-linux-gnueabi-gcc from the gcc-arm-linux-gnueabi package).
a) variables being written to, but never read
This is stuff like:

--- a/edk2/BaseTools/Source/C/EfiLdrImage/EfiLdrImage.c
+++ b/edk2/BaseTools/Source/C/EfiLdrImage/EfiLdrImage.c
@@ -181,7 +181,7 @@ Returns:
   CHAR8* OutputFileName = NULL;
   CHAR8* InputFileNames[MAX_PE_IMAGES + 1];
   UINT8 InputFileCount = 0;
-  BOOLEAN QuietFlag = FALSE;
+  //BOOLEAN QuietFlag = FALSE;
   UINT64        DebugLevel = 0;
   UINT64        VerboseLevel = 0;
   EFI_STATUS Status = EFI_SUCCESS;
@@ -220,7 +220,7 @@ Returns:
     }
if ((stricmp (argv[0], "-q") == 0) || (stricmp (argv[0], "--quiet") == 0)) {
-      QuietFlag = TRUE;
+      //QuietFlag = TRUE;
       argc --;
       argv ++;
       continue;
QuietFlag is never used (read), newer gccs detect this and report it as
 a fatal warning by default (not sure where this warning is configured
 as a fatal one, whether it's in UEFI or in the toolchain config).
There are many such cases, but some are a bit more worrying, like
 variables holding the return values of fread of fwrite not being
 checked:
--- a/edk2/BaseTools/Source/C/GenVtf/GenVtf.c
+++ b/edk2/BaseTools/Source/C/GenVtf/GenVtf.c
@@ -1141,7 +1141,7 @@ Returns:
   EFI_STATUS  Status;
   UINT64      CompStartAddress;
   UINT64      FileSize;
-  UINT64      NumByteRead;
+  //UINT64      NumByteRead;
   UINT64      NumAdjustByte;
   UINT8       *Buffer;
   FILE        *Fp;
@@ -1189,7 +1189,7 @@ Returns:
     //
     // Read first 64 bytes of PAL header and use it to find version info
     //
-    NumByteRead = fread (Buffer, sizeof (UINT8), SIZE_OF_PAL_HEADER, Fp);
+    /* NumByteRead = */fread (Buffer, sizeof (UINT8), SIZE_OF_PAL_HEADER, Fp);
//
     // PAL header contains the version info. Currently, we will use the header
@@ -1200,7 +1200,7 @@ Returns:
     }
   }
-  NumByteRead = fread (Buffer, sizeof (UINT8), (UINTN) FileSize, Fp);
+  /* NumByteRead = */fread (Buffer, sizeof (UINT8), (UINTN) FileSize, Fp);
   fclose (Fp);
//
I just quickly hacked the source to get it to build, but we should fix
 the code to loop until all bytes have been read or to raise an error.
The last such cases is a conditional piece of code:
--- a/edk2/BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c
+++ b/edk2/BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c
@@ -1919,11 +1919,13 @@ static SRes LzmaEnc_CodeOneBlock(CLzmaEnc *p, Bool useLimits, UInt32 maxPackSize
 static SRes LzmaEnc_Alloc(CLzmaEnc *p, UInt32 keepWindowSize, ISzAlloc *alloc, ISzAlloc *allocBig)
 {
   UInt32 beforeSize = kNumOpts;
+  #ifdef COMPRESS_MF_MT
   Bool btMode;
+  #endif
   if (!RangeEnc_Alloc(&p->rc, alloc))
     return SZ_ERROR_MEM;
-  btMode = (p->matchFinderBase.btMode != 0);
   #ifdef COMPRESS_MF_MT
+  btMode = (p->matchFinderBase.btMode != 0);
   p->mtMode = (p->multiThread && !p->fastMode && btMode);
   #endif
b) conditions being always true
A bunch of asserts trigger a warning in newer GCCs, essentially the
 ASSERT macros in question are doing multiple things:
 - testing whether the object is null
 - testing some other condition on the object, like some state being
   correct
 but the objects are guaranteed to never be null from GCC PoV, so it
 raises a fatal warning; as a workaround I just commented the asserts
 out, but I guess what we should do is review whether any of the
 ASSERT_LOCKED() users has a chance of getting a NULL pointer, and
 either drop the NULL check or replace the macro with two.
--- a/edk2/MdeModulePkg/Core/Dxe/Event/Event.c
+++ b/edk2/MdeModulePkg/Core/Dxe/Event/Event.c
@@ -225,7 +225,7 @@ CoreNotifyEvent (
   //
   // Event database must be locked
   //
-  ASSERT_LOCKED (&gEventQueueLock);
+  //ASSERT_LOCKED (&gEventQueueLock);
//
   // If the event is queued somewhere, remove it
I got this for a bunch of ASSERT_LOCKED(),
 ASSERT_PROTOCOL_ALREADY_INSTALLED(), and ASSERT_VOLUME_LOCKED()
These are all issues in edk2, except the ASSERT_VOLUME_LOCKED() macro
 issue which is the only issue in FatPkg and only there.
A cleaner workaround would be to make this warnings non-fatal for now
 for this toolchain, but the code should also be fixed not to trigger
 them.  What's the best way to address this?  Is there some bug tracker
 I should report these issues at?
To try the resulting image, I just did:
    qemu-system-arm -mtdblock \
        ../Build/BeagleBoard/DEBUG_ARMGCC/FV/BeagleBoard_EFI_flashboot.fd \
        -nographic -M beagle
which sent this on the serial port:
UART Enabled
Magic delay to disable watchdog timers properly.
ASSERT_EFI_ERROR (Status = Unsupported)
ASSERT /home/lool/git/edk2/edk2/edk2/EmbeddedPkg/Library/PrePiLib/PrePiLib.c(73): !EFI_ERROR (Status)
I didn't actually look into this issue yet.
I tried passing a barebox based SD image I had built earlier (see other
 message) with -sd sd.img, but that didn't change the output.
-- 
Loïc Minier


    

Re: UEFI on BeagleBoard qEmu