[edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Gavin Xue]

Different symbol (PROCESSOR_BIND_H__) define in RISCV64 ProcessorBinding.h
from other CPU Arch. An unexception compilation error generated
if include __PROCESSOR_BIND_H__ symbol in header file for
cross-platform compiling.

Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Cc: Yimin Wang <yimin.wang@intel.com>
Cc: Alan Sheng <alan.sheng@intel.com>
Signed-off-by: Gavin Xue <gavin.xue@intel.com>
---
 MdePkg/Include/RiscV64/ProcessorBind.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/RiscV64/ProcessorBind.h b/MdePkg/Include/RiscV64/ProcessorBind.h
index 1d42d92de4..7f24e77b8c 100644
--- a/MdePkg/Include/RiscV64/ProcessorBind.h
+++ b/MdePkg/Include/RiscV64/ProcessorBind.h
@@ -2,13 +2,14 @@
   Processor or Compiler specific defines and types for RISC-V
 
   Copyright (c) 2016 - 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
+  Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
-#ifndef PROCESSOR_BIND_H__
-#define PROCESSOR_BIND_H__
+#ifndef __PROCESSOR_BIND_H__
+#define __PROCESSOR_BIND_H__
 
 ///
 /// Define the processor type so other code can make processor based choices
-- 
2.33.0.windows.1

Re: [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Sunil V L]

On Fri, Jun 16, 2023 at 03:22:57PM +0800, Gavin Xue wrote:
> Different symbol (PROCESSOR_BIND_H__) define in RISCV64 ProcessorBinding.h
> from other CPU Arch. An unexception compilation error generated
> if include __PROCESSOR_BIND_H__ symbol in header file for
> cross-platform compiling.
> 
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Yimin Wang <yimin.wang@intel.com>
> Cc: Alan Sheng <alan.sheng@intel.com>
> Signed-off-by: Gavin Xue <gavin.xue@intel.com>
> ---
>  MdePkg/Include/RiscV64/ProcessorBind.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/RiscV64/ProcessorBind.h b/MdePkg/Include/RiscV64/ProcessorBind.h
> index 1d42d92de4..7f24e77b8c 100644
> --- a/MdePkg/Include/RiscV64/ProcessorBind.h
> +++ b/MdePkg/Include/RiscV64/ProcessorBind.h
> @@ -2,13 +2,14 @@
>    Processor or Compiler specific defines and types for RISC-V
>  
>    Copyright (c) 2016 - 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +  Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
>  
> -#ifndef PROCESSOR_BIND_H__
> -#define PROCESSOR_BIND_H__
> +#ifndef __PROCESSOR_BIND_H__
> +#define __PROCESSOR_BIND_H__
>  
As per EDK2 coding standards, the include guard names should not
start with _. So, I guess you need to fix the header files of other
architectures which are using _ in their include guard.

Thanks,
Sunil

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Pedro Falcato]

On Fri, Jun 16, 2023 at 11:36 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Fri, Jun 16, 2023 at 03:22:57PM +0800, Gavin Xue wrote:
> > Different symbol (PROCESSOR_BIND_H__) define in RISCV64 ProcessorBinding.h
> > from other CPU Arch. An unexception compilation error generated
> > if include __PROCESSOR_BIND_H__ symbol in header file for
> > cross-platform compiling.

(replying through Sunil's reply as the original email doesn't seem to
have been sent to the list)

What exactly is the problem here? The header has its include guard,
you include it once, it defines PROCESSOR_BIND_H__, you include it
twice, the #ifndef doesn't pass and nothing gets "included" again.
I really don't understand what your problem is, here. Particularly the
__PROCESSOR_BIND_H__ inclusion bit, this all seems like a giant hack.

-- 
Pedro

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Xue, Gavin]

Hi Sunil/Pedro,

1. As you know, ProcessorBind.h file of CPU Architecture file declares sets of base types for edk2 code compiling.
So data type in edk2 code doesn't rely on specific compiler (msvc, gcc etc.), which is a good design.

