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

[Andrew Fish via Groups.Io]

> On Sep 30, 2019, at 3:35 PM, Laszlo Ersek <lersek at 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 at intel.com <mailto:liming.gao at 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".
> 

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. 

[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 (
>> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48311): https://edk2.groups.io/g/devel/message/48311
Mute This Topic: https://groups.io/mt/34309058/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20190930/ab47d7f5/attachment.htm>