Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64
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 Sent: Tuesday, July 4, 2023 1:02 AM To: devel@edk2.groups.io; Kinney, Michael D Cc: Xue, Gavin ; suni...@ventanamicro.com; Warkentin, Andrei ; Wang, Yimin ; Sheng, Alan 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 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106655): https://edk2.groups.io/g/devel/message/106655 Mute This Topic: https://groups.io/mt/99567569/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64
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 Sent: Saturday, July 1, 2023 12:59 AM To: Xue, Gavin ; devel@edk2.groups.io; Pedro Falcato Cc: suni...@ventanamicro.com; Warkentin, Andrei ; Wang, Yimin ; Sheng, Alan ; Kinney, Michael D 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 > Sent: Friday, June 30, 2023 2:29 AM > To: Kinney, Michael D ; > devel@edk2.groups.io; Pedro Falcato > Cc: suni...@ventanamicro.com; Warkentin, Andrei > ; Wang, Yimin ; > Sheng, Alan > 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 > Sent: Wednesday, June 28, 2023 4:29 AM > To: devel@edk2.groups.io; Xue, Gavin ; Pedro > Falcato > Cc: suni...@ventanamicro.com; Warkentin, Andrei > ; Wang, Yimin ; > Sheng, Alan ; Kinney, Michael D > > 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 On Behalf Of Xue, > Gavin > > Sent: Thursday, June 22, 2023 2:59 AM > > To: Pedro Falcato > > Cc: devel@edk2.groups.io; suni...@ventanamicro.com; Warkentin, Andrei > > ; Wang, Yimin ; > Sheng, > > Alan > > 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 > > Sent: Wednesday, June 21, 2023 10:16 PM > > To: Xue, Gavin > > Cc: devel@edk2.groups.io; suni...@ventanamicro.com; Warkentin, Andrei > > ; Wang, Yimin ; > Sheng, > > Alan > > 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 > 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 > > ed
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 Sent: Wednesday, June 28, 2023 4:29 AM To: devel@edk2.groups.io; Xue, Gavin ; Pedro Falcato Cc: suni...@ventanamicro.com; Warkentin, Andrei ; Wang, Yimin ; Sheng, Alan ; Kinney, Michael D 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 On Behalf Of Xue, Gavin > Sent: Thursday, June 22, 2023 2:59 AM > To: Pedro Falcato > Cc: devel@edk2.groups.io; suni...@ventanamicro.com; Warkentin, Andrei > ; Wang, Yimin ; Sheng, > Alan > 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 > Sent: Wednesday, June 21, 2023 10:16 PM > To: Xue, Gavin > Cc: devel@edk2.groups.io; suni...@ventanamicro.com; Warkentin, Andrei > ; Wang, Yimin ; Sheng, > Alan > 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 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 > > 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 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106568): https://edk2.groups.io/g/devel/message/106568 Mute This Topic: https://groups.io/mt/99567569/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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 Sent: Wednesday, June 21, 2023 10:16 PM To: Xue, Gavin Cc: devel@edk2.groups.io; suni...@ventanamicro.com; Warkentin, Andrei ; Wang, Yimin ; Sheng, Alan 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 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 > 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106286): https://edk2.groups.io/g/devel/message/106286 Mute This Topic: https://groups.io/mt/99567569/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2 PATCH] MdePkg: Use same ProcessorBind symbol define for RISCV64
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 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 Sent: Friday, June 16, 2023 10:12 PM To: devel@edk2.groups.io; suni...@ventanamicro.com Cc: Xue, Gavin ; Warkentin, Andrei ; Wang, Yimin ; Sheng, Alan 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 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106189): https://edk2.groups.io/g/devel/message/106189 Mute This Topic: https://groups.io/mt/99567569/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