But in practice, for the purpose of reuse, some code can be built with edk2, and also can be built to a standalone application (e.g. Win App).
Just like below code piece:
===========
#ifndef __WRAPPER_BASE_TYPES_H__
#define __WRAPPER_BASE_TYPES_H__

//
// To avoid definition conflict during EDK2 build, it must include
// ProcessorBind.h before xxx.h
//
#ifndef __PROCESSOR_BIND_H__

#include <stdint.h>
typedef uint8_t  UINT8;
==========

In this case, if this is a edk2 build, the code will refer to data types from ProcessorBind.h, otherwise, it will refer to stdint.h from compiler.

2. Regarding the guard name, it's same __PROCESSOR_BIND_H__ macro in AArch64/Arm/Ebc/Ia32/X64, but it is PROCESSOR_BIND_H_
in RiscV64 and LoongArh64. For above code, if we build BIOS for RISCV64, it will try to include stdint.h due to different guard name.

I am not sure if we can use same guard name to keep code alignment, or give some comments. Thanks.

Best regards,
Gavin

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Friday, June 16, 2023 10:12 PM
To: devel@edk2.groups.io; sunilvl@ventanamicro.com
Cc: Xue, Gavin <gavin.xue@intel.com>; Warkentin, Andrei <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng, Alan <alan.sheng@intel.com>
Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

On Fri, Jun 16, 2023 at 11:36 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Fri, Jun 16, 2023 at 03:22:57PM +0800, Gavin Xue wrote:
> > Different symbol (PROCESSOR_BIND_H__) define in RISCV64 
> > ProcessorBinding.h from other CPU Arch. An unexception compilation 
> > error generated if include __PROCESSOR_BIND_H__ symbol in header 
> > file for cross-platform compiling.

(replying through Sunil's reply as the original email doesn't seem to have been sent to the list)

What exactly is the problem here? The header has its include guard, you include it once, it defines PROCESSOR_BIND_H__, you include it twice, the #ifndef doesn't pass and nothing gets "included" again.
I really don't understand what your problem is, here. Particularly the __PROCESSOR_BIND_H__ inclusion bit, this all seems like a giant hack.

--
Pedro

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Pedro Falcato]

On Fri, Jun 16, 2023 at 4:52 PM Xue, Gavin <gavin.xue@intel.com> wrote:
>
> Hi Sunil/Pedro,
>
> 1. As you know, ProcessorBind.h file of CPU Architecture file declares sets of base types for edk2 code compiling.
> So data type in edk2 code doesn't rely on specific compiler (msvc, gcc etc.), which is a good design.
>
> But in practice, for the purpose of reuse, some code can be built with edk2, and also can be built to a standalone application (e.g. Win App).
> Just like below code piece:
> ===========
> #ifndef __WRAPPER_BASE_TYPES_H__
> #define __WRAPPER_BASE_TYPES_H__
>
> //
> // To avoid definition conflict during EDK2 build, it must include
> // ProcessorBind.h before xxx.h
> //
> #ifndef __PROCESSOR_BIND_H__
>
> #include <stdint.h>
> typedef uint8_t  UINT8;
> ==========
>
> In this case, if this is a edk2 build, the code will refer to data types from ProcessorBind.h, otherwise, it will refer to stdint.h from compiler.
>
> 2. Regarding the guard name, it's same __PROCESSOR_BIND_H__ macro in AArch64/Arm/Ebc/Ia32/X64, but it is PROCESSOR_BIND_H_
> in RiscV64 and LoongArh64. For above code, if we build BIOS for RISCV64, it will try to include stdint.h due to different guard name.
>
> I am not sure if we can use same guard name to keep code alignment, or give some comments. Thanks.

Hi,
Hmm, interesting problem. Have you tried to #ifndef with some other
define? Like, I don't know, MAX_UINTN or EFIAPI?

