From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=sLSkw5NR; spf=pass (domain: apple.com, ip: 17.151.62.67, mailfrom: afish@apple.com) Received: from nwk-aaemail-lapp02.apple.com (nwk-aaemail-lapp02.apple.com [17.151.62.67]) by groups.io with SMTP; Mon, 30 Sep 2019 14:12:12 -0700 Received: from pps.filterd (nwk-aaemail-lapp02.apple.com [127.0.0.1]) by nwk-aaemail-lapp02.apple.com (8.16.0.27/8.16.0.27) with SMTP id x8UKqEdT005561; Mon, 30 Sep 2019 14:12:10 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=8oAG2CwxdKkRYD6C+PMlC3Vnds1vyzPuejjhF14v01Y=; b=sLSkw5NRKm/QHNMHtTQXfaA/gQN2XEUsvVdgKrlLY2dZiJavs3/Kd5njttSZ6ieOVJwD mqIlLMjtAozdsNyBzgOwfME2F0ucLYmcMauNrx/5de7ucJoMDBrrZZaNRsW/n2Bqrw93 RP5eqx9GXQjcU5ATQgwLhP3702jGu9YSiUkDiQv5icmYnNdI+g59lIpUszwiidHlpXPq mRYFGRbBbvxT9ODOrQ2AVGu11TflkZMONHk7LBzl4KeeAh4zk4P46Mbo9OgTT53x/e1D QLi1skm26Z08jUgoFRJxNFtgUMlk4L5NMc3zRm3gjSnCKieErCpeK2vYOhsYt23ow68W aQ== Received: from ma1-mtap-s03.corp.apple.com (ma1-mtap-s03.corp.apple.com [17.40.76.7]) by nwk-aaemail-lapp02.apple.com with ESMTP id 2va43h3vf7-17 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Mon, 30 Sep 2019 14:12:10 -0700 Received: from nwk-mmpp-sz13.apple.com (nwk-mmpp-sz13.apple.com [17.128.115.216]) by ma1-mtap-s03.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PYN00ISCXK04X40@ma1-mtap-s03.corp.apple.com>; Mon, 30 Sep 2019 14:12:01 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz13.apple.com by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PYN00C00WNQZ200@nwk-mmpp-sz13.apple.com>; Mon, 30 Sep 2019 14:12:00 -0700 (PDT) X-Va-CD: 0 X-Va-ID: eb12a1f8-75cd-4128-94a6-9d5e479d916c X-V-A: X-V-T-CD: 08777febe38bb384cc57fda39d0586b7 X-V-E-CD: c887cf08f41fd1ec965524243a178af0 X-V-R-CD: ad8d7ee477c78db17969d13d16ba8105 X-V-CD: 0 X-V-ID: 6fe628a8-65a9-4531-9bd2-47ff84d87799 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-09-30_11:,, signatures=0 Received: from [17.103.11.227] (unknown [17.103.11.227]) by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PYN009GVXJYY100@nwk-mmpp-sz13.apple.com>; Mon, 30 Sep 2019 14:11:59 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [Patch 05/12] MdePkg BaseIoLibIntrinsic: Remove __inline__ attribute for IO functions Date: Mon, 30 Sep 2019 16:11:52 -0500 In-reply-to: Cc: liming.gao@intel.com To: devel@edk2.groups.io, lersek@redhat.com References: <1569570395-11240-1-git-send-email-liming.gao@intel.com> <1569570395-11240-6-git-send-email-liming.gao@intel.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-09-30_11:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_57E6F75C-A539-47D7-A87F-AA1FA75638F1" --Apple-Mail=_57E6F75C-A539-47D7-A87F-AA1FA75638F1 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Sep 30, 2019, at 3:35 PM, Laszlo Ersek wrote: >=20 > Hi Liming, >=20 > 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. >>=20 >> Signed-off-by: Liming Gao > >> --- >> MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c | 6 ------ >> 1 file changed, 6 deletions(-) >=20 > Did you regression-test this change against GCC48 (for example)? >=20 > 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__. >=20 > If we remove __inline__ for the whole GCC toolchain *family*, then I > think we need a better justification than just "makes CLANG9 fail". >=20 Yikes, Looks like __inline__ is the C89 version of inline.=20 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 opp= osite of those in C99[4] , with the exception that gnu89 permits redefinition of an extern = inline function as an unqualified function, while C99 inline does not[5] . Th= us, gnu89 extern inline without redefinition is like C99 inline, and gnu89 = inline is like C99 extern inline; in other words, in gnu89, a function defi= ned 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 an= d always if defined without. The rationale for C99, in contrast, is that it= would be astonishing if using inline would have a side-effect=E2=80=94to always emit = a non-inlined version of the function=E2=80=94that is contrary to what its = name suggests. The remarks for C99 about the need to provide exactly one externally visib= le 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=3Dc99 was explicitly specified.[6] With version 5[5] , gcc switched from gnu89 to the g= nu11 dialect, effectively enabling C99 inline semantics by default. To use = gnu89 semantics instead, they have to be enabled explicitly, either with -s= td=3Dgnu89 or, to only affect inlining, -fgnu89-inline, or by adding the gn= u_inline attribute to all inline declarations. To ensure C99 semantics, eit= her -std=3Dc99, -std=3Dc11, -std=3Dgnu99 or -std=3Dgnu11 (without -fgnu89-i= nline) can be used.[3] " And the above makes you look at the C99 definition "In C99, a function def= ined inline will never, and a function defined extern inline will always, e= mit an externally visible function. ". So this make me wonder if clang is g= etting more pedantic about the C99 definition of inline (__inline__). So I = wonder if we could use an` if ( __STDC_VERSION__ < 199901L)` to turn off th= e __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 se= ems 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.=20 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.=20 [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 >=20 >> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c b/MdePkg/Libr= ary/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. >>=20 >> **/ >> -__inline__ >> UINT8 >> EFIAPI >> IoRead8 ( >> @@ -60,7 +59,6 @@ IoRead8 ( >> @return The value written the I/O port. >>=20 >> **/ >> -__inline__ >> UINT8 >> EFIAPI >> IoWrite8 ( >> @@ -87,7 +85,6 @@ IoWrite8 ( >> @return The value read. >>=20 >> **/ >> -__inline__ >> UINT16 >> EFIAPI >> IoRead16 ( >> @@ -117,7 +114,6 @@ IoRead16 ( >> @return The value written the I/O port. >>=20 >> **/ >> -__inline__ >> UINT16 >> EFIAPI >> IoWrite16 ( >> @@ -145,7 +141,6 @@ IoWrite16 ( >> @return The value read. >>=20 >> **/ >> -__inline__ >> UINT32 >> EFIAPI >> IoRead32 ( >> @@ -175,7 +170,6 @@ IoRead32 ( >> @return The value written the I/O port. >>=20 >> **/ >> -__inline__ >> UINT32 >> EFIAPI >> IoWrite32 ( >>=20 >=20 >=20 >=20 --Apple-Mail=_57E6F75C-A539-47D7-A87F-AA1FA75638F1 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Sep 30, 2= 019, 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>
---
MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c | 6 ------<= br class=3D"">1 file changed, 6 deletions(-)
Did you regression-test this cha= nge against GCC48 (for example)?

I can't tell why we have t= he __inline__'s in the first place. They date
back to historical commit e1f414b6a7d8 ("Import some= basic libraries
ins= tances 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 jus= tification 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....

"gnu89 semantics of inline and extern inline= &nbs= p;ar= e essentially the exact opposite of those in C99[4], with = the exception that gnu89 permits redefinition of an extern inline function as an = unqualified function, while C99 inline does not[5]. Thus, gnu89 extern inline without redefinition is like C99 inline<= /code>, and= gnu89 inline is like C99 extern inline; in other words, in gnu89, a function defined= &nbs= p;in= line=  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 <= a href=3D"https://en.wikipedia.org/wiki/Principle_of_least_astonishment" ti= tle=3D"Principle of least astonishment" style=3D"caret-color: rgb(34, 34, 3= 4); font-family: sans-serif; font-size: 14.000000953674316px; text-decorati= on: none; color: rgb(11, 0, 128); background-image: none;" class=3D"">aston= ishing&= nbsp;if using inline would have a side-effect=E2=80=94to always emit a non-inlined = version of the function=E2=80=94that is contrary to what its name suggests.=
The remarks for C99 about the nee= d to provide exactly one externally visible function instance for inlined f= unctions and about the resulting problem with unreachable code apply mutati= s mutandis to gnu89 as well.
gcc up to and including version 4.2 used gnu89<= /span> = ;inl= ine&= nbsp;semantics even when -std=3Dc99 was explicitly specified.[6]&nbs= p;Wi= th version 5[5], gcc switche= d from gnu89 to the gnu11 dialect, effectively enabling C99 inline semantics by d= efault. To use gnu89 semantics instead, they have to be enabled explicitly,= either with -std=3Dgnu89 or, to only affect inlining, -fgnu89-inline, or by adding the<= /span> = ;gnu= _inline attribute to all inline declarations. To ensure C99 semantics, either -std=3D= c99,= &nbs= p;-s= td=3Dc11, -std=3Dgnu99 or -std=3Dgnu11 (without -fgnu89-inline) can be used.[3]"

= And the above makes you look at the C99 definition "In C99, a function defined inline will never, and a func= tion defined extern inline will always, emit an externally visibl= e 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 __inlin= e__ to fix the clang issue?

It also see= ms strange to me the __inline__ only exists next to the library function. G= iven it is not in the header (and the code is not in the header) I'm not re= ally sure what the compiler can do? When the BaseIoLibIntrinsic librar= y gets compiled it is going to create the intrinsic functions. It seems lik= e 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__ h= elps. Does the compiler tag the object with some kind of hint? If you are d= oing 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/BaseIo= LibIntrinsic/IoLibGcc.c:35:= __inline__
Library/BaseIoLibIntrinsic/IoLibGcc.= c:63:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:90:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:120:__inline__
Library/BaseIoLibI= ntrinsic/IoLibGcc.c:148:__inline__
Library/BaseIoLibIntrinsic/IoLibGcc.c:178:__inline__


Thanks,

Andrew Fish

Thanks
Laszlo

diff --git a/M= dePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c b/MdePkg/Library/BaseIoLibIntri= nsic/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
EF= IAPI
IoRead8 (
@@ -60,7 +59,6 @@ IoRead8 (
  @return The value written the I/O port.

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

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

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

**/
-__inline__
UINT32EFIAPI
IoRead32 (
@@ -175,7 +170,6 = @@ IoRead32 (
  @return The value written the I/O p= ort.

**/
-__inline__
UINT32
EFIAPI
IoWrite32 (




--Apple-Mail=_57E6F75C-A539-47D7-A87F-AA1FA75638F1--