[PATCH V2 0/8] Decrease the name collisions

[PATCH V2 0/8] Decrease the name collisions

[Gao, Zhichao]

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

V1:
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".

V2:
Abandon v1.
Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
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 (8):
  MdePkg/UefiDebugLibConOut: Decrease the name collisions
  MdePkg/UefiDebugLibDebugPortProtocol: Decrease the name collisions
  MdePkg/UefiDebugLibStdErr: Decrease the name collisions
  IntelFrameworkModulePkg: Decrease the name collisions
  MdeModulePkg/FirmwarePerformanceDxe: Decrease the name collisions
  IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease the name collisions
  IntelFrameworkModulePkg: Decrease the name collisions
  MdeModulePkg/StatusCodeHandlerRuntimeDxe: Decrease the name collisions

 .../SmmRuntimeDxeSupport.c                                    | 2 +-
 .../DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c | 2 +-
 IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c | 2 +-
 .../FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c  | 2 +-
 .../RuntimeDxe/StatusCodeHandlerRuntimeDxe.c                  | 2 +-
 MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
 .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
 MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
 8 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.21.0.windows.1

[PATCH V2 1/8] MdePkg/UefiDebugLibConOut: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
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..bb7b144569 100644
--- a/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
@@ -15,9 +15,9 @@
 //
 // BOOLEAN value to indicate if it is at the post ExitBootServices pahse
 //
-BOOLEAN     mPostEBS = FALSE;
+BOOLEAN               mPostEBS = FALSE;
 
-EFI_EVENT   mExitBootServicesEvent;
+static EFI_EVENT      mExitBootServicesEvent;
 
 //
 // Pointer to SystemTable
-- 
2.21.0.windows.1

[PATCH V2 2/8] MdePkg/UefiDebugLibDebugPortProtocol: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
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..5d108288c9 100644
--- a/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
@@ -15,9 +15,9 @@
 //
 // BOOLEAN value to indicate if it is at the post ExitBootServices pahse
 //
-BOOLEAN     mPostEBS = FALSE;
+BOOLEAN               mPostEBS = FALSE;
 
-EFI_EVENT   mExitBootServicesEvent;
+static EFI_EVENT      mExitBootServicesEvent;
 
 //
 // Pointer to SystemTable
-- 
2.21.0.windows.1

[PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
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..bb7b144569 100644
--- a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
@@ -15,9 +15,9 @@
 //
 // BOOLEAN value to indicate if it is at the post ExitBootServices pahse
 //
-BOOLEAN     mPostEBS = FALSE;
+BOOLEAN               mPostEBS = FALSE;
 
-EFI_EVENT   mExitBootServicesEvent;
+static EFI_EVENT      mExitBootServicesEvent;
 
 //
 // Pointer to SystemTable
-- 
2.21.0.windows.1

[PATCH V2 4/8] IntelFrameworkModulePkg: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/IntelFrameworkModulePkg/Universal/StatusCode/DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c b/IntelFrameworkModulePkg/Universal/StatusCode/DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c
index 0f9185144d..5399bc7123 100644
--- a/IntelFrameworkModulePkg/Universal/StatusCode/DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c
+++ b/IntelFrameworkModulePkg/Universal/StatusCode/DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c
@@ -9,7 +9,7 @@
 
 #include "DatahubStatusCodeHandlerDxe.h"
 
-EFI_EVENT                 mExitBootServicesEvent     = NULL;
+static EFI_EVENT          mExitBootServicesEvent     = NULL;
 EFI_RSC_HANDLER_PROTOCOL  *mRscHandlerProtocol       = NULL;
 
 /**
-- 
2.21.0.windows.1

[PATCH V2 5/8] MdeModulePkg/FirmwarePerformanceDxe: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@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>
---
 .../FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c    | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
index 9713048f1f..85ca1e8d11 100644
--- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
+++ b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
@@ -40,7 +40,7 @@ EFI_RSC_HANDLER_PROTOCOL    *mRscHandlerProtocol = NULL;
 BOOLEAN                     mLockBoxReady = FALSE;
 EFI_EVENT                   mReadyToBootEvent;
 EFI_EVENT                   mLegacyBootEvent;
-EFI_EVENT                   mExitBootServicesEvent;
+static EFI_EVENT            mExitBootServicesEvent;
 UINTN                       mFirmwarePerformanceTableTemplateKey  = 0;
 BOOLEAN                     mDxeCoreReportStatusCodeEnable = FALSE;
 
-- 
2.21.0.windows.1

[PATCH V2 6/8] IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@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>
---
 IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
index fe344a5327..459acc694b 100644
--- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
+++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
@@ -39,7 +39,7 @@ extern EFI_GUID gAddPerfRecordProtocolGuid;
 extern EFI_GUID gFspHobGuid;
 extern EFI_GUID gFspApiPerformanceGuid;
 
-EFI_EVENT mExitBootServicesEvent     = NULL;
+static EFI_EVENT mExitBootServicesEvent     = NULL;
 
 /**
   Relocate this image under 4G memory.
-- 
2.21.0.windows.1

[PATCH V2 7/8] IntelFrameworkModulePkg: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../SmmRuntimeDxeSupport.c                                      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/IntelFrameworkModulePkg/Library/SmmRuntimeDxeReportStatusCodeLibFramework/SmmRuntimeDxeSupport.c b/IntelFrameworkModulePkg/Library/SmmRuntimeDxeReportStatusCodeLibFramework/SmmRuntimeDxeSupport.c
index 4a6137a509..e5db127b8a 100644
--- a/IntelFrameworkModulePkg/Library/SmmRuntimeDxeReportStatusCodeLibFramework/SmmRuntimeDxeSupport.c
+++ b/IntelFrameworkModulePkg/Library/SmmRuntimeDxeReportStatusCodeLibFramework/SmmRuntimeDxeSupport.c
@@ -9,7 +9,7 @@
 #include "ReportStatusCodeLibInternal.h"
 
 EFI_EVENT                     mVirtualAddressChangeEvent;
-EFI_EVENT                     mExitBootServicesEvent;
+static EFI_EVENT              mExitBootServicesEvent;
 EFI_STATUS_CODE_DATA          *mStatusCodeData;
 BOOLEAN                       mInSmm;
 EFI_SMM_BASE_PROTOCOL         *mSmmBase;
-- 
2.21.0.windows.1

[PATCH V2 8/8] MdeModulePkg/StatusCodeHandlerRuntimeDxe: Decrease the name collisions

[Gao, Zhichao]

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

Add a 'static' descriptor to the global variables that only
used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@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>
---
 .../StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c
index 0d327d40e3..e22dae5736 100644
--- a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c
+++ b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c
@@ -10,7 +10,7 @@
 #include "StatusCodeHandlerRuntimeDxe.h"
 
 EFI_EVENT                 mVirtualAddressChangeEvent = NULL;
-EFI_EVENT                 mExitBootServicesEvent     = NULL;
+static EFI_EVENT          mExitBootServicesEvent     = NULL;
 EFI_RSC_HANDLER_PROTOCOL  *mRscHandlerProtocol       = NULL;
 
 /**
-- 
2.21.0.windows.1

Re: [PATCH V2 6/8] IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease the name collisions

[Chiu, Chasel]

Reviewed-by: Chasel Chiu <chasel.chiu@intel.com>

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Wednesday, April 24, 2019 12:58 PM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Chiu, Chasel <chasel.chiu@intel.com>;
> Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: [PATCH V2 6/8] IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease
> the name collisions
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> 
> Add a 'static' descriptor to the global variables that only used in a single file to
> minimize the name collisions.
> This is only for the varable named 'mExitBootServicesEvent'.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@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>
> ---
>  IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git
> a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> index fe344a5327..459acc694b 100644
> --- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> +++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> @@ -39,7 +39,7 @@ extern EFI_GUID gAddPerfRecordProtocolGuid;  extern
> EFI_GUID gFspHobGuid;  extern EFI_GUID gFspApiPerformanceGuid;
> 
> -EFI_EVENT mExitBootServicesEvent     = NULL;
> +static EFI_EVENT mExitBootServicesEvent     = NULL;
> 
>  /**
>    Relocate this image under 4G memory.
> --
> 2.21.0.windows.1

Re: [PATCH V2 6/8] IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease the name collisions

[Nate DeSimone]

Update copyright year from 2016 to 2019. After that is done...

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Gao, Zhichao 
Sent: Tuesday, April 23, 2019 9:58 PM
To: devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: [PATCH V2 6/8] IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease the name collisions

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

Add a 'static' descriptor to the global variables that only used in a single file to minimize the name collisions.
This is only for the varable named 'mExitBootServicesEvent'.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@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>
---
 IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
index fe344a5327..459acc694b 100644
--- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
+++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
@@ -39,7 +39,7 @@ extern EFI_GUID gAddPerfRecordProtocolGuid;  extern EFI_GUID gFspHobGuid;  extern EFI_GUID gFspApiPerformanceGuid;
 
-EFI_EVENT mExitBootServicesEvent     = NULL;
+static EFI_EVENT mExitBootServicesEvent     = NULL;
 
 /**
   Relocate this image under 4G memory.
--
2.21.0.windows.1

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Ard Biesheuvel]

On Wed, 24 Apr 2019 at 06:59, Gao, Zhichao <zhichao.gao@intel.com> wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
>
> Add a 'static' descriptor to the global variables that only
> used in a single file to minimize the name collisions.
> This is only for the varable named 'mExitBootServicesEvent'.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 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..bb7b144569 100644
> --- a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> +++ b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> @@ -15,9 +15,9 @@
>  //
>  // BOOLEAN value to indicate if it is at the post ExitBootServices pahse
>  //
> -BOOLEAN     mPostEBS = FALSE;
> +BOOLEAN               mPostEBS = FALSE;
>
> -EFI_EVENT   mExitBootServicesEvent;
> +static EFI_EVENT      mExitBootServicesEvent;
>

Please use STATIC not static.


>  //
>  // Pointer to SystemTable
> --
> 2.21.0.windows.1
>
>
> 
>

Re: [PATCH V2 0/8] Decrease the name collisions

[Laszlo Ersek]

Hi Zhichao,

On 04/24/19 06:58, Zhichao Gao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> 
> V1:
> 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".
> 
> V2:
> Abandon v1.
> Add a 'static' descriptor to the global variables that only
> used in a single file to minimize the name collisions.
> This is only for the varable named 'mExitBootServicesEvent'.

I recommend a number of easy changes:

(1) in all of the subject lines, please replace

  Decrease the name collisions

with

  make mExitBootServicesEvent STATIC

(The longest resultant subject line,
"MdeModulePkg/StatusCodeHandlerRuntimeDxe: make mExitBootServicesEvent
STATIC", will be 76 chars wide, but I think it's acceptable in this case
-- there's simply no way to express what we do in fewer characters. I
have thought of "hide mExitBootServicesEvent" too, but "hide" is ambiguous.)

(2) In the commit messages, I suggest replacing "descriptor" with
"storage-class specifier". (That's the term ISO C uses.)

(3) Please spell it as "STATIC" in source code, not "static". For
whatever reason, the edk2 coding style requires STATIC. (See
"MdePkg/Include/Base.h".)

(4) In the first three patches (the DebugLib instances), there is a
blank line between "mPostEBS" and "mExitBootServicesEvent". For that
reason, I think it's unnecessary to change the indentation of
"mPostEBS", when you add STATIC to "mExitBootServicesEvent".


Having stated those, I totally defer to the respective package owners on
this series, as I don't co-maintain any of the affected packages. If
they disagree with the above, I'm OK too.

Thanks
Laszlo


> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 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 (8):
>   MdePkg/UefiDebugLibConOut: Decrease the name collisions
>   MdePkg/UefiDebugLibDebugPortProtocol: Decrease the name collisions
>   MdePkg/UefiDebugLibStdErr: Decrease the name collisions
>   IntelFrameworkModulePkg: Decrease the name collisions
>   MdeModulePkg/FirmwarePerformanceDxe: Decrease the name collisions
>   IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease the name collisions
>   IntelFrameworkModulePkg: Decrease the name collisions
>   MdeModulePkg/StatusCodeHandlerRuntimeDxe: Decrease the name collisions
> 
>  .../SmmRuntimeDxeSupport.c                                    | 2 +-
>  .../DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c | 2 +-
>  IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c | 2 +-
>  .../FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c  | 2 +-
>  .../RuntimeDxe/StatusCodeHandlerRuntimeDxe.c                  | 2 +-
>  MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
>  .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
>  MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
>  8 files changed, 11 insertions(+), 11 deletions(-)
> 

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Michael D Kinney]

Hi Ard,

I see a mix of use of 'static' and 'STATIC' in the sources.

The reason to use STATIC is if it needs to be replaced with
something other than 'static' for a specific compiler or
debug scenario.

Long ago, there were some source level debug issue with
'static' symbols, so using the macro STATIC was preferred
so it could be mapped to nothing for a debug scenario. That
issue no long exists.

Do you know of a reason today to map 'STATIC' to anything
Other than 'static'?

I also looked at the EDK II C Coding Standard.  It is also
inconsistent and had references to both 'static' and 'STATIC'.
We should enter a few BZs to update that doc once we have 
clear guidance from this discussion.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Ard Biesheuvel
> Sent: Wednesday, April 24, 2019 1:07 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>; Gao,
> Zhichao <zhichao.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; 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 V2 3/8]
> MdePkg/UefiDebugLibStdErr: Decrease the name collisions
> 
> On Wed, 24 Apr 2019 at 06:59, Gao, Zhichao
> <zhichao.gao@intel.com> wrote:
> >
> > REF:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> >
> > Add a 'static' descriptor to the global variables that
> only
> > used in a single file to minimize the name collisions.
> > This is only for the varable named
> 'mExitBootServicesEvent'.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > 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..bb7b144569 100644
> > ---
> a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > +++
> b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > @@ -15,9 +15,9 @@
> >  //
> >  // BOOLEAN value to indicate if it is at the post
> ExitBootServices pahse
> >  //
> > -BOOLEAN     mPostEBS = FALSE;
> > +BOOLEAN               mPostEBS = FALSE;
> >
> > -EFI_EVENT   mExitBootServicesEvent;
> > +static EFI_EVENT      mExitBootServicesEvent;
> >
> 
> Please use STATIC not static.
> 
> 
> >  //
> >  // Pointer to SystemTable
> > --
> > 2.21.0.windows.1
> >
> >
> >
> >
> 
> 

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Ard Biesheuvel]

On Wed, 24 Apr 2019 at 18:59, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> I see a mix of use of 'static' and 'STATIC' in the sources.
>
> The reason to use STATIC is if it needs to be replaced with
> something other than 'static' for a specific compiler or
> debug scenario.
>
> Long ago, there were some source level debug issue with
> 'static' symbols, so using the macro STATIC was preferred
> so it could be mapped to nothing for a debug scenario. That
> issue no long exists.
>
> Do you know of a reason today to map 'STATIC' to anything
> Other than 'static'?
>
> I also looked at the EDK II C Coding Standard.  It is also
> inconsistent and had references to both 'static' and 'STATIC'.
> We should enter a few BZs to update that doc once we have
> clear guidance from this discussion.
>

I wasn't aware that lower case static is also in use, so I was simply
extrapolating from personal experience.

i think there should be no reason to use the preprocessor to change
'static' into something else, and so I'm happy to settle on lowercase
static, as long as we are consistent. But more importantly, now that
this has come up, what I would like to do is make 'static' mandatory
unless the code requires otherwise: this results in much better code
generation (given that the compiler can infer constness for non-const
static variables but not for ones with external linkage) and generally
improves programmer hygiene when it comes to linking to arbitrary
symbols that are really internal to a library.


> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> > On Behalf Of Ard Biesheuvel
> > Sent: Wednesday, April 24, 2019 1:07 AM
> > To: edk2-devel-groups-io <devel@edk2.groups.io>; Gao,
> > Zhichao <zhichao.gao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>; 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 V2 3/8]
> > MdePkg/UefiDebugLibStdErr: Decrease the name collisions
> >
> > On Wed, 24 Apr 2019 at 06:59, Gao, Zhichao
> > <zhichao.gao@intel.com> wrote:
> > >
> > > REF:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> > >
> > > Add a 'static' descriptor to the global variables that
> > only
> > > used in a single file to minimize the name collisions.
> > > This is only for the varable named
> > 'mExitBootServicesEvent'.
> > >
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > 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..bb7b144569 100644
> > > ---
> > a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > > +++
> > b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > > @@ -15,9 +15,9 @@
> > >  //
> > >  // BOOLEAN value to indicate if it is at the post
> > ExitBootServices pahse
> > >  //
> > > -BOOLEAN     mPostEBS = FALSE;
> > > +BOOLEAN               mPostEBS = FALSE;
> > >
> > > -EFI_EVENT   mExitBootServicesEvent;
> > > +static EFI_EVENT      mExitBootServicesEvent;
> > >
> >
> > Please use STATIC not static.
> >
> >
> > >  //
> > >  // Pointer to SystemTable
> > > --
> > > 2.21.0.windows.1
> > >
> > >
> > >
> > >
> >
> > 
>

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Michael D Kinney]

Hi Ard,

I see no issues with using 'static' on more symbols.

I am not sure how to enforce it, so it would be a good
recommendation for code style and a good recommendation
for a code reviews.

Can you provide an example where code generation is 
improved when 'static' is used.  That would be good to
include with the recommendations and would help encourage
developers to follow the recommendation.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Ard Biesheuvel
> Sent: Wednesday, April 24, 2019 10:10 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [PATCH V2 3/8]
> MdePkg/UefiDebugLibStdErr: Decrease the name collisions
> 
> On Wed, 24 Apr 2019 at 18:59, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > Hi Ard,
> >
> > I see a mix of use of 'static' and 'STATIC' in the
> sources.
> >
> > The reason to use STATIC is if it needs to be replaced
> with
> > something other than 'static' for a specific compiler
> or
> > debug scenario.
> >
> > Long ago, there were some source level debug issue with
> > 'static' symbols, so using the macro STATIC was
> preferred
> > so it could be mapped to nothing for a debug scenario.
> That
> > issue no long exists.
> >
> > Do you know of a reason today to map 'STATIC' to
> anything
> > Other than 'static'?
> >
> > I also looked at the EDK II C Coding Standard.  It is
> also
> > inconsistent and had references to both 'static' and
> 'STATIC'.
> > We should enter a few BZs to update that doc once we
> have
> > clear guidance from this discussion.
> >
> 
> I wasn't aware that lower case static is also in use, so
> I was simply
> extrapolating from personal experience.
> 
> i think there should be no reason to use the preprocessor
> to change
> 'static' into something else, and so I'm happy to settle
> on lowercase
> static, as long as we are consistent. But more
> importantly, now that
> this has come up, what I would like to do is make
> 'static' mandatory
> unless the code requires otherwise: this results in much
> better code
> generation (given that the compiler can infer constness
> for non-const
> static variables but not for ones with external linkage)
> and generally
> improves programmer hygiene when it comes to linking to
> arbitrary
> symbols that are really internal to a library.
> 
> 
> > > -----Original Message-----
> > > From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io]
> > > On Behalf Of Ard Biesheuvel
> > > Sent: Wednesday, April 24, 2019 1:07 AM
> > > To: edk2-devel-groups-io <devel@edk2.groups.io>; Gao,
> > > Zhichao <zhichao.gao@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>; 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 V2 3/8]
> > > MdePkg/UefiDebugLibStdErr: Decrease the name
> collisions
> > >
> > > On Wed, 24 Apr 2019 at 06:59, Gao, Zhichao
> > > <zhichao.gao@intel.com> wrote:
> > > >
> > > > REF:
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> > > >
> > > > Add a 'static' descriptor to the global variables
> that
> > > only
> > > > used in a single file to minimize the name
> collisions.
> > > > This is only for the varable named
> > > 'mExitBootServicesEvent'.
> > > >
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > 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..bb7b144569 100644
> > > > ---
> > >
> a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > > > +++
> > >
> b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > > > @@ -15,9 +15,9 @@
> > > >  //
> > > >  // BOOLEAN value to indicate if it is at the post
> > > ExitBootServices pahse
> > > >  //
> > > > -BOOLEAN     mPostEBS = FALSE;
> > > > +BOOLEAN               mPostEBS = FALSE;
> > > >
> > > > -EFI_EVENT   mExitBootServicesEvent;
> > > > +static EFI_EVENT      mExitBootServicesEvent;
> > > >
> > >
> > > Please use STATIC not static.
> > >
> > >
> > > >  //
> > > >  // Pointer to SystemTable
> > > > --
> > > > 2.21.0.windows.1
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> >
> 
> 

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Ard Biesheuvel]

On Wed, 24 Apr 2019 at 19:31, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> I see no issues with using 'static' on more symbols.
>
> I am not sure how to enforce it, so it would be a good
> recommendation for code style and a good recommendation
> for a code reviews.
>

In Linux, it is one of the things enforced by the sparse tool: if no
declaration exists anywhere in a .h file, it warns about it, since it
means only the defining translation unit can refer to it.

> Can you provide an example where code generation is
> improved when 'static' is used.  That would be good to
> include with the recommendations and would help encourage
> developers to follow the recommendation.
>

Something like

STATIC UINT32 mVar = FixedPcdGet32 (FooCount);

VOID Func (VOID)
{
      UINT32 Index;

   for (Index = 0; Index < mVar; Index++) {
     ...
   }
}

If the compiler notices that there are no other assignments of mVar
(or can prove they are unreachable), it simply treats mVar as an
immediate constant, and removes the whole loop if mVar == 0. This is
only possible when using STATIC (or when using LTO, and so the lack of
STATIC may be one of the reasons LTO is so effective on our codebase)

The compiler will even track value ranges, i.e., if the only other
reachable assignment sets mVar to 1, it can take that into account in
the code generation as well.

As soon as the mVar symbol gets exported, the compiler must assume
that it can hold any value representable by the type, which defeats
many of these optimizations.


>
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> > On Behalf Of Ard Biesheuvel
> > Sent: Wednesday, April 24, 2019 10:10 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; Gao, Zhichao
> > <zhichao.gao@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>;
> > Bi, Dandan <dandan.bi@intel.com>
> > Subject: Re: [edk2-devel] [PATCH V2 3/8]
> > MdePkg/UefiDebugLibStdErr: Decrease the name collisions
> >
> > On Wed, 24 Apr 2019 at 18:59, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > I see a mix of use of 'static' and 'STATIC' in the
> > sources.
> > >
> > > The reason to use STATIC is if it needs to be replaced
> > with
> > > something other than 'static' for a specific compiler
> > or
> > > debug scenario.
> > >
> > > Long ago, there were some source level debug issue with
> > > 'static' symbols, so using the macro STATIC was
> > preferred
> > > so it could be mapped to nothing for a debug scenario.
> > That
> > > issue no long exists.
> > >
> > > Do you know of a reason today to map 'STATIC' to
> > anything
> > > Other than 'static'?
> > >
> > > I also looked at the EDK II C Coding Standard.  It is
> > also
> > > inconsistent and had references to both 'static' and
> > 'STATIC'.
> > > We should enter a few BZs to update that doc once we
> > have
> > > clear guidance from this discussion.
> > >
> >
> > I wasn't aware that lower case static is also in use, so
> > I was simply
> > extrapolating from personal experience.
> >
> > i think there should be no reason to use the preprocessor
> > to change
> > 'static' into something else, and so I'm happy to settle
> > on lowercase
> > static, as long as we are consistent. But more
> > importantly, now that
> > this has come up, what I would like to do is make
> > 'static' mandatory
> > unless the code requires otherwise: this results in much
> > better code
> > generation (given that the compiler can infer constness
> > for non-const
> > static variables but not for ones with external linkage)
> > and generally
> > improves programmer hygiene when it comes to linking to
> > arbitrary
> > symbols that are really internal to a library.
> >
> >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io]
> > > > On Behalf Of Ard Biesheuvel
> > > > Sent: Wednesday, April 24, 2019 1:07 AM
> > > > To: edk2-devel-groups-io <devel@edk2.groups.io>; Gao,
> > > > Zhichao <zhichao.gao@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>; 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 V2 3/8]
> > > > MdePkg/UefiDebugLibStdErr: Decrease the name
> > collisions
> > > >
> > > > On Wed, 24 Apr 2019 at 06:59, Gao, Zhichao
> > > > <zhichao.gao@intel.com> wrote:
> > > > >
> > > > > REF:
> > > > https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> > > > >
> > > > > Add a 'static' descriptor to the global variables
> > that
> > > > only
> > > > > used in a single file to minimize the name
> > collisions.
> > > > > This is only for the varable named
> > > > 'mExitBootServicesEvent'.
> > > > >
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > 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..bb7b144569 100644
> > > > > ---
> > > >
> > a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > > > > +++
> > > >
> > b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
> > > > > @@ -15,9 +15,9 @@
> > > > >  //
> > > > >  // BOOLEAN value to indicate if it is at the post
> > > > ExitBootServices pahse
> > > > >  //
> > > > > -BOOLEAN     mPostEBS = FALSE;
> > > > > +BOOLEAN               mPostEBS = FALSE;
> > > > >
> > > > > -EFI_EVENT   mExitBootServicesEvent;
> > > > > +static EFI_EVENT      mExitBootServicesEvent;
> > > > >
> > > >
> > > > Please use STATIC not static.
> > > >
> > > >
> > > > >  //
> > > > >  // Pointer to SystemTable
> > > > > --
> > > > > 2.21.0.windows.1
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > >
> >
> > 
>

Re: [PATCH V2 0/8] Decrease the name collisions

[Gao, Zhichao]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, April 24, 2019 7:16 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [PATCH V2 0/8] Decrease the name collisions
> 
> Hi Zhichao,
> 
> On 04/24/19 06:58, Zhichao Gao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1740
> >
> > V1:
> > 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".
> >
> > V2:
> > Abandon v1.
> > Add a 'static' descriptor to the global variables that only used in a
> > single file to minimize the name collisions.
> > This is only for the varable named 'mExitBootServicesEvent'.
> 
> I recommend a number of easy changes:
> 
> (1) in all of the subject lines, please replace
> 
>   Decrease the name collisions
> 
> with
> 
>   make mExitBootServicesEvent STATIC
> 
> (The longest resultant subject line,
> "MdeModulePkg/StatusCodeHandlerRuntimeDxe: make
> mExitBootServicesEvent STATIC", will be 76 chars wide, but I think it's
> acceptable in this case
> -- there's simply no way to express what we do in fewer characters. I have
> thought of "hide mExitBootServicesEvent" too, but "hide" is ambiguous.)

Agree. That would be more specific for this patch.

> 
> (2) In the commit messages, I suggest replacing "descriptor" with "storage-
> class specifier". (That's the term ISO C uses.)

Agree.

> 
> (3) Please spell it as "STATIC" in source code, not "static". For whatever
> reason, the edk2 coding style requires STATIC. (See
> "MdePkg/Include/Base.h".)

Depend on the discuss between Mike and Ard. I'd like to keep using the static. Or wait for someone else have different opinions.

> 
> (4) In the first three patches (the DebugLib instances), there is a blank line
> between "mPostEBS" and "mExitBootServicesEvent". For that reason, I think
> it's unnecessary to change the indentation of "mPostEBS", when you add
> STATIC to "mExitBootServicesEvent".
> 

That my personal habit to make the variables aligned if they are closed. Seems this is an unnecessary change.

Thanks,
Zhichao

> 
> Having stated those, I totally defer to the respective package owners on this
> series, as I don't co-maintain any of the affected packages. If they disagree
> with the above, I'm OK too.
> 
> Thanks
> Laszlo
> 
> 
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > 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 (8):
> >   MdePkg/UefiDebugLibConOut: Decrease the name collisions
> >   MdePkg/UefiDebugLibDebugPortProtocol: Decrease the name collisions
> >   MdePkg/UefiDebugLibStdErr: Decrease the name collisions
> >   IntelFrameworkModulePkg: Decrease the name collisions
> >   MdeModulePkg/FirmwarePerformanceDxe: Decrease the name collisions
> >   IntelFsp2WrapperPkg/FspWrapperNotifyDxe: Decrease the name
> collisions
> >   IntelFrameworkModulePkg: Decrease the name collisions
> >   MdeModulePkg/StatusCodeHandlerRuntimeDxe: Decrease the name
> > collisions
> >
> >  .../SmmRuntimeDxeSupport.c                                    | 2 +-
> >  .../DatahubStatusCodeHandlerDxe/DatahubStatusCodeHandlerDxe.c | 2
> +-
> > IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c | 2
> +-
> > .../FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c  | 2 +-
> >  .../RuntimeDxe/StatusCodeHandlerRuntimeDxe.c                  | 2 +-
> >  MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c       | 4 ++--
> >  .../UefiDebugLibDebugPortProtocol/DebugLibConstructor.c       | 4 ++--
> >  MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c       | 4 ++--
> >  8 files changed, 11 insertions(+), 11 deletions(-)
> >

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Laszlo Ersek]

On 04/24/19 19:09, Ard Biesheuvel wrote:
> On Wed, 24 Apr 2019 at 18:59, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
>>
>> Hi Ard,
>>
>> I see a mix of use of 'static' and 'STATIC' in the sources.
>>
>> The reason to use STATIC is if it needs to be replaced with
>> something other than 'static' for a specific compiler or
>> debug scenario.
>>
>> Long ago, there were some source level debug issue with
>> 'static' symbols, so using the macro STATIC was preferred
>> so it could be mapped to nothing for a debug scenario. That
>> issue no long exists.
>>
>> Do you know of a reason today to map 'STATIC' to anything
>> Other than 'static'?
>>
>> I also looked at the EDK II C Coding Standard.  It is also
>> inconsistent and had references to both 'static' and 'STATIC'.
>> We should enter a few BZs to update that doc once we have
>> clear guidance from this discussion.
>>
> 
> I wasn't aware that lower case static is also in use, so I was simply
> extrapolating from personal experience.

+1

> i think there should be no reason to use the preprocessor to change
> 'static' into something else, and so I'm happy to settle on lowercase
> static, as long as we are consistent.

+1 (same for CONST / const)

> But more importantly, now that
> this has come up, what I would like to do is make 'static' mandatory
> unless the code requires otherwise: this results in much better code
> generation (given that the compiler can infer constness for non-const
> static variables but not for ones with external linkage) and generally
> improves programmer hygiene when it comes to linking to arbitrary
> symbols that are really internal to a library.

I agree; for new code, giving internal linkage to as many as possible
objects that have static storage duration is the way to go. ("Make as
many globals 'static' as you can.")

Thanks
Laszlo

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Michael D Kinney]

Hi Laszlo,

I have entered 2 BZ for the STATIC -> static conversion.
one for the EDK II C Coding Standard Specification and 
one for the C code in EDK II.

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

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

Converting all CONST -> const will require additional 
discussion because the UEFI Specification defines the 
keyword 'CONST' in Section 2.3.1 - Data Types and is
consistently used in APIs defined in the UEFI/PI 
Specifications.

Best regards,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, April 26, 2019 9:49 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: Re: [edk2-devel] [PATCH V2 3/8]
> MdePkg/UefiDebugLibStdErr: Decrease the name collisions
> 
> On 04/24/19 19:09, Ard Biesheuvel wrote:
> > On Wed, 24 Apr 2019 at 18:59, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> >>
> >> Hi Ard,
> >>
> >> I see a mix of use of 'static' and 'STATIC' in the
> sources.
> >>
> >> The reason to use STATIC is if it needs to be
> replaced with
> >> something other than 'static' for a specific compiler
> or
> >> debug scenario.
> >>
> >> Long ago, there were some source level debug issue
> with
> >> 'static' symbols, so using the macro STATIC was
> preferred
> >> so it could be mapped to nothing for a debug
> scenario. That
> >> issue no long exists.
> >>
> >> Do you know of a reason today to map 'STATIC' to
> anything
> >> Other than 'static'?
> >>
> >> I also looked at the EDK II C Coding Standard.  It is
> also
> >> inconsistent and had references to both 'static' and
> 'STATIC'.
> >> We should enter a few BZs to update that doc once we
> have
> >> clear guidance from this discussion.
> >>
> >
> > I wasn't aware that lower case static is also in use,
> so I was simply
> > extrapolating from personal experience.
> 
> +1
> 
> > i think there should be no reason to use the
> preprocessor to change
> > 'static' into something else, and so I'm happy to
> settle on lowercase
> > static, as long as we are consistent.
> 
> +1 (same for CONST / const)
> 
> > But more importantly, now that
> > this has come up, what I would like to do is make
> 'static' mandatory
> > unless the code requires otherwise: this results in
> much better code
> > generation (given that the compiler can infer
> constness for non-const
> > static variables but not for ones with external
> linkage) and generally
> > improves programmer hygiene when it comes to linking
> to arbitrary
> > symbols that are really internal to a library.
> 
> I agree; for new code, giving internal linkage to as
> many as possible
> objects that have static storage duration is the way to
> go. ("Make as
> many globals 'static' as you can.")
> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Laszlo Ersek]

On 04/29/19 18:40, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> I have entered 2 BZ for the STATIC -> static conversion.
> one for the EDK II C Coding Standard Specification and 
> one for the C code in EDK II.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1766
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1767
> 
> Converting all CONST -> const will require additional 
> discussion because the UEFI Specification defines the 
> keyword 'CONST' in Section 2.3.1 - Data Types and is
> consistently used in APIs defined in the UEFI/PI 
> Specifications.

Good point, thanks!
Laszlo

> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, April 26, 2019 9:49 AM
>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney,
>> Michael D <michael.d.kinney@intel.com>
>> Cc: devel@edk2.groups.io; Gao, Zhichao
>> <zhichao.gao@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>> Subject: Re: [edk2-devel] [PATCH V2 3/8]
>> MdePkg/UefiDebugLibStdErr: Decrease the name collisions
>>
>> On 04/24/19 19:09, Ard Biesheuvel wrote:
>>> On Wed, 24 Apr 2019 at 18:59, Kinney, Michael D
>>> <michael.d.kinney@intel.com> wrote:
>>>>
>>>> Hi Ard,
>>>>
>>>> I see a mix of use of 'static' and 'STATIC' in the
>> sources.
>>>>
>>>> The reason to use STATIC is if it needs to be
>> replaced with
>>>> something other than 'static' for a specific compiler
>> or
>>>> debug scenario.
>>>>
>>>> Long ago, there were some source level debug issue
>> with
>>>> 'static' symbols, so using the macro STATIC was
>> preferred
>>>> so it could be mapped to nothing for a debug
>> scenario. That
>>>> issue no long exists.
>>>>
>>>> Do you know of a reason today to map 'STATIC' to
>> anything
>>>> Other than 'static'?
>>>>
>>>> I also looked at the EDK II C Coding Standard.  It is
>> also
>>>> inconsistent and had references to both 'static' and
>> 'STATIC'.
>>>> We should enter a few BZs to update that doc once we
>> have
>>>> clear guidance from this discussion.
>>>>
>>>
>>> I wasn't aware that lower case static is also in use,
>> so I was simply
>>> extrapolating from personal experience.
>>
>> +1
>>
>>> i think there should be no reason to use the
>> preprocessor to change
>>> 'static' into something else, and so I'm happy to
>> settle on lowercase
>>> static, as long as we are consistent.
>>
>> +1 (same for CONST / const)
>>
>>> But more importantly, now that
>>> this has come up, what I would like to do is make
>> 'static' mandatory
>>> unless the code requires otherwise: this results in
>> much better code
>>> generation (given that the compiler can infer
>> constness for non-const
>>> static variables but not for ones with external
>> linkage) and generally
>>> improves programmer hygiene when it comes to linking
>> to arbitrary
>>> symbols that are really internal to a library.
>>
>> I agree; for new code, giving internal linkage to as
>> many as possible
>> objects that have static storage duration is the way to
>> go. ("Make as
>> many globals 'static' as you can.")
>>
>> Thanks
>> Laszlo

Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions

[Leif Lindholm]

(Ancient thread, responding due to finding this when looking at recent
BZ activity.)

On Wed, Apr 24, 2019 at 05:31:36PM +0000, Michael D Kinney wrote:
> I see no issues with using 'static' on more symbols.
> 
> I am not sure how to enforce it, so it would be a good
> recommendation for code style and a good recommendation
> for a code reviews.

For GCC family, -Wmissing-prototypes is a pretty good way of catching
at least functions that should be static. Adding that to default build
flags would be a way of preventing new such code being added ... after
we had gone through and made sure all current code built with it...

Regards,

Leif