-- 
Pedro

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Xue, Gavin]

Hi Pedro,

Thanks for your feedback.

The sample code what I listed in last mail is from/owned by another team, and I didn't find other special #ifndef case for RSIC-V building so far.
RISC-V is an new processor architecture in edk2 implementation, in our internal BIOS code, there are many similar common code for edk2 and Windows app (for simulation).
It's better if we can reuse existing code (mostly are from x86) and minimize modifications as much as possible. So I think use same guard name is make sense.
How about your comments? Thanks.

Best regards,
Gavin

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Wednesday, June 21, 2023 10:16 PM
To: Xue, Gavin <gavin.xue@intel.com>
Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng, Alan <alan.sheng@intel.com>
Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

On Fri, Jun 16, 2023 at 4:52 PM Xue, Gavin <gavin.xue@intel.com> wrote:
>
> Hi Sunil/Pedro,
>
> 1. As you know, ProcessorBind.h file of CPU Architecture file declares sets of base types for edk2 code compiling.
> So data type in edk2 code doesn't rely on specific compiler (msvc, gcc etc.), which is a good design.
>
> But in practice, for the purpose of reuse, some code can be built with edk2, and also can be built to a standalone application (e.g. Win App).
> Just like below code piece:
> ===========
> #ifndef __WRAPPER_BASE_TYPES_H__
> #define __WRAPPER_BASE_TYPES_H__
>
> //
> // To avoid definition conflict during EDK2 build, it must include
> // ProcessorBind.h before xxx.h
> //
> #ifndef __PROCESSOR_BIND_H__
>
> #include <stdint.h>
> typedef uint8_t  UINT8;
> ==========
>
> In this case, if this is a edk2 build, the code will refer to data types from ProcessorBind.h, otherwise, it will refer to stdint.h from compiler.
>
> 2. Regarding the guard name, it's same __PROCESSOR_BIND_H__ macro in AArch64/Arm/Ebc/Ia32/X64, but it is PROCESSOR_BIND_H_
> in RiscV64 and LoongArh64. For above code, if we build BIOS for RISCV64, it will try to include stdint.h due to different guard name.
>
> I am not sure if we can use same guard name to keep code alignment, or give some comments. Thanks.

Hi,
Hmm, interesting problem. Have you tried to #ifndef with some other
define? Like, I don't know, MAX_UINTN or EFIAPI?

-- 
Pedro

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Pedro Falcato]

On Thu, Jun 22, 2023 at 10:59 AM Xue, Gavin <gavin.xue@intel.com> wrote:
>
> Hi Pedro,
>
> Thanks for your feedback.
>
> The sample code what I listed in last mail is from/owned by another team, and I didn't find other special #ifndef case for RSIC-V building so far.
> RISC-V is an new processor architecture in edk2 implementation, in our internal BIOS code, there are many similar common code for edk2 and Windows app (for simulation).
> It's better if we can reuse existing code (mostly are from x86) and minimize modifications as much as possible. So I think use same guard name is make sense.
> How about your comments? Thanks.

+CC Mike K

I (personally) would oppose this change. The specific include guard's
name is an implementation detail that should not be relied upon by the
header's consumers. In my view, your code should just get fixed.
The ideas I put out should still work for any CPU arch (even if hacky;
ideally, you would just set some sort of IS_EFI macro when building).

Mike, any opinion on this matter?

-- 
Pedro

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Michael D Kinney]

It is better if we can use the same include guard names, but is not 
strictly required for builds to work.

What is the specific error message seen when using the same include guard
names as other CPU types?

Include guards have 2 elements work discussing:
* Use of define names that start with '_' or '__' are reserved either by the 
  ANSI C spec or for compilers.  The historical use by EDK II code to start
  include guards with '_' could cause potential conflicts with some compilers
  and may need to be addressed everywhere.

