[PATCH 0/3] MdePkg/DebugLib: Change the global variable name

[PATCH 0/3] MdePkg/DebugLib: Change the global variable name

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740

The DebugLib instances of DebugPortProtocol, ConOut and StdErr
use a global variable "mExitBootServicesEvent" which is in
conflict with the same variable in StatusCodeHandlerRuntimeDxe.inf.
That would cause a build error through GCC5. So change the
name to the "mDebugLibExitBootServicesEvent".

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>

Zhichao Gao (3):
  MdePkg/UefiDebugLibConOut: Change the global variable name
  MdePkg/UefiDebugLibStdErr: Change the global variable name
  MdePkg/UefiDebugLibDebugPortProtocol: Change the global variable name

 MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
 .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
 MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.21.0.windows.1

[PATCH 1/3] MdePkg/UefiDebugLibConOut: Change the global variable name

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740

The DebugLib instances of ConOut use a global variable
"mExitBootServicesEvent" which is in conflict with
the same variable in StatusCodeHandlerRuntimeDxe.inf.
That would cause a build error through GCC5. So change the
name to the "mDebugLibExitBootServicesEvent".

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c b/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
index d4fdfbab55..11b21cf0cc 100644
--- a/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
@@ -17,7 +17,7 @@
 //
 BOOLEAN     mPostEBS = FALSE;
 
-EFI_EVENT   mExitBootServicesEvent;
+EFI_EVENT   mDebugLibExitBootServicesEvent;
 
 //
 // Pointer to SystemTable
@@ -70,7 +70,7 @@ DxeDebugLibConstructor(
                                 ExitBootServicesCallback,
                                 NULL,
                                 &gEfiEventExitBootServicesGuid,
-                                &mExitBootServicesEvent
+                                &mDebugLibExitBootServicesEvent
                                 );
 
   return EFI_SUCCESS;
-- 
2.21.0.windows.1

[PATCH 2/3] MdePkg/UefiDebugLibStdErr: Change the global variable name

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740

The DebugLib instances of StdErr use a global variable
"mExitBootServicesEvent" which is in conflict with the
same variable in StatusCodeHandlerRuntimeDxe.inf.
That would cause a build error through GCC5. So change the
name to the "mDebugLibExitBootServicesEvent".

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
index d4fdfbab55..11b21cf0cc 100644
--- a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
@@ -17,7 +17,7 @@
 //
 BOOLEAN     mPostEBS = FALSE;
 
-EFI_EVENT   mExitBootServicesEvent;
+EFI_EVENT   mDebugLibExitBootServicesEvent;
 
 //
 // Pointer to SystemTable
@@ -70,7 +70,7 @@ DxeDebugLibConstructor(
                                 ExitBootServicesCallback,
                                 NULL,
                                 &gEfiEventExitBootServicesGuid,
-                                &mExitBootServicesEvent
+                                &mDebugLibExitBootServicesEvent
                                 );
 
   return EFI_SUCCESS;
-- 
2.21.0.windows.1

[PATCH 3/3] MdePkg/UefiDebugLibDebugPortProtocol: Change the global variable name

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740

The DebugLib instances of DebugPortProtocol use a global
variable "mExitBootServicesEvent" which is in conflict with
the same variable in StatusCodeHandlerRuntimeDxe.inf.
That would cause a build error through GCC5. So change the
name to the "mDebugLibExitBootServicesEvent".

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c b/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
index ed2cb70c21..f4c4a7d1e7 100644
--- a/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
@@ -17,7 +17,7 @@
 //
 BOOLEAN     mPostEBS = FALSE;
 
-EFI_EVENT   mExitBootServicesEvent;
+EFI_EVENT   mDebugLibExitBootServicesEvent;
 
 //
 // Pointer to SystemTable
@@ -70,7 +70,7 @@ DxeDebugLibConstructor(
               ExitBootServicesCallback,
               NULL,
               &gEfiEventExitBootServicesGuid,
-              &mExitBootServicesEvent
+              &mDebugLibExitBootServicesEvent
               );
 
   return EFI_SUCCESS;
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 0/3] MdePkg/DebugLib: Change the global variable name

[Liming Gao]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, Zhichao
> Sent: Tuesday, April 23, 2019 10:35 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: [edk2-devel] [PATCH 0/3] MdePkg/DebugLib: Change the global variable name
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> 
> The DebugLib instances of DebugPortProtocol, ConOut and StdErr
> use a global variable "mExitBootServicesEvent" which is in
> conflict with the same variable in StatusCodeHandlerRuntimeDxe.inf.
> That would cause a build error through GCC5. So change the
> name to the "mDebugLibExitBootServicesEvent".
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> 
> Zhichao Gao (3):
>   MdePkg/UefiDebugLibConOut: Change the global variable name
>   MdePkg/UefiDebugLibStdErr: Change the global variable name
>   MdePkg/UefiDebugLibDebugPortProtocol: Change the global variable name
> 
>  MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
>  .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
>  MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH 0/3] MdePkg/DebugLib: Change the global variable name

[Philippe Mathieu-Daudé]

