Ard, I agree that it would be good practice for a library instance to only have the public interfaces(functions/data) that are declared in the library class .h file.
I would be useful to have a report that showed the extra interfaces. One additional constraint I just found with the MSFT tool chains involves the GLOBAL_REMOVE_IF_UNREFERENCED. This macro can not be combined with static. So the recommendation in the thread to add static to mMemoryDiscoveredNotifyList in SecPeiDebugAgentLib breaks the build. d:\work\github\tianocore\edk2\SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c(35): error C2496: 'mDebugAgentMemoryDiscoveredNotifyList': 'selectany' can only be applied to data items with external linkage The MSFT tool chains depend on the following #define in order for The optimizer to remove global variables that are not used. This is a valuable size optimization for libraries that may be sparsely used by modules. #define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany) Mike > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Wednesday, May 24, 2017 2:44 PM > To: Kinney, Michael D <[email protected]> > Cc: Laszlo Ersek <[email protected]>; Wu, Hao A <[email protected]>; edk2- > [email protected]; [email protected]; Fan, Jeff <[email protected]> > Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix > duplicate > symbol > > On 24 May 2017 at 13:18, Kinney, Michael D <[email protected]> wrote: > > Laszlo, > > > > I agree with the request to add 'static' to the variable declaration > > in the SecPeiDebugAgentLib. The variable name change will be retained > > because the same symbol name can still be confusing when debugging. > > > > The part that is more concerning is why this duplicate symbol reference > > was not triggered by MSFT or GCC tool chain families, so I did some > > more investigation from the MSFT side this morning. > > > > My first thought was that the optimizer in the compiler/linker was > > optimizing away the duplicate symbol before the final link stage, > > so I tried -b NOOPT builds. That also did not trigger a duplicate > > symbol error and the MAP file contains only the single reference to > > _mMemoryDiscoveredNotifyList from the DxeIpl. > > > > 0002:000001b8 _mMemoryDiscoveredNotifyList 000164f8 > > DxeIpl:DxeLoad.obj > > > > I then evaluated what functions in the DebugAgentLib the DxeIpl > > is using. It turns out that it is only using a single function > > SaveAndSetDebugTimerInterrupt() that is implemented in > > > > SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c > > > > The mMemoryDiscoveredNotifyList duplicate symbol is implemented in > > > > > > SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c > > > > Even with a MSFT -b NOOPT build, when I look at the MAP file, the > > linker only pulls in symbols from the obj in the DebugAgentLib that > > contains the one SaveAndSetDebugTimerInterrupt() function. The symbols > > from the other objs in the DebugAgentLib are not included in the final > > DxeIpl PE/COFF image. > > > > 0001:000081b0 _InitializeDebugTimer 00008410 f > SecPeiDebugAgentLib:DebugTimer.obj > > 0001:00008330 _IsDebugTimerTimeout 00008590 f > SecPeiDebugAgentLib:DebugTimer.obj > > 0001:000083d0 _SaveAndSetDebugTimerInterrupt 00008630 f > SecPeiDebugAgentLib:DebugTimer.obj > > > > So it appears that even with -b NOOPT builds, the linker is > > filtering out unreferenced objs which explains why the duplicate > > symbol error is not triggered. > > > > It does not need to filter out what it never considered for inclusion > in the first place. That is why it is called a library: only objects > that satisfy some as yet unresolved symbol reference will get pulled > in. > > > The XCODE5 tool chain appears to be evaluating symbols > > across all objs in a lib and detects a duplicate symbol. > > > > The question is what is the required linker behavior for EDK II > > builds? Require library filtering at obj level, or require no > > duplicates across all objs in all libs? > > > > Require no *externally visible* duplicates at all. That is really the > only scaleable solution. Libraries declare their externally visible > symbols in their .h file, and ideally, nothing else should be exposed. > I know this is infeasible in the general case, due to the fact that > libraries typically consist of several objects, but it should be the > goal not to export anything if we can avoid it. Note that this applies > to functions as well as variables _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
