Hello Mike, Thanks for the details. I do not understand your comment about "a debug issue seen when optimization and comdat folding is disabled". EDK2 enables comdat folding for all builds (RELEASE, DEBUG, NOOPT). So comdat folding is encountered even when stepping through a DEBUG or NOOPT build.
I don't have any objection to removal of static functions in BaseOrderedCollectionRedBlackTreeLib. I just used this as an opportunity to ask about static functions. Thanks for explaining that static functions are permitted. Thanks, Scott -----Original Message----- From: Kinney, Michael D [mailto:[email protected]] Sent: Wednesday, August 20, 2014 11:22 AM To: [email protected] Subject: Re: [edk2] [PATCH] MdePkg: refine code for BaseOrderedCollectionRedBlackTreeLib Scott, comdat folding is related to optimization. My other email on this thread describes a debug issue seen when optimization and comdat folding is disabled along with an example that can be used to evaluate the issue on different debuggers. I want to clarify that there is no "ban" on the use of static. That is legal C syntax and developers are welcome to use static in their packages. It is only a recommendation for EDK II packages that are shared by all platforms to improve debug. I have had my own debug experiences where I have wasted many hours root causing difficult firmware issues only to find the debugger was showing me the incorrect function names. At the time, discouraging the use of static seemed like a reasonable recommendation. MdePkg/Include/Base.h defines STATIC with the intended use for variables/datum elements. /// /// Datum is scoped to the current file or function. /// #define STATIC static I agree that if we want to change this from a recommendation to a requirement that it would need to be clearly documented in the EDK II coding document along with clear justification for the requirement. Even as a recommendation, it would be good to mention in the EDK II coding document and the EDK II module writer's guide along with clear justification for the recommendation. Given that all of the EDKI II packages shared by all platforms are consistently following this recommendation, do you see any harm in applying this recommendation to the new MdePkg BaseOrderedCollectionRedBlackTreeLib? We could also work on evaluating all the latest debuggers to see if this debug issue seen in the past is still present or not. If the no debuggers have the issue, then the recommendation could be dropped. Thanks, Mike -----Original Message----- From: Scott Duplichan [mailto:[email protected]] Sent: Tuesday, August 19, 2014 10:28 PM To: [email protected] Subject: Re: [edk2] [PATCH] MdePkg: refine code for BaseOrderedCollectionRedBlackTreeLib Brian J. Johnson [mailto:[email protected]] wrote: [...] ]> In this case, even the Microsoft Visual Studio debugger gives ]> the appearance of stepping through the 'wrong' function. But ]> the linker /verbose output confirms the debugger is correct: ]> ]> Selected symbol: ]> _f1 from 1.obj ]> Replaced symbol(s): ]> _f2 from 1.obj ]> _f3 from 1.obj ] ]FWIW I have seen this behavior most commonly with functions which, after ]optimization, simply return a constant (eg. TRUE, or EFI_SUCCESS.) It's ]a bit surprising when looking at symbol names, but it always seems to ]produce correct results. ]-- ] ] Brian That is my experience too. I see now that this optimization is called 'comdat folding' and is explained here: http://msdn.microsoft.com/en-us/library/bxwfs976.aspx Here are a few cases of comdat folding from a current EDK2 OVMF build using VS2010 tools: Selected symbol: PeiDefaultIoWrite16 from PeiCore.lib(CpuIo.obj) Replaced symbol(s): PeiDefaultIoWrite32 from PeiCore.lib(CpuIo.obj) PeiDefaultIoWrite64 from PeiCore.lib(CpuIo.obj) PeiDefaultIoWrite8 from PeiCore.lib(CpuIo.obj) PeiDefaultMemWrite16 from PeiCore.lib(CpuIo.obj) PeiDefaultMemWrite32 from PeiCore.lib(CpuIo.obj) PeiDefaultMemWrite64 from PeiCore.lib(CpuIo.obj) PeiDefaultMemWrite8 from PeiCore.lib(CpuIo.obj) Selected symbol: CoreEmptyCallbackFunction from DxeCore.lib(Dispatcher.obj) Replaced symbol(s): SzFree from LzmaDecompressLib.lib(LzmaDecompress.obj) Selected symbol: DxePcdSet8 from PcdDxe.lib(Pcd.obj) Replaced symbol(s): DxePcdSetBool from PcdDxe.lib(Pcd.obj) Here is a case where more complex functions are folded. The source code of the merged functions is quite different, but evidently the intermediate code is the same: Selected symbol: HexCharToUintn from UefiShellDebug1CommandsLib.lib(UefiShellDebug1CommandsLib.obj) Replaced symbol(s): InternalHexCharToUintn from BaseLib.lib(String.obj) Building OVMF using VS2010 results in 35 cases of comdat folding. I confirmed the optimization can be prevented replacing linker options " /OPT:REF /OPT:ICF=10" with "/NOICF". Avoiding the use of static functions is _not_ preventing the comdat folding optimization. So either the original problem was due to another cause, or more likely the removal of static functions did avoid comdat folding when a different compiler version and/or options was used. I think the decision to ban the use of static functions should be revisited. If the original debugger problem can be reproduced and confirmed to be due to comdat folding, then engineers can learn to recognize and appreciate comdat folding and understand that debugger behavior is correct. Alternatively, comdat folding could be disabled for the NOOPT build. If the original debugger problem is not due to comdat folding, then it should be documented along with steps to reproduce in case someone wants to investigate. Right now, the static function ban is not even mentioned in an EDK2 coding document. Banning a C language feature as mainstream as static functions should be avoided if possible, and carefully documented if not. Thanks, Scott ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