* Modern compilers support #pragma once that provides the same feature and 
  may actually have some build performance benefits.  This is a better long term
  direction to remove the misuse of '_' and '__' and avoid potential collisions
  with ANSI C or compilers.  It also reduces the number of defines in an EDK II
  build.

	https://en.wikipedia.org/wiki/Pragma_once

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Xue, Gavin
> Sent: Thursday, June 22, 2023 2:59 AM
> To: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng,
> Alan <alan.sheng@intel.com>
> Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> Hi Pedro,
> 
> Thanks for your feedback.
> 
> The sample code what I listed in last mail is from/owned by another team,
> and I didn't find other special #ifndef case for RSIC-V building so far.
> RISC-V is an new processor architecture in edk2 implementation, in our
> internal BIOS code, there are many similar common code for edk2 and Windows
> app (for simulation).
> It's better if we can reuse existing code (mostly are from x86) and
> minimize modifications as much as possible. So I think use same guard name
> is make sense.
> How about your comments? Thanks.
> 
> Best regards,
> Gavin
> 
> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Wednesday, June 21, 2023 10:16 PM
> To: Xue, Gavin <gavin.xue@intel.com>
> Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng,
> Alan <alan.sheng@intel.com>
> Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> On Fri, Jun 16, 2023 at 4:52 PM Xue, Gavin <gavin.xue@intel.com> wrote:
> >
> > Hi Sunil/Pedro,
> >
> > 1. As you know, ProcessorBind.h file of CPU Architecture file declares
> sets of base types for edk2 code compiling.
> > So data type in edk2 code doesn't rely on specific compiler (msvc, gcc
> etc.), which is a good design.
> >
> > But in practice, for the purpose of reuse, some code can be built with
> edk2, and also can be built to a standalone application (e.g. Win App).
> > Just like below code piece:
> > ===========
> > #ifndef __WRAPPER_BASE_TYPES_H__
> > #define __WRAPPER_BASE_TYPES_H__
> >
> > //
> > // To avoid definition conflict during EDK2 build, it must include
> > // ProcessorBind.h before xxx.h
> > //
> > #ifndef __PROCESSOR_BIND_H__
> >
> > #include <stdint.h>
> > typedef uint8_t  UINT8;
> > ==========
> >
> > In this case, if this is a edk2 build, the code will refer to data types
> from ProcessorBind.h, otherwise, it will refer to stdint.h from compiler.
> >
> > 2. Regarding the guard name, it's same __PROCESSOR_BIND_H__ macro in
> AArch64/Arm/Ebc/Ia32/X64, but it is PROCESSOR_BIND_H_
> > in RiscV64 and LoongArh64. For above code, if we build BIOS for RISCV64,
> it will try to include stdint.h due to different guard name.
> >
> > I am not sure if we can use same guard name to keep code alignment, or
> give some comments. Thanks.
> 
> Hi,
> Hmm, interesting problem. Have you tried to #ifndef with some other
> define? Like, I don't know, MAX_UINTN or EFIAPI?
> 
> --
> Pedro
> 
> 
> 
> 

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Xue, Gavin]

Hi Mike,

Thanks for your comments.
I haven't seen specific error message when using the same include guard name:
__PROCESSOR_BIND_H__ .

For short-term, I think RISC-V also could use same guard name with AArch64/Arm/Ebc/Ia32/X64
CPU architecture, which also keep code alignment.
How about your comment? Thanks.

Best regards,
Gavin

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Wednesday, June 28, 2023 4:29 AM
To: devel@edk2.groups.io; Xue, Gavin <gavin.xue@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
Cc: sunilvl@ventanamicro.com; Warkentin, Andrei <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng, Alan <alan.sheng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

It is better if we can use the same include guard names, but is not 
strictly required for builds to work.

What is the specific error message seen when using the same include guard
names as other CPU types?

Include guards have 2 elements work discussing:
* Use of define names that start with '_' or '__' are reserved either by the 
  ANSI C spec or for compilers.  The historical use by EDK II code to start
  include guards with '_' could cause potential conflicts with some compilers
  and may need to be addressed everywhere.

