Re: [edk2] proposal to handle invalid compiler warnings

Subject: Re: [edk2] proposal to handle invalid compiler warnings

From: Laszlo Ersek <lersek@redhat.com>

To: Bill Paul <wpaul@windriver.com>, edk2-devel@lists.sourceforge.net

Date: 2014-03-26 00:14:19

On 03/25/14 00:12, Bill Paul wrote:
> Of all the gin joints in all the towns in all the world, Laszlo Ersek had to 
> walk into mine at 06:32:57 on Monday 24 March 2014 and say:
> 
>> On 03/23/14 03:19, Scott Duplichan wrote:
>>> Laszlo Ersek [mailto:lersek@redhat.com] wrote:
>>>
>>> ]Hence for gcc-4.4 to gcc-4.6, we should degrade -Wuninitialized from
>>> ]error to warning.
>>>
>>> Has anyone proposed reducing the number of gcc versions supported?
>>> Would dropping support for 4.4, 4,5 and 4.6 create objections?
>>
>> At least RHEL-6 (+derivatives) and Debian oldstable (+derivatives) ship
>> gcc-4.4.
> 
> You're also still using gcc 4.3.0 for the UNIX-like cross-build configuration 
> (via mingw-gcc-build.py). (No, I won't stop reminding you that you support 
> this. :) )

Good :)

> 
> A while back, I asked a question about warnings that I saw in 
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c. The simplest case 
> was:
> 
> EFI_STATUS
> FvbGetPhysicalAddress (
>   IN UINTN                                Instance,
>   OUT EFI_PHYSICAL_ADDRESS                *Address,
>   IN ESAL_FWB_GLOBAL                      *Global,
>   IN BOOLEAN                              Virtual
>   )
> [...]
> {
>   EFI_FW_VOL_INSTANCE *FwhInstance;
>   EFI_STATUS          Status;
>      
>   //
>   // Find the right instance of the FVB private data
>   //
>   Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual);
>   ASSERT_EFI_ERROR (Status);
>   *Address = FwhInstance->FvBase[Virtual];
> 
>   return EFI_SUCCESS;
> }
> 
> [...]
> /home/wpaul/edk/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c: 
> In function 'FvbGetPhysicalAddress':
> /home/wpaul/edk/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c:249: 
> error: 'FwhInstance' may be used uninitialized in this function
> [...]
> 
> (Note that this not the only warning like this which appears in this file; I 
> just picked this one as an example.)
> 
> I see this when apparently nobody else does because I build with the cross-
> compiler configuration mentioned above, using gcc 4.3.0.
> 
> When I asked about this before, you cited part of the GCC documentation that 
> states that the behavior of -Wuninitialized depends on the optimization level.

This warning is also visible with gcc-4.4, but only when building for
Ia32. The warning is not emitted by either gcc-4.8 (arch-independently),
or by gcc-4.4, when building for X64.


> 
> Sure enough, it does. In this case, we're using -Os to make gcc optimize for 
> smallest code size.
> 
> Another contributing factor here is that the GetFvbInstance() function is also 
> in FwBlockService.c.
> 
> The result is that gcc is inlining GetFvbInstance() into 
> FvbGetPhysicalAddress(). Since gcc is combining the two functions together, it 
> can tell that there may be a problem due to FwhInstance potentially being 
> uninitialized.

I believe this inlining is also the difference between X64 and Ia32,
when using gcc-4.4.

> 
> Now, you might be thinking: "Wait a minute... GetFvbInstance() is being 
> declared as a global function, not static. Surely it can't be inlined: it has 
> to be compiled separately so that functions outside this module can invoke 
> it."
> 
> You're right, and gcc is compiling it as a standalone function. It is _ALSO_ 
> inlining it into the all functions that call it within this compilation unit. 
> This is probably an artifact of the optimization logic in this particular 
> version of gcc.
> 
> Go figure.
> 
> Anyway, long story short, I think for this particular set of circumstances, 
> unusual though they may be, the warning has merit.

