On 03/18/16 17:26, Ard Biesheuvel wrote: > On 18 March 2016 at 17:22, Laszlo Ersek <[email protected]> wrote: >> On 03/18/16 15:48, Gao, Liming wrote: >>> Laszlo: >>> >>> I understand the request to BaseTools is that AutoGen code can pass >>> GCC or MSFT compiler without any warning. If so, EDKII module can enable >>> non default warnings in itself. I will evaluate BaseTools. >> >> Thank you! >> >>> And, you raise one issue that the different ARCHs have the different >>> warning setting. I suggest to keep them consistent to avoid the >>> different build results. How about adding -Wno-unused-but-set-variable >>> option to AARCH64 DEBUG. >> >> Can we move in the other direction perhaps? Can we remove >> -Wno-unused-but-set-variable from the following settings: >> >> - GCC46_IA32_CC_FLAGS >> - GCC46_X64_CC_FLAGS >> >> (these are inherited by the >= 4.6 GCC settings for IA32 and X64,) >> >> - RELEASE_GCC46_ARM_CC_FLAGS >> - RELEASE_GCC47_ARM_CC_FLAGS >> - RELEASE_GCC47_AARCH64_CC_FLAGS >> - RELEASE_GCC48_ARM_CC_FLAGS >> - RELEASE_GCC48_AARCH64_CC_FLAGS >> - RELEASE_GCC49_ARM_CC_FLAGS >> - RELEASE_GCC49_AARCH64_CC_FLAGS >> >> I believe all of the above settings include >> "-Wno-unused-but-set-variable" only for the following reason: >> >> When GCC46 was enabled in BaseTools, the "-Wall" option (which we do >> enable as a basis) suddenly started including >> "-Wunused-but-set-variable" too. Namely, that warning option was new in >> the gcc-4.6 release, and "-Wall" by default included it: >> >> https://gcc.gnu.org/gcc-4.6/changes.html >> >> The edk2 tree wouldn't build with "-Wunused-but-set-variable"; so, in >> order to expedite GCC46 enablement, the "-Wno-unused-but-set-variable" >> was set. See commit 2bcc713e74b94. >> >> However, in reality, the number of warnings triggered by this setting is >> very low, in my testing -- just a handful. I listed them yesterday; the >> following modules are affected (while building ArmVirtPkg and OvmfPkg): >> >> - MdeModulePkg/Bus/Pci/PciHostBridgeDxe >> - UefiCpuPkg/Library/MtrrLib >> - UefiCpuPkg/PiSmmCpuDxeSmm >> >> If I send a series that cleans up these warnings, would you agree (and >> perhaps even test with your own platforms) to remove >> "-Wno-unused-but-set-variable"? >> >> I agree that we should unify the warning flags, but I think we should >> move towards stricter warnings, not laxer warnings -- provided we can >> adapt the tree more or less easily. >> > > I strongly agree with Laszlo here. The issues that were caught by this > warning are all examples of where refactoring of code results in > variables to be left behind that are completely unused. The EDK2 > codebase is difficult enough to understand as it is, and having > variables with no purpose left behind only makes that worse. > > And indeed, since some platforms already use this flag, the codebase > is mostly clean with respect to such occurrences, and could easily be > kept that way.
... I started this work. I've written ~33 patches thus far. The code base is much less clean than I expected. The mess comes to sunlight when you build the top-level packages in full, using their own DSC files. Hence the 33 patches, and I still haven't looked at a handful of top-level packages. Unfortunately, I've run into a big issue: the DEBUG(()) macro. (To a smaller extent, the ASSERT_EFI_ERROR() macro as well.) These macros expand to the null replacement string when MDEPKG_NDEBUG is defined (which is occasionally the case for RELEASE builds). If those macro invocations constitute the only read accesses to some local variables, then the MDEPKG_NDEBUG build triggers "-Wunused-but-set-variable". For ASSERT_EFI_ERROR(), I could work it around like this: ((VOID)(TRUE ? 0 : (StatusParameter))) but I have no clue how to do the same with DEBUG(), which can receive any number of arguments (of any type too). This seems like a deal breaker to me. I could surround all those local variable definitions with "#ifndef MDEPKG_NDEBUG", but it would be very ugly. :( Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