On 4/23/19 4:35 AM, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> 
> The DebugLib instances of DebugPortProtocol, ConOut and StdErr
> use a global variable "mExitBootServicesEvent" which is in
> conflict with the same variable in StatusCodeHandlerRuntimeDxe.inf.
> That would cause a build error through GCC5. So change the
> name to the "mDebugLibExitBootServicesEvent".
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> 
> Zhichao Gao (3):
>   MdePkg/UefiDebugLibConOut: Change the global variable name
>   MdePkg/UefiDebugLibStdErr: Change the global variable name
>   MdePkg/UefiDebugLibDebugPortProtocol: Change the global variable name
> 
>  MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
>  .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
>  MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH 0/3] MdePkg/DebugLib: Change the global variable name

[Laszlo Ersek]

On 04/23/19 04:35, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> 
> The DebugLib instances of DebugPortProtocol, ConOut and StdErr
> use a global variable "mExitBootServicesEvent" which is in
> conflict with the same variable in StatusCodeHandlerRuntimeDxe.inf.
> That would cause a build error through GCC5. So change the
> name to the "mDebugLibExitBootServicesEvent".
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> 
> Zhichao Gao (3):
>   MdePkg/UefiDebugLibConOut: Change the global variable name
>   MdePkg/UefiDebugLibStdErr: Change the global variable name
>   MdePkg/UefiDebugLibDebugPortProtocol: Change the global variable name
> 
>  MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
>  .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
>  MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 

The proper solution for this kind of error is to make as many as possible instances of "mExitBootServicesEvent" in edk2 STATIC.

See for example commit 7b13510f2a0a ("MdeModulePkg/BootMaintenanceManagerUiLib: hide library-internal symbol", 2016-05-17).

In particular, this patch renames three instances of mExitBootServicesEvent, but there are more:

- IntelFrameworkModulePkg/Library/SmmRuntimeDxeReportStatusCodeLibFramework/SmmRuntimeDxeSupport.c
- IntelFrameworkModulePkg/Universal/StatusCode/DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c
- IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
- MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
- MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c

Based on a brief investigation, it seems like the "STATIC" approach should work for all 8 (eight) files above. But, minimally, STATIC should be employed with library instances.

I seem to remember that there used to be debugging issues with Visual Studio if global variables were made STATIC -- but I think that only applied to old (no longer supported by edk2?) Visual Studio versions. If you can't use STATIC here, please at least explain why, in the commit messages.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH 0/3] MdePkg/DebugLib: Change the global variable name

[Gao, Zhichao]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 23, 2019 10:08 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/3] MdePkg/DebugLib: Change the global
> variable name
> 
> On 04/23/19 04:35, Gao, Zhichao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> >
> > The DebugLib instances of DebugPortProtocol, ConOut and StdErr use a
> > global variable "mExitBootServicesEvent" which is in conflict with the
> > same variable in StatusCodeHandlerRuntimeDxe.inf.
> > That would cause a build error through GCC5. So change the name to the
> > "mDebugLibExitBootServicesEvent".
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> >
> > Zhichao Gao (3):
> >   MdePkg/UefiDebugLibConOut: Change the global variable name
> >   MdePkg/UefiDebugLibStdErr: Change the global variable name
> >   MdePkg/UefiDebugLibDebugPortProtocol: Change the global variable
> > name
> >
> >  MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
> >  .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
> >  MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> >
> 
> The proper solution for this kind of error is to make as many as possible
> instances of "mExitBootServicesEvent" in edk2 STATIC.
> 
> See for example commit 7b13510f2a0a
> ("MdeModulePkg/BootMaintenanceManagerUiLib: hide library-internal
> symbol", 2016-05-17).
> 
> In particular, this patch renames three instances of mExitBootServicesEvent,
> but there are more:
> 
> -
> IntelFrameworkModulePkg/Library/SmmRuntimeDxeReportStatusCodeLibFr
> amework/SmmRuntimeDxeSupport.c
> -
> IntelFrameworkModulePkg/Universal/StatusCode/DatahubStatusCodeHandl
> erDxe/DatahubStatusCodeHandlerDxe.c
> - IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> -
> MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/Firmw
> arePerformanceDxe.c
> -
> MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHan
> dlerRuntimeDxe.c
> 
> Based on a brief investigation, it seems like the "STATIC" approach should
> work for all 8 (eight) files above. But, minimally, STATIC should be employed
> with library instances.
> 
> I seem to remember that there used to be debugging issues with Visual
> Studio if global variables were made STATIC -- but I think that only applied to
> old (no longer supported by edk2?) Visual Studio versions. If you can't use
> STATIC here, please at least explain why, in the commit messages.

Thanks for your particular comments. I used to receive a comment to suggest me to make some global variable as 'STATIC'. But I consider the variable as the component scope that the variable may be used by the other files in the component. I forgot to consider the namespace collisions.
Agree with you, the variable such as 'mExitBootServicesEvent' is only used to create an event and close and it is useless for other files. All such variables should add a 'STATIC' description for them. I would update all the files your mentioned to decrease the namespace collisions.

Thanks,
Zhichao

> 
> Thanks,
> Laszlo