That could be the case -- I don't want to eliminate the warning, I just
want to prevent it from aborting the build. So that we can defer
"fixing" it.

> 
> Even forgetting the compiler-specific issues, it draws attention to what are 
> arguably real issues with the code. On that note, I have another question: 
> under what circumstances does the ASSERT_EFI_ERROR() macro actually do 
> anything? I noticed that it depends on the macro DEBUG_CODE(), which is a no-
> op if EFI_DEBUG is not defined. So when exactly is EFI_DEBUG defined? I though 
> that building with -b DEBUG would do it (vs. -b RELEASE) but that doesn't seem 
> to do it. Am I missing something?

If you run ctags on the edk2 tree, and use a compatible editor, you can
follow ASSERT_EFI_ERROR(). The macro is defined in
"MdePkg/Include/Library/DebugLib.h", and its actions depend on
MDEPKG_NDEBUG being *undefined*.

OVMF in particular sets MDEPKG_NDEBUG for RELEASE builds, see the
OvmfPkg/OvmfPkg*.dsc files (the [BuildOptions] section).

If you build with GCC and -b DEBUG, then ASSERT_EFI_ERROR() should work.
I'm not sure what mingw-gcc-build.py does; maybe it defines
MDEPKG_NDEBUG even for DEBUG builds.

> 
> Whatever the case, since ASSERT_EFI_ERROR() has no effect if EFI_DEBUG isn't 
> defined, we end up with no error checking of the GetFvbInstance() return 
> value, which allows us to fall through and potentially dereference an invalid 
> FwhInstance pointer if GetFvbInstance() fails, which is what makes time travel 
> possible.
> 
> Er... or not. (One day I'll figure out the right beginning to that sentence. 
> One day.)
> 
> Anyway, at this point I'm sure someone is thinking "well, if we drop support 
> for older versions of gcc, that'll take care of that problem, right?" In fact 
> I think someone already suggested this.
> 
> I don't think that's the right answer. While gcc 4.3.0's behavior here may be 
> a little unusual, I'm not sure that it's necessarily wrong. If it's not wrong, 
> then who's to say that a future version of gcc won't exhibit the same behavior 
> again?
> 
> I know that most compiler warnings are irritating and have little to no actual 
> effect on runtime behavior, but every once in a while they highlight real 
> problems. I think this is one of those times. So my opinion is that I'd rather 
> see people just fix the warnings rather than find ways around them.

I want to keep warnings enabled, and I want to fix them. What I don't
want is warnings breaking the build for people who happen to use a
different compiler from mine. That is, warnings I never had a chance to see.

Let me quote my original email -- I proposed these flags:

GCC:*_GCC44_*_CC_FLAGS               = -Wno-error=uninitialized
GCC:*_GCC45_*_CC_FLAGS               = -Wno-error=uninitialized
GCC:*_GCC46_*_CC_FLAGS               = -Wno-error=uninitialized
GCC:*_GCC47_*_CC_FLAGS               = -Wno-error=maybe-uninitialized
GCC:*_GCC48_*_CC_FLAGS               = -Wno-error=maybe-uninitialized

They do *not* disable these warnings. They just downgrade them to
warnings, where otherwise they'd count as errors (thanks to a preceding,
global -Werror). The idea is that developers capture the warnings in the
build log that *their* compiler version happens to emit, for their
patches, fix them up, and the patches get committed. When someone else
encounters these same types of warnings, but for a different set of code
locations (due to a different compiler version), they either notice the
warnings (and report them), or they don't. But certainly the build won't
break for them.

Compiler warnings are a *help for developers*. They aren't supposed to
be a *nuisance for users*. Noticing and addressing warnings are a
responsibility for people who both (a) are developers, (b) see the
warnings directly or learn about them from external reports.

I probably overused *bold*. Sorry about that.

Thanks
Laszlo

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel