Of all the gin joints in all the towns in all the world, Laszlo Ersek had to walk into mine at 13:01:37 on Tuesday 24 June 2014 and say:
> On 06/24/14 20:11, Bill Paul wrote: > > There are actually 5 places in FwBlockService.c where FwhInstance isn't > > initialized. > > These are the call chains to GetFvbInstance(): > > FvbProtocolGetPhysicalAddress() -- protocol member > FvbGetPhysicalAddress() > GetFvbInstance() > > FvbProtocolGetAttributes() -- protocol member > FvbGetVolumeAttributes() > GetFvbInstance() > > FvbProtocolGetBlockSize() -- protocol member > FvbGetLbaAddress() > GetFvbInstance() > > FvbProtocolSetAttributes() -- protocol member > FvbSetVolumeAttributes() > GetFvbInstance() > > FvbProtocolEraseBlocks() -- protocol member > GetFvbInstance() > > Facts: > - The GetFvbInstance() function always conforms to the following > postcondition (note that Instance is an input parameter): > > (Instance >= Global->NumFv && > retval == EFI_INVALID_PARAMETER && > *FwhInstance is not touched) || > > (Instance < Global->NumFv && > retval == EFI_SUCCESS && > *FwhInstance is set) > > In English, a valid Instance argument guarantees that you get > EFI_SUCCESS and *FwhInstance gets set; otherwise you get > EFI_INVALID_PARAMETER and *FwhInstance is not touched. > > - All five call chains above pass FvbDevice->Instance as Instance. Each > outermost protocol member ensures this. If there's another function > between the outermost protoco member and the inner GetFvbInstance() > function, then the middle function just passes along Instance > unmodified. > > - "FvbDevice->Instance" is set in FvbInitialize(): > > FvbDevice->Instance = mFvbModuleGlobal->NumFv; > mFvbModuleGlobal->NumFv++; > > - In the protocol member functions, the FvbDevice pointer is derived > from the This parameter (the protocol interface). It is the protocol > user's responsibility to pass a valid This argument to the member > functions. The CR() macro, underlying the FVB_DEVICE_FROM_THIS() > macro, ASSERT()s in DEBUG builds that the signature matches. An > invalid This argument is a breach of contract. > > These particular five callchains rightfully demand that GetFvbInstance() > succeed; a retval different from EFI_SUCCESS Should Never Happen (TM) > and deserves an ASSERT(). And, EFI_SUCCESS implies that *FwhInstance is > set. > > GCC may not see it, but the ASSERT()s are not laziness; they are > correct. They mean "I'm passing a correct Instance argument; if you fail > then the world has ended". As I've said, the ASSERT() macros resolve to no-ops in some cases. If the world does end, we want to be informed of it reliably, and we won't be if the ASSERT()s turn to no-ops. > Shutting up GCC is the right thing to do. We will have to agree to disagree on this. Nevertheless, I think we can agree that the code as it is needs to be changed in some way. > > Whether they all get flagged by the compiler or not depends on > > what version of GCC you use and just how aggressively it chooses to do > > inlining. You can actually defeat the warnings by adding -fno-inline to > > CFLAGS. (GCC only notices that the uninitialized pointers are a problem > > when the smaller functions containing the FwhInstance declaration are > > inlined into other functions.) > > > > However that just shuts up the compiler; I think the correct thing to do > > is to actually clean up the way the code does error handling. Right now > > everything > > > > depends on asserts, e.g.: > > // > > // Find the right instance of the FVB private data > > // > > Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual); > > ASSERT_EFI_ERROR (Status); > > *Address = FwhInstance->FvBase[Virtual]; > > > > The problem is that if you decide to do a build with MDEPKG_NDEBUG > > defined, then ASSERT_EFI_ERROR() resolves to a no-op, and then you could > > end up dereferencing a NULL or garbage pointer if GetFvbInstance() > > fails. > > This should never happen (assuming that the user of the protocol passes > in the correct protocol address as This, which is something the code is > allowed to assume; hence the ASSERT()s are justified). > > > Note that for me this always happens because I do RELEASE builds using > > the UNIXGCC toolchain (the native toolchain on FreeBSD 9.x doesn't quite > > cut it), and as it says in the OVMF README: > > > > [...] > > === UNIXGCC Debug === > > > > If you build with the UNIXGCC toolchain, then debugging will be disabled > > due to larger image sizes being produced by the UNIXGCC toolchain. The > > first choice recommendation is to use GCC44 or newer instead. > > [...] > > > > And indeed, in OvmfPkgIa32.dsc, it says: > > > > [BuildOptions] > > > > GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG > > GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG > > INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG > > MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG > > GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse > > > > Now, please don't tell me that the fact that the warnings only show on > > some versions of GCC is a compiler problem and I should be using a newer > > one instead. The GCC toolchain generated by the mingw-gcc-build.py works > > perfectly well. It isn't buggy. The warnings are not false positives. > > They are false positives, given the protocol user's responsibilities > (ie. "pass in the valid This argument, then everything will work"). > GCC's data flow analysis can't see the above reasoning. > > I'm not arguing that it should be able to derive the above argument -- > gcc errs on the safe side, but then it's justified for the programmer to > shut it up. > > > Using a newer GCC > > doesn't "fix" the problem: it _hides_ the problem. (And for all you know, > > a future version of GCC might start emitting the warnings too.) I'm > > pretty sure a static analysis tool like Coverity would barf all over > > this too. > > Coverity is awesome, but it also generates reports chock full of > impossible cases (which are impossible due to constraints and > responsibilities that are not visible to Coverity). > > > And please also don't tell me: "Hey Bill, that's great, if you could just > > submit a patch..." I'm not submitting a patch. Because just initializing > > the pointers isn't the right fix, > > Indeed it isn't. Initializing the pointers is a kludge that shuts up the > false positives of the compiler, so that it doesn't trip us up in these > particular locations, and we can continue to requests its warnings, > because in some cases they are valid. The right fix would be either > enhancing the compiler, or annotating the code with extra information > for the compiler. *sigh* > I hate such kludges with a passion, but GCC's diagnostics pragmas (which > would allow you to suppress warnings only in specific spots) are not > allowed in edk2. I had suggested to use special values in such kludge > initializers / assignments that communicate clearly that the actual > value is meaningless (eg. (VOID*)0xDEADC0DE rather than NULL). The > proposal was rejected for style reasons. Fine. I don't care how you deal with it, but could you please just do us all a favor and get rid of the warnings? -Bill > Thanks > Laszlo > > --------------------------------------------------------------------------- > --- Open source business process management suite built on Java and Eclipse > Turn processes into business applications with Bonita BPM Community > Edition Quickly connect people, data, and systems into organized workflows > Winner of BOSSIE, CODIE, OW2 and Gartner awards > http://p.sf.net/sfu/Bonitasoft > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel -- ============================================================================= -Bill Paul (510) 749-2329 | Senior Member of Technical Staff, [email protected] | Master of Unix-Fu - Wind River Systems ============================================================================= "I put a dollar in a change machine. Nothing changed." - George Carlin ============================================================================= ------------------------------------------------------------------------------ Open source business process management suite built on Java and Eclipse Turn processes into business applications with Bonita BPM Community Edition Quickly connect people, data, and systems into organized workflows Winner of BOSSIE, CODIE, OW2 and Gartner awards http://p.sf.net/sfu/Bonitasoft _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