* Modern compilers support #pragma once that provides the same feature and 
  may actually have some build performance benefits.  This is a better long term
  direction to remove the misuse of '_' and '__' and avoid potential collisions
  with ANSI C or compilers.  It also reduces the number of defines in an EDK II
  build.

	https://en.wikipedia.org/wiki/Pragma_once

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Xue, Gavin
> Sent: Thursday, June 22, 2023 2:59 AM
> To: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng,
> Alan <alan.sheng@intel.com>
> Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> Hi Pedro,
> 
> Thanks for your feedback.
> 
> The sample code what I listed in last mail is from/owned by another team,
> and I didn't find other special #ifndef case for RSIC-V building so far.
> RISC-V is an new processor architecture in edk2 implementation, in our
> internal BIOS code, there are many similar common code for edk2 and Windows
> app (for simulation).
> It's better if we can reuse existing code (mostly are from x86) and
> minimize modifications as much as possible. So I think use same guard name
> is make sense.
> How about your comments? Thanks.
> 
> Best regards,
> Gavin
> 
> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Wednesday, June 21, 2023 10:16 PM
> To: Xue, Gavin <gavin.xue@intel.com>
> Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng,
> Alan <alan.sheng@intel.com>
> Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> On Fri, Jun 16, 2023 at 4:52 PM Xue, Gavin <gavin.xue@intel.com> wrote:
> >
> > Hi Sunil/Pedro,
> >
> > 1. As you know, ProcessorBind.h file of CPU Architecture file declares
> sets of base types for edk2 code compiling.
> > So data type in edk2 code doesn't rely on specific compiler (msvc, gcc
> etc.), which is a good design.
> >
> > But in practice, for the purpose of reuse, some code can be built with
> edk2, and also can be built to a standalone application (e.g. Win App).
> > Just like below code piece:
> > ===========
> > #ifndef __WRAPPER_BASE_TYPES_H__
> > #define __WRAPPER_BASE_TYPES_H__
> >
> > //
> > // To avoid definition conflict during EDK2 build, it must include
> > // ProcessorBind.h before xxx.h
> > //
> > #ifndef __PROCESSOR_BIND_H__
> >
> > #include <stdint.h>
> > typedef uint8_t  UINT8;
> > ==========
> >
> > In this case, if this is a edk2 build, the code will refer to data types
> from ProcessorBind.h, otherwise, it will refer to stdint.h from compiler.
> >
> > 2. Regarding the guard name, it's same __PROCESSOR_BIND_H__ macro in
> AArch64/Arm/Ebc/Ia32/X64, but it is PROCESSOR_BIND_H_
> > in RiscV64 and LoongArh64. For above code, if we build BIOS for RISCV64,
> it will try to include stdint.h due to different guard name.
> >
> > I am not sure if we can use same guard name to keep code alignment, or
> give some comments. Thanks.
> 
> Hi,
> Hmm, interesting problem. Have you tried to #ifndef with some other
> define? Like, I don't know, MAX_UINTN or EFIAPI?
> 
> --
> Pedro
> 
> 
> 
> 

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Michael D Kinney]

Using the same include guard define name is preferred.

Why was anything other than that considered?

Mike

