| Message ID | CAF7UmSx4WiM2kMCCtgLJio3iuhqewYRdb_eyEsPvdQw_-O=F+g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. > Besides, Shumin is a young man. J Thanks, I'll keep it in mind. :) Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 18 March 2016 at 17:22, Laszlo Ersek <lersek@redhat.com> 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. Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/18/16 17:26, Ard Biesheuvel wrote: > On 18 March 2016 at 17:22, Laszlo Ersek <lersek@redhat.com> 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/19/16 00:27, Andrew Fish wrote: > >> On Mar 18, 2016, at 3:46 PM, Michael Brown <mcb30@ipxe.org> wrote: >> >> On 18/03/16 22:30, Laszlo Ersek wrote: >>> 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). >> >> With the caveat that I haven't looked at how DEBUG() is implemented in EDK2: >> >> Could this rely upon dead code elimination? At least gcc will happily accept something which expands to >> >> if ( 0 ) { >> do_something_with ( a_variable ); >> } >> >> and treat that as being a "use" of a_variable (for the purposes of -Wunused-but-set-variable). >> > > All the DEBUG macros work this way all ready. MDEPKG_NDEBUG was a hack-a-round for compilers that don't do link time optimization. Like, all versions of gcc that are currently supported? :) > So you don't need to redefine anything, just don't use MDEPKG_NDEBUG. That sounds like good advice, but reality would have to catch up: $ git grep MDEPKG_NDEBUG -- '*.dsc' ArmPkg/ArmPkg.dsc: RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc: GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc: GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc: INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc: MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc: GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc: GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc: INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc: MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32.dsc: GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32.dsc: GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32.dsc: INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32.dsc: MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32X64.dsc: GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32X64.dsc: GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32X64.dsc: INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG OvmfPkg/OvmfPkgIa32X64.dsc: MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG OvmfPkg/OvmfPkgX64.dsc: GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG OvmfPkg/OvmfPkgX64.dsc: GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG OvmfPkg/OvmfPkgX64.dsc: INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG OvmfPkg/OvmfPkgX64.dsc: MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc: ICC:*_*_*_CC_FLAGS = -D MDEPKG_NDEBUG Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc: GCC:*_*_*_CC_FLAGS = -D MDEPKG_NDEBUG Vlv2TbltDevicePkg/PlatformPkgIA32.dsc: ICC:*_*_*_CC_FLAGS = /D MDEPKG_NDEBUG Vlv2TbltDevicePkg/PlatformPkgIA32.dsc: GCC:*_*_*_CC_FLAGS = -D MDEPKG_NDEBUG Vlv2TbltDevicePkg/PlatformPkgX64.dsc: ICC:*_*_*_CC_FLAGS = /D MDEPKG_NDEBUG Vlv2TbltDevicePkg/PlatformPkgX64.dsc: GCC:*_*_*_CC_FLAGS = -D MDEPKG_NDEBUG > That will give you the same path as VC++ and clang (assuming LTO is enabled). Well, LTO is unusable in edk2 with the supported GCC toolchains, to my knowledge. > https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/DebugLib.h > > #if !defined(MDEPKG_NDEBUG) > #define DEBUG(Expression) \ > do { \ > if (DebugPrintEnabled ()) { \ > _DEBUG (Expression); \ > } \ > } while (FALSE) > #else > #define DEBUG(Expression) > #endif > > > If you have code that only exists in DEBUG builds you can use these macros to isolate it. > > /** > Macro that marks the beginning of debug source code. > If the DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of PcdDebugProperyMask is set, > then this macro marks the beginning of source code that is included in a module. > Otherwise, the source lines between DEBUG_CODE_BEGIN() and DEBUG_CODE_END() > are not included in a module. > **/ > #define DEBUG_CODE_BEGIN() do { if (DebugCodeEnabled ()) { UINT8 __DebugCodeLocal > > > /** > The macro that marks the end of debug source code. > If the DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of PcdDebugProperyMask is set, > then this macro marks the end of source code that is included in a module. > Otherwise, the source lines between DEBUG_CODE_BEGIN() and DEBUG_CODE_END() > are not included in a module. > **/ > #define DEBUG_CODE_END() __DebugCodeLocal = 0; __DebugCodeLocal++; } } while (FALSE) Right, but the code that triggers the problem is more like: CHAR8 *Message; Message = ""; if (whatever) { // do some real logic Message = "blah\n"; // do some more real logic } DEBUG ((EFI_D_INFO, "%a\n", Message)); I.e., it's hard to find a common, tight scope for all uses of the variable in question. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/22/16 04:21, Gao, Liming wrote: > Laszlo: > Could we separate the warning option topic? Sure. > I think we still need > more discussion on how to enable warning unused-but-set-variable when > MDEPKG_NDEBUG is defined. One simple way is to disable this warning > when MDEPKG_NDEBUG is set. TBH, I shelved my patches after sending my last email -- since the warning cannot be generally enabled due to gcc not supporting LTO with edk2, I kind of lost interest. Thanks Laszlo > > For the patch to add UNUSED notation to Base.h, I have no comment. > > Thanks > Liming >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Saturday, March 19, 2016 12:22 AM >> To: Gao, Liming; Leif Lindholm >> Cc: Ard Biesheuvel; edk2-devel@lists.01.org; Andrew Fish; Kinney, Michael D; >> Qiu, Shumin >> Subject: Re: [edk2] [PATCH] MdePkg: add UNUSED notation to Base.h >> >> 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. >> >>> Besides, Shumin is a young man. J >> >> Thanks, I'll keep it in mind. :) >> >> Laszlo > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/BaseTools/Source/Python/AutoGen/GenC.py b/BaseTools/Source/Python/AutoGen/GenC.py index 842d8bd..418bc47 100644 --- a/BaseTools/Source/Python/AutoGen/GenC.py +++ b/BaseTools/Source/Python/AutoGen/GenC.py @@ -59,6 +59,9 @@ gAutoGenHeaderString = TemplateString("""\ ${FileName} Abstract: Auto-generated ${FileName} for building module or library. **/ +#pragma GCC diagnostic ignored "-Wunused-macros" +#pragma GCC diagnostic ignored "-Wmissing-variable-declarations" +#pragma GCC diagnostic ignored "-Wunused-parameter" """) gAutoGenHPrologueString = TemplateString("""