Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek <lersek@redhat.com>]

On 05/24/17 22:18, Kinney, Michael D 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.
>
> The XCODE5 tool chain appears to be evaluating symbols
> across all objs in a lib and detects a duplicate symbol.

Thank you for the thorough investigation. I agree that keeping the name
change *and* adding STATIC is the best combination.

> 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?

Consider "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c":

> CONST EFI_PEI_NOTIFY_DESCRIPTOR mMemoryDiscoveredNotifyList = {
>   (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>   &gEfiPeiMemoryDiscoveredPpiGuid,
>   InstallIplPermanentMemoryPpis
> };

(a) This variable has external linkage. From ISO C99, "6.2.2 Linkages of
identifiers":

> 5 [...] If the declaration of an identifier for an object has file
>   scope and no storage-class specifier, its linkage is external.

(b) This variable has static storage duration. From ISO C99, "6.2.4
Storage durations of objects":

> 3 An object whose identifier is declared with external or internal
>   linkage, or with the storage-class specifier static has static
>   storage duration. Its lifetime is the entire execution of the
>   program and its stored value is initialized only once, prior to
>   program startup.

(c) The quoted declaration is an external definition of
"mMemoryDiscoveredNotifyList". From ISO C99, "6.9.2 External object
definitions":

> 1 If the declaration of an identifier for an object has file scope and
>   an initializer, the declaration is an external definition for the
>   identifier.

The exact same statements can be made about
"SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c":

> GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR mMemoryDiscoveredNotifyList[1] = {
>   {
>     (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>     &gEfiPeiMemoryDiscoveredPpiGuid,
>     DebugAgentCallbackMemoryDiscoveredPpi
>   }
> };

(I see "GLOBAL_REMOVE_IF_UNREFERENCED", but with GCC, that expands to
nothing.)

So, we have two external definitions for "mMemoryDiscoveredNotifyList".
(It's especially funny because even their types don't match.)

This is what ISO C99, "6.9 External definitions" says:

> Syntax
>
> [...]
>
> Constraints
>
> [...]
>
> Semantics
>
> [...]
>
> 5 [...] If an identifier declared with external linkage is used in an
>   expression (other than as part of the operand of a sizeof operator
>   whose result is an integer constant), somewhere in the entire
>   program there shall be exactly one external definition for the
>   identifier [...]

Given that we use "mMemoryDiscoveredNotifyList" in two expressions,
namely:

* "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c":

>     Status = PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList);

* "SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c"

>     Status = PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList[0]);

the requirement definitely applies, and we're breaking it.


Now, the second question is, why don't some compilers complain about it?
The reason is that they don't have to (I missed this in my previous
email). This is what ISO C99, "5.1.1.3 Diagnostics" says:

> 1 A conforming implementation shall produce at least one diagnostic
>   message (identified in an implementation-defined manner) if a
>   preprocessing translation unit or translation unit contains a
>   violation of any syntax rule or constraint, even if the behavior is
>   also explicitly specified as undefined or implementation-defined.
>   Diagnostic messages need not be produced in other circumstances.
>   [...]

Note that when we break the requirement of "exactly one", we break
*Semantics*, not *Syntax* or *Constraints*. So the bug "only" causes
undefined behavior, and the compiler is not required to produce a
diagnostic message.

It's a good thing that the XCODE5 toolchain helped us catch this bug.

Thanks
Laszlo