> -----Original Message-----
> From: Xue, Gavin <gavin.xue@intel.com>
> Sent: Friday, June 30, 2023 2:29 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; Pedro Falcato <pedro.falcato@gmail.com>
> Cc: sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng, Alan <alan.sheng@intel.com>
> Subject: RE: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> Hi Mike,
> 
> Thanks for your comments.
> I haven't seen specific error message when using the same include guard
> name:
> __PROCESSOR_BIND_H__ .
> 
> For short-term, I think RISC-V also could use same guard name with
> AArch64/Arm/Ebc/Ia32/X64
> CPU architecture, which also keep code alignment.
> How about your comment? Thanks.
> 
> Best regards,
> Gavin
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, June 28, 2023 4:29 AM
> To: devel@edk2.groups.io; Xue, Gavin <gavin.xue@intel.com>; Pedro
> Falcato <pedro.falcato@gmail.com>
> Cc: sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng, Alan <alan.sheng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> It is better if we can use the same include guard names, but is not
> strictly required for builds to work.
> 
> What is the specific error message seen when using the same include
> guard
> names as other CPU types?
> 
> Include guards have 2 elements work discussing:
> * Use of define names that start with '_' or '__' are reserved either
> by the
>   ANSI C spec or for compilers.  The historical use by EDK II code to
> start
>   include guards with '_' could cause potential conflicts with some
> compilers
>   and may need to be addressed everywhere.
> 
> * Modern compilers support #pragma once that provides the same feature
> and
>   may actually have some build performance benefits.  This is a better
> long term
>   direction to remove the misuse of '_' and '__' and avoid potential
> collisions
>   with ANSI C or compilers.  It also reduces the number of defines in
> an EDK II
>   build.
> 
> 	https://en.wikipedia.org/wiki/Pragma_once
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Xue,
> Gavin
> > Sent: Thursday, June 22, 2023 2:59 AM
> > To: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> > <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng,
> > Alan <alan.sheng@intel.com>
> > Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> > symbol define for RISCV64
> >
> > Hi Pedro,
> >
> > Thanks for your feedback.
> >
> > The sample code what I listed in last mail is from/owned by another
> team,
> > and I didn't find other special #ifndef case for RSIC-V building so
> far.
> > RISC-V is an new processor architecture in edk2 implementation, in
> our
> > internal BIOS code, there are many similar common code for edk2 and
> Windows
> > app (for simulation).
> > It's better if we can reuse existing code (mostly are from x86) and
> > minimize modifications as much as possible. So I think use same guard
> name
> > is make sense.
> > How about your comments? Thanks.
> >
> > Best regards,
> > Gavin
> >
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Wednesday, June 21, 2023 10:16 PM
> > To: Xue, Gavin <gavin.xue@intel.com>
> > Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> > <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng,
> > Alan <alan.sheng@intel.com>
> > Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> > symbol define for RISCV64
> >
> > On Fri, Jun 16, 2023 at 4:52 PM Xue, Gavin <gavin.xue@intel.com>
> wrote:
> > >
> > > Hi Sunil/Pedro,
> > >
> > > 1. As you know, ProcessorBind.h file of CPU Architecture file
> declares
> > sets of base types for edk2 code compiling.
> > > So data type in edk2 code doesn't rely on specific compiler (msvc,
> gcc
> > etc.), which is a good design.
> > >
> > > But in practice, for the purpose of reuse, some code can be built
> with
> > edk2, and also can be built to a standalone application (e.g. Win
> App).
> > > Just like below code piece:
> > > ===========
> > > #ifndef __WRAPPER_BASE_TYPES_H__
> > > #define __WRAPPER_BASE_TYPES_H__
> > >
> > > //
> > > // To avoid definition conflict during EDK2 build, it must include
> > > // ProcessorBind.h before xxx.h
> > > //
> > > #ifndef __PROCESSOR_BIND_H__
> > >
> > > #include <stdint.h>
> > > typedef uint8_t  UINT8;
> > > ==========
> > >
> > > In this case, if this is a edk2 build, the code will refer to data
> types
> > from ProcessorBind.h, otherwise, it will refer to stdint.h from
> compiler.
> > >
> > > 2. Regarding the guard name, it's same __PROCESSOR_BIND_H__ macro
> in
> > AArch64/Arm/Ebc/Ia32/X64, but it is PROCESSOR_BIND_H_
> > > in RiscV64 and LoongArh64. For above code, if we build BIOS for
> RISCV64,
> > it will try to include stdint.h due to different guard name.
> > >
> > > I am not sure if we can use same guard name to keep code alignment,
> or
> > give some comments. Thanks.
> >
> > Hi,
> > Hmm, interesting problem. Have you tried to #ifndef with some other
> > define? Like, I don't know, MAX_UINTN or EFIAPI?
> >
> > --
> > Pedro
> >
> >
> > 
> >

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Xue, Gavin]

