__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>
---
MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c | 6 ------
1 file changed, 6 deletions(-)
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 (
--
2.13.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48197): https://edk2.groups.io/g/devel/message/48197
Mute This Topic: https://groups.io/mt/34309058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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>
> ---
> 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".
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 (#48303): https://edk2.groups.io/g/devel/message/48303
Mute This Topic: https://groups.io/mt/34309058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> On Sep 30, 2019, at 3:35 PM, Laszlo Ersek <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".
>
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 (
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48579): https://edk2.groups.io/g/devel/message/48579
Mute This Topic: https://groups.io/mt/34309058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 10/08/19 16:47, Gao, Liming wrote: > [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. > [Liming] This seems the remaining clean up task. So, I prefer to remove > __inline__ if no impact on GCC tool chain. OK. Given your testing with GCC48, I'm fine. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48602): https://edk2.groups.io/g/devel/message/48602 Mute This Topic: https://groups.io/mt/34309058/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Laszlo: > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, October 9, 2019 4:22 AM > To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; afish@apple.com > Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove __inline__ attribute for IO functions > > On 10/08/19 16:47, Gao, Liming wrote: > > > [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. > > > [Liming] This seems the remaining clean up task. So, I prefer to remove > > __inline__ if no impact on GCC tool chain. > > OK. Given your testing with GCC48, I'm fine. > With this patch set, I verify GCC48/GCC49/GCC5 on Ovmf3264. They can all boot to Shell. Are they enough? Thanks Liming > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48727): https://edk2.groups.io/g/devel/message/48727 Mute This Topic: https://groups.io/mt/34309058/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Liming, Andrew, On 10/10/19 14:32, Liming Gao wrote: > Laszlo: > >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Wednesday, October 9, 2019 4:22 AM >> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; afish@apple.com >> Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove __inline__ attribute for IO functions >> >> On 10/08/19 16:47, Gao, Liming wrote: >> >>> [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. >> >>> [Liming] This seems the remaining clean up task. So, I prefer to remove >>> __inline__ if no impact on GCC tool chain. >> >> OK. Given your testing with GCC48, I'm fine. >> > With this patch set, I verify GCC48/GCC49/GCC5 on Ovmf3264. They can all boot to Shell. > Are they enough? Would you guys agree with the following commit message, on this patch? """ __inline__ has no discernible effect with the GCC48 / GCC49 / GCC5 toolchains, but it breaks the build with CLANG9. Remove __inline__. """ If you can update the commit message like that, then you can add: Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48741): https://edk2.groups.io/g/devel/message/48741 Mute This Topic: https://groups.io/mt/34309058/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Laszlo: >-----Original Message----- >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >Laszlo Ersek >Sent: Friday, October 11, 2019 12:32 AM >To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; >afish@apple.com >Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove >__inline__ attribute for IO functions > >Hi Liming, Andrew, > >On 10/10/19 14:32, Liming Gao wrote: >> Laszlo: >> >>> -----Original Message----- >>> From: Laszlo Ersek <lersek@redhat.com> >>> Sent: Wednesday, October 9, 2019 4:22 AM >>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; >afish@apple.com >>> Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: >Remove __inline__ attribute for IO functions >>> >>> On 10/08/19 16:47, Gao, Liming wrote: >>> >>>> [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. >>> >>>> [Liming] This seems the remaining clean up task. So, I prefer to remove >>>> __inline__ if no impact on GCC tool chain. >>> >>> OK. Given your testing with GCC48, I'm fine. >>> >> With this patch set, I verify GCC48/GCC49/GCC5 on Ovmf3264. They can all >boot to Shell. >> Are they enough? > >Would you guys agree with the following commit message, on this patch? > >""" >__inline__ has no discernible effect with the GCC48 / GCC49 / GCC5 >toolchains, but it breaks the build with CLANG9. > >Remove __inline__. >""" > Agree. I will update the commit message. Thanks for your suggestion. Liming >If you can update the commit message like that, then you can add: > >Acked-by: Laszlo Ersek <lersek@redhat.com> > >Thanks! >Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48769): https://edk2.groups.io/g/devel/message/48769 Mute This Topic: https://groups.io/mt/34309058/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 2019-10-10 09:32:19, Laszlo Ersek wrote: > Hi Liming, Andrew, > > On 10/10/19 14:32, Liming Gao wrote: > > Laszlo: > > > >> -----Original Message----- > >> From: Laszlo Ersek <lersek@redhat.com> > >> Sent: Wednesday, October 9, 2019 4:22 AM > >> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; afish@apple.com > >> Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove __inline__ attribute for IO functions > >> > >> On 10/08/19 16:47, Gao, Liming wrote: > >> > >>> [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. > >> > >>> [Liming] This seems the remaining clean up task. So, I prefer to remove > >>> __inline__ if no impact on GCC tool chain. > >> > >> OK. Given your testing with GCC48, I'm fine. > >> > > With this patch set, I verify GCC48/GCC49/GCC5 on Ovmf3264. They can all boot to Shell. > > Are they enough? > > Would you guys agree with the following commit message, on this patch? > > """ > __inline__ has no discernible effect with the GCC48 / GCC49 / GCC5 > toolchains, but it breaks the build with CLANG9. > > Remove __inline__. > """ Would it be more accurate to say it didn't have a functional difference? Did we rule out that it might have made a difference in code gen? I guess I wouldn't be surprised if the older non-LTO toolchains didn't make use of it anyway. So, maybe there is no difference in code gen. With LTO the linker is hopefully smarter about auto-inlining anyhow. -Jordan -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48853): https://edk2.groups.io/g/devel/message/48853 Mute This Topic: https://groups.io/mt/34309058/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Jordan: >-----Original Message----- >From: Justen, Jordan L >Sent: Saturday, October 12, 2019 3:22 AM >To: afish@apple.com; Laszlo Ersek <lersek@redhat.com>; >devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com> >Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove >__inline__ attribute for IO functions > >On 2019-10-10 09:32:19, Laszlo Ersek wrote: >> Hi Liming, Andrew, >> >> On 10/10/19 14:32, Liming Gao wrote: >> > Laszlo: >> > >> >> -----Original Message----- >> >> From: Laszlo Ersek <lersek@redhat.com> >> >> Sent: Wednesday, October 9, 2019 4:22 AM >> >> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; >afish@apple.com >> >> Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: >Remove __inline__ attribute for IO functions >> >> >> >> On 10/08/19 16:47, Gao, Liming wrote: >> >> >> >>> [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. >> >> >> >>> [Liming] This seems the remaining clean up task. So, I prefer to remove >> >>> __inline__ if no impact on GCC tool chain. >> >> >> >> OK. Given your testing with GCC48, I'm fine. >> >> >> > With this patch set, I verify GCC48/GCC49/GCC5 on Ovmf3264. They can all >boot to Shell. >> > Are they enough? >> >> Would you guys agree with the following commit message, on this patch? >> >> """ >> __inline__ has no discernible effect with the GCC48 / GCC49 / GCC5 >> toolchains, but it breaks the build with CLANG9. >> >> Remove __inline__. >> """ > >Would it be more accurate to say it didn't have a functional >difference? Did we rule out that it might have made a difference in >code gen? Yes. The function are same. I verify GCC5 before and after this change. The generated image is also same. So, there is no different code gen. Thanks Liming > >I guess I wouldn't be surprised if the older non-LTO toolchains didn't >make use of it anyway. So, maybe there is no difference in code gen. > >With LTO the linker is hopefully smarter about auto-inlining anyhow. > >-Jordan -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48859): https://edk2.groups.io/g/devel/message/48859 Mute This Topic: https://groups.io/mt/34309058/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2025 Red Hat, Inc.