Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove __inline__ attribute for IO functions

["Liming Gao" <liming.gao@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6462 bytes --]



From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Andrew Fish via Groups.Io
Sent: Tuesday, October 1, 2019 5:12 AM
To: devel@edk2.groups.io; lersek@redhat.com
Cc: Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove __inline__ attribute for IO functions




On Sep 30, 2019, at 3:35 PM, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:

Hi Liming,

On 09/27/19 09:46, Liming Gao wrote:

__inline__ attribute will make the functions not be exposed as the
library interface. It will cause CLANG9 compiler fail.

Signed-off-by: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>>
---
MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c | 6 ------
1 file changed, 6 deletions(-)

Did you regression-test this change against GCC48 (for example)?

I can't tell why we have the __inline__'s in the first place. They date
back to historical commit e1f414b6a7d8 ("Import some basic libraries
instances for Mde Packages.", 2007-06-22). And that commit does not
explain __inline__.

If we remove __inline__ for the whole GCC toolchain *family*, then I
think we need a better justification than just "makes CLANG9 fail".

[Liming] I verify GCC5 tool chain. I will verify GCC48/GCC49 and XCODE5.
I don’t know the specific reason about __inline__. If there is no impact on
other GCC tool chain, I prefer to remove them.

Yikes,

Looks like __inline__ is the C89 version of inline.

I'm kind of surprised clang with LTO would just not ignore the inline, but then I came across....

https://en.wikipedia.org/wiki/Inline_function
"gnu89 semantics of inline and extern inline are essentially the exact opposite of those in C99[4]<https://en.wikipedia.org/wiki/Inline_function#cite_note-4>, with the exception that gnu89 permits redefinition of an extern inline function as an unqualified function, while C99 inline does not[5]<https://en.wikipedia.org/wiki/Inline_function#cite_note-gcc-5-porting-5>. Thus, gnu89 extern inline without redefinition is like C99 inline, and gnu89 inline is like C99 extern inline; in other words, in gnu89, a function defined inline will always and a function defined extern inline will never emit an externally visible function. The rationale for this is that it matches variables, for which storage will never be reserved if defined as extern and always if defined without. The rationale for C99, in contrast, is that it would be astonishing<https://en.wikipedia.org/wiki/Principle_of_least_astonishment> if using inline would have a side-effect—to always emit a non-inlined version of the function—that is contrary to what its name suggests.
The remarks for C99 about the need to provide exactly one externally visible function instance for inlined functions and about the resulting problem with unreachable code apply mutatis mutandis to gnu89 as well.
gcc up to and including version 4.2 used gnu89 inline semantics even when -std=c99 was explicitly specified.[6]<https://en.wikipedia.org/wiki/Inline_function#cite_note-6> With version 5[5]<https://en.wikipedia.org/wiki/Inline_function#cite_note-gcc-5-porting-5>, gcc switched from gnu89 to the gnu11 dialect, effectively enabling C99 inline semantics by default. To use gnu89 semantics instead, they have to be enabled explicitly, either with -std=gnu89 or, to only affect inlining, -fgnu89-inline, or by adding the gnu_inline attribute to all inline declarations. To ensure C99 semantics, either -std=c99, -std=c11, -std=gnu99 or -std=gnu11 (without -fgnu89-inline) can be used.[3]<https://en.wikipedia.org/wiki/Inline_function#cite_note-gcc-inline-3>"

And the above makes you look at the C99 definition "In C99, a function defined inline will never, and a function defined extern inline will always, emit an externally visible function. ". So this make me wonder if clang is getting more pedantic about the C99 definition of inline (__inline__). So I wonder if we could use an` if ( __STDC_VERSION__ < 199901L)` to turn off the __inline__ to fix the clang issue?

It also seems strange to me the __inline__ only exists next to the library function. Given it is not in the header (and the code is not in the header) I'm not really sure what the compiler can do? When the BaseIoLibIntrinsic library gets compiled it is going to create the intrinsic functions. It seems like code only comes together a link time? So unless the GCC linker was doing inline code generation at link time I'm not sure  how the __inline__ helps. Does the compiler tag the object with some kind of hint? If you are doing Link Time Optimization (LTO) the __inline__ is kind of a moot point as the code gen will always inline simple stuff like this.

I'd point out when we ported to GCC we came from VC++ and always had LTO, so it is likely we did not have a good grasp of GCC inlining. Thus there is a non-zero chance this code is a no-op even on old GCC versions. But it is worth checking out.

[Liming] This seems the remaining clean up task. So, I prefer to remove __inline__ if no impact on GCC tool chain.

Thanks
Liming
[1]  $ git grep __inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:35:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:63:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:90:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:120:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:148:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:178:__inline__



Thanks,

Andrew Fish


Thanks
Laszlo


diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c b/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
index 055f0a947e..b3a1a20256 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
+++ b/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
@@ -32,7 +32,6 @@
  @return The value read.

**/
-__inline__
UINT8
EFIAPI
IoRead8 (
@@ -60,7 +59,6 @@ IoRead8 (
  @return The value written the I/O port.

**/
-__inline__
UINT8
EFIAPI
IoWrite8 (
@@ -87,7 +85,6 @@ IoWrite8 (
  @return The value read.

**/
-__inline__
UINT16
EFIAPI
IoRead16 (
@@ -117,7 +114,6 @@ IoRead16 (
  @return The value written the I/O port.

**/
-__inline__
UINT16
EFIAPI
IoWrite16 (
@@ -145,7 +141,6 @@ IoWrite16 (
  @return The value read.

**/
-__inline__
UINT32
EFIAPI
IoRead32 (
@@ -175,7 +170,6 @@ IoRead32 (
  @return The value written the I/O port.

**/
-__inline__
UINT32
EFIAPI
IoWrite32 (






[-- Attachment #2: Type: text/html, Size: 29441 bytes --]