If we agree to use the same include guard define name for all CPU architecture,
I re-sent out the v2 new patch. Please help to review it. Thanks.

Best regards,
Gavin

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Saturday, July 1, 2023 12:59 AM
To: Xue, Gavin <gavin.xue@intel.com>; devel@edk2.groups.io; Pedro Falcato <pedro.falcato@gmail.com>
Cc: sunilvl@ventanamicro.com; Warkentin, Andrei <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng, Alan <alan.sheng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

Using the same include guard define name is preferred.

Why was anything other than that considered?

Mike

> -----Original Message-----
> From: Xue, Gavin <gavin.xue@intel.com>
> Sent: Friday, June 30, 2023 2:29 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; Pedro Falcato <pedro.falcato@gmail.com>
> Cc: sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng, Alan <alan.sheng@intel.com>
> Subject: RE: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> Hi Mike,
> 
> Thanks for your comments.
> I haven't seen specific error message when using the same include guard
> name:
> __PROCESSOR_BIND_H__ .
> 
> For short-term, I think RISC-V also could use same guard name with
> AArch64/Arm/Ebc/Ia32/X64
> CPU architecture, which also keep code alignment.
> How about your comment? Thanks.
> 
> Best regards,
> Gavin
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, June 28, 2023 4:29 AM
> To: devel@edk2.groups.io; Xue, Gavin <gavin.xue@intel.com>; Pedro
> Falcato <pedro.falcato@gmail.com>
> Cc: sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng, Alan <alan.sheng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> symbol define for RISCV64
> 
> It is better if we can use the same include guard names, but is not
> strictly required for builds to work.
> 
> What is the specific error message seen when using the same include
> guard
> names as other CPU types?
> 
> Include guards have 2 elements work discussing:
> * Use of define names that start with '_' or '__' are reserved either
> by the
>   ANSI C spec or for compilers.  The historical use by EDK II code to
> start
>   include guards with '_' could cause potential conflicts with some
> compilers
>   and may need to be addressed everywhere.
> 
> * Modern compilers support #pragma once that provides the same feature
> and
>   may actually have some build performance benefits.  This is a better
> long term
>   direction to remove the misuse of '_' and '__' and avoid potential
> collisions
>   with ANSI C or compilers.  It also reduces the number of defines in
> an EDK II
>   build.
> 
> 	https://en.wikipedia.org/wiki/Pragma_once
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Xue,
> Gavin
> > Sent: Thursday, June 22, 2023 2:59 AM
> > To: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> > <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng,
> > Alan <alan.sheng@intel.com>
> > Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> > symbol define for RISCV64
> >
> > Hi Pedro,
> >
> > Thanks for your feedback.
> >
> > The sample code what I listed in last mail is from/owned by another
> team,
> > and I didn't find other special #ifndef case for RSIC-V building so
> far.
> > RISC-V is an new processor architecture in edk2 implementation, in
> our
> > internal BIOS code, there are many similar common code for edk2 and
> Windows
> > app (for simulation).
> > It's better if we can reuse existing code (mostly are from x86) and
> > minimize modifications as much as possible. So I think use same guard
> name
> > is make sense.
> > How about your comments? Thanks.
> >
> > Best regards,
> > Gavin
> >
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Wednesday, June 21, 2023 10:16 PM
> > To: Xue, Gavin <gavin.xue@intel.com>
> > Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> > <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>;
> Sheng,
> > Alan <alan.sheng@intel.com>
> > Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind
> > symbol define for RISCV64
> >
> > On Fri, Jun 16, 2023 at 4:52 PM Xue, Gavin <gavin.xue@intel.com>
> wrote:
> > >
> > > Hi Sunil/Pedro,
> > >
> > > 1. As you know, ProcessorBind.h file of CPU Architecture file
> declares
> > sets of base types for edk2 code compiling.
> > > So data type in edk2 code doesn't rely on specific compiler (msvc,
> gcc
> > etc.), which is a good design.
> > >
> > > But in practice, for the purpose of reuse, some code can be built
> with
> > edk2, and also can be built to a standalone application (e.g. Win
> App).
> > > Just like below code piece:
> > > ===========
> > > #ifndef __WRAPPER_BASE_TYPES_H__
> > > #define __WRAPPER_BASE_TYPES_H__
> > >
> > > //
> > > // To avoid definition conflict during EDK2 build, it must include
> > > // ProcessorBind.h before xxx.h
> > > //
> > > #ifndef __PROCESSOR_BIND_H__
> > >
> > > #include <stdint.h>
> > > typedef uint8_t  UINT8;
> > > ==========
> > >
> > > In this case, if this is a edk2 build, the code will refer to data
> types
> > from ProcessorBind.h, otherwise, it will refer to stdint.h from
> compiler.
> > >
> > > 2. Regarding the guard name, it's same __PROCESSOR_BIND_H__ macro
> in
> > AArch64/Arm/Ebc/Ia32/X64, but it is PROCESSOR_BIND_H_
> > > in RiscV64 and LoongArh64. For above code, if we build BIOS for
> RISCV64,
> > it will try to include stdint.h due to different guard name.
> > >
> > > I am not sure if we can use same guard name to keep code alignment,
> or
> > give some comments. Thanks.
> >
> > Hi,
> > Hmm, interesting problem. Have you tried to #ifndef with some other
> > define? Like, I don't know, MAX_UINTN or EFIAPI?
> >
> > --
> > Pedro
> >
> >
> > 
> >

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Pedro Falcato]

