From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::243; helo=mail-it0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B14B321B0283A for ; Thu, 7 Dec 2017 03:36:28 -0800 (PST) Received: by mail-it0-x243.google.com with SMTP id f190so13636967ita.5 for ; Thu, 07 Dec 2017 03:41:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=fejf5LDaC/USbeNSn4NpZEJZGNIPMKfWfyhyVkPtKFQ=; b=UHKAmympCWQ5po4PufE+83yoXww7TNhnAlHHI7CmYJyWGhIaIT+zdV0CJZx6/yaZ4e JDkJsm830wwDqP4jtuuHHw0eHN+/4+/DBbRYY8WnJ2KQjqebUKC2nQSlmYgEMvCOokH4 76vOMC39sD1ADDErpilxKfMFTcCq8/RlZDaj8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=fejf5LDaC/USbeNSn4NpZEJZGNIPMKfWfyhyVkPtKFQ=; b=PUtK91CNukfhu7jXFlOojX0m5gTb4NgurgbgGmYdjf2LEflFrav5sII2bm/ByxWDK2 msqBj7ke5+ZH/LjUsVKL3XeFdEmxmRZiHaD1z3hmDhLeA/4TiLZNEYibZLAyDje+4dAD InO6ggR6HjsPDbcQCHJWcSb98aJuUpG3hoTUssbTJUdJysC74oiQbd6iv1TsvYnbZISG V9i2bPku4XE49beNsrZTcyfBZbLXI4A4wTAbNmGngU6nWaoNe4BVeZyDJvamJ4aCc8hW T+007nVclLKztZPqpiEG4GHlNQp0RnmYSPpjFkunHSJ+egDN5JmPihhgCiTgPIIiptRn B78Q== X-Gm-Message-State: AKGB3mLFe+8Sc3jx4dP14q7DTNFaGTmCYx7I+KX+IEBFVjzrhYhib6WV W98RX19erggmVLjaXp37CbknCucK/096dErzzDlfZ5/U X-Google-Smtp-Source: AGs4zMYFLwDsjz6V21UxbPQVUtXC28ET3GFP4qxLos1Z+0Ze1N39Ynoygh+l8C1scvSc5LE3+cGOx3sehJGpryBcooU= X-Received: by 10.36.145.203 with SMTP id i194mr918645ite.73.1512646860942; Thu, 07 Dec 2017 03:41:00 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Thu, 7 Dec 2017 03:41:00 -0800 (PST) In-Reply-To: <5b49394f-cb68-c00a-6174-a14a619e02c5@redhat.com> References: <1512632891-5236-1-git-send-email-liming.gao@intel.com> <5b49394f-cb68-c00a-6174-a14a619e02c5@redhat.com> From: Ard Biesheuvel Date: Thu, 7 Dec 2017 11:41:00 +0000 Message-ID: To: Laszlo Ersek Cc: Liming Gao , "edk2-devel@lists.01.org" , Michael Kinney , Hao Wu , Andrew Fish , Jeff Fan Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Dec 2017 11:36:28 -0000 Content-Type: text/plain; charset="UTF-8" On 7 December 2017 at 11:18, Laszlo Ersek wrote: > On 12/07/17 09:46, Ard Biesheuvel wrote: >> On 7 December 2017 at 07:48, Liming Gao wrote: >>> From: Michael Kinney >>> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=573 >>> https://bugzilla.tianocore.org/show_bug.cgi?id=796 >>> >>> The same issue is reported again by GCC. Resend this patch again. >>> This patch renames the duplicated function name to fix it. >>> >>> The SecPeiDebugAgentLib uses the global variable >>> mMemoryDiscoveredNotifyList for a PPI notification on >>> the Memory Discovered PPI. This same variable name is >>> used in the DxeIplPeim for the same PPI notification. >>> >>> The XCODE5 tool chain detects this duplicate symbol >>> when the OVMF platform is built with the flag >>> -D SOURCE_DEBUG_ENABLE. >>> >>> The fix is to rename this global variable in the >>> SecPeiDebugAgentLib library. >>> >> >> No, the fix is to make it STATIC unless it *needs* to be a global. >> Is that the case here? > > I agree with you (of course), but Mike explained earlier (if I recall > correctly -- and perhaps you remember too) that giving internal linkage > to global variables (i.e., making them STATIC) messes either with > debuggability under VS, or else defeats "GLOBAL_REMOVE_IF_UNREFERENCED". > (I'm not sure which one of the two.) > That doesn't quite ring a bell, but if that is the case, it deserves a mention. Note that STATIC variables are also removed when unreferenced (but may require a warning override like we have for GCC if it is only used from DEBUG () code). In any case, polluting the global namespace in a heterogeneous project like EDK2 is something that should only be done with good reason IMO. > So, I've settled on considering "extern by default" just another > peculiarity of edk2. *shrug* I'm just glad -fno-common catches bugs like > this! > Well, the thing is, external linkage defeats optimizations in the compiler, and also prevents it from emitting the variable into a read-only section even if it would otherwise be able to infer from the usage that the variable is never modified. > Reviewed-by: Laszlo Ersek > > (Obviously I'm not trying to dismiss your objection at all! Just stating > my view. If the patch is changed to STATIC, I'll R-b that version *more > happily* than this one.) > > Thanks! > Laszlo > >> >> >>> Cc: Andrew Fish >>> Cc: Jeff Fan >>> Cc: Hao Wu >>> Cc: Laszlo Ersek >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Michael D Kinney >>> Reviewed-by: Jeff Fan >>> --- >>> .../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c >>> index b717e33..9f5223a 100644 >>> --- a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c >>> +++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c >>> @@ -32,7 +32,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPTOR mVectorHandoffInf >>> } >>> }; >>> >>> -GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR mMemoryDiscoveredNotifyList[1] = { >>> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR mDebugAgentMemoryDiscoveredNotifyList[1] = { >>> { >>> (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), >>> &gEfiPeiMemoryDiscoveredPpiGuid, >>> @@ -554,7 +554,7 @@ InitializeDebugAgent ( >>> // Register for a callback once memory has been initialized. >>> // If memery has been ready, the callback funtion will be invoked immediately >>> // >>> - Status = PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList[0]); >>> + Status = PeiServicesNotifyPpi (&mDebugAgentMemoryDiscoveredNotifyList[0]); >>> if (EFI_ERROR (Status)) { >>> DEBUG ((EFI_D_ERROR, "DebugAgent: Failed to register memory discovered callback function!\n")); >>> CpuDeadLoop (); >>> -- >>> 2.6.3.windows.1 >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> GitPatchExtractor 1.1 >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >