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::22b; helo=mail-it0-x22b.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22b.google.com (mail-it0-x22b.google.com [IPv6:2607:f8b0:4001:c0b::22b]) (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 1B82221B0284B for ; Thu, 7 Dec 2017 06:17:24 -0800 (PST) Received: by mail-it0-x22b.google.com with SMTP id f143so14711869itb.0 for ; Thu, 07 Dec 2017 06:21:58 -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=6oN+c0fjuYlVSkSlmMOW8IqHSEGUGttVhA9D6K9zRd8=; b=AXI7Qx7iAu9fLBLvCgIORe+zQ9eYyzxtubjwuSDOaXSKoTOxlcDdQU5zm2uhucsFo0 FrZNysdjtCNe73crIlGtq/L0/VjeuM1ve2tzF+zmdqfXagBTNVxfBdF32GKgMyKE9SWM ppHd4vDuha6otBkmV65IU31UsM8JE1SB+CXUg= 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=6oN+c0fjuYlVSkSlmMOW8IqHSEGUGttVhA9D6K9zRd8=; b=PcH5dJ6AeQ6ctwOkv/xB0KTYmag5zn/Te6LPP6EoUbpOerxAu4V/V9lc91e7R07qld DVsCGQd6Xs254ougZWMebsubkrAunaoJauCOHwS5t4ga8cF1FvnOy0jzaTGIyZUkiODZ tiXZ0YR2ZgO3hZyRpD4ZqK5K/+RpCAeJS+qimxJQ8a4nXDe6AYV3wuGZiYdeE1+Cs948 msMV97qPSFtpt0o7Ukxwc4wsIE3b83WeMQ4XILMPBz8Q+UcqzHlNorF7F0nnVf7w/sFJ bqGJMij3M/1mqFX5jmXKUyVm3v+zVY88cUjBK8gehuvgEM1ECvYwYJh+ohkflsyx+iVt eF8w== X-Gm-Message-State: AKGB3mIBrpDi6Ba6vrPh1L7kD5fIoIRvtU0DPn97Bxsy/stZtQmeKNEG 4i8g5ZsZAW1zQClEWfXpgnALEqlKoaXiN85Mr8X/6w== X-Google-Smtp-Source: AGs4zMbPX3xFn+x4N4EfLNrA0ijeT4boXa+dNyvlDIFtbgKGbLkeAfe9DnEZckN8+DFb3/gmfUJzcdFC63FBVEbAI/E= X-Received: by 10.36.55.138 with SMTP id r132mr1499051itr.34.1512656517620; Thu, 07 Dec 2017 06:21:57 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Thu, 7 Dec 2017 06:21:57 -0800 (PST) In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E18C020@SHSMSX104.ccr.corp.intel.com> References: <1512632891-5236-1-git-send-email-liming.gao@intel.com> <5b49394f-cb68-c00a-6174-a14a619e02c5@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E18C020@SHSMSX104.ccr.corp.intel.com> From: Ard Biesheuvel Date: Thu, 7 Dec 2017 14:21:57 +0000 Message-ID: To: "Gao, Liming" Cc: Laszlo Ersek , "edk2-devel@lists.01.org" , "Kinney, Michael D" , "Wu, Hao A" , Andrew Fish 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 14:17:25 -0000 Content-Type: text/plain; charset="UTF-8" On 7 December 2017 at 14:19, Gao, Liming wrote: > Ard and Ersek: > On VS, static may make debug become hard. And, STATIC + GLOBAL_REMOVE_IF_UNREFERENCED may failure on old VS before VS supports /Gw option. So, I don't add static here. > I would like to separate static usage topic. We could discuss more and summary the rule on how to use static in code. But for this build failure issue, I prefer to fix it with this solution first. Is it OK to you? > Yes that is fine. -- Ard. >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Thursday, December 7, 2017 7:41 PM >> To: Laszlo Ersek >> Cc: Gao, Liming ; edk2-devel@lists.01.org; Kinney, Michael D ; Wu, Hao A >> ; Andrew Fish ; Jeff Fan >> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol >> >> 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 >> >