On Fri, Jun 30, 2023 at 5:59 PM Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Using the same include guard define name is preferred.
>
> Why was anything other than that considered?

I don't see the point of making the include guard an actual part of
the "API". Consumers should not depend on it being named $WHATEVER.
That is a hack.
Include guards are an implementation detail and making that stable
actively stops you from doing things like using #pragma once or fixing
the __DOUBLE_UNDERSCORE_H__ stuff.
So I would vote for not changing this, downstream consumers that rely
on __PROCESSOR_BIND_H__ should be fixed, downstream.

-- 
Pedro

Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

[Xue, Gavin]

Hi Pedro,

There are 3 different include guard names for ProcessorBind.h file.
AArch64/Arm/Ebc/Ia32/X64 use __PROCESSOR_BIND_H__ (start/end with 2 underscore characters).
RISCV64 uses PROCESSOR_BIND_H__ (end with 2 underscore)
LoongArch64 uses PROCESSOR_BIND_H_ (end with  1 underscore)

From the code rule of uniform style, I think it is also a necessary change to
same include guard name.
If you still think we shouldn't have the change, I am okay to close this patch. Thanks.

Best regards,
Gavin

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Tuesday, July 4, 2023 1:02 AM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Xue, Gavin <gavin.xue@intel.com>; sunilvl@ventanamicro.com; Warkentin, Andrei <andrei.warkentin@intel.com>; Wang, Yimin <yimin.wang@intel.com>; Sheng, Alan <alan.sheng@intel.com>
Subject: Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64

On Fri, Jun 30, 2023 at 5:59 PM Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Using the same include guard define name is preferred.
>
> Why was anything other than that considered?

I don't see the point of making the include guard an actual part of
the "API". Consumers should not depend on it being named $WHATEVER.
That is a hack.
Include guards are an implementation detail and making that stable
actively stops you from doing things like using #pragma once or fixing
the __DOUBLE_UNDERSCORE_H__ stuff.
So I would vote for not changing this, downstream consumers that rely
on __PROCESSOR_BIND_H__ should be fixed, downstream.

-- 
Pedro