[edk2,edk2-staging,10/20] IntelUndiPkg/XGigUndiDxe: drop StdLibC library class reference

Message ID 20181115023353.20159-11-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • IntelUndiPkg/XGigUndiDxe: fix GCC / ARM build issues
Related show

Commit Message

Ard Biesheuvel Nov. 15, 2018, 2:33 a.m.
StdLibc should not be used in drivers (it has dependencies on Shell
protocols), but in fact, we don't appear to rely on it in the first
place, so just drop the reference.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -
 1 file changed, 1 deletion(-)

-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Carsey, Jaben Nov. 15, 2018, 3:16 p.m. | #1
Agreed.  
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>


> -----Original Message-----

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of

> Ard Biesheuvel

> Sent: Wednesday, November 14, 2018 6:34 PM

> To: edk2-devel@lists.01.org

> Cc: Kacperski, Kamil <kamil.kacperski@intel.com>; Jin, Eric

> <eric.jin@intel.com>; Orlowski, Pawel <pawel.orlowski@intel.com>; Kinney,

> Michael D <michael.d.kinney@intel.com>; Hsiung, Harry L

> <harry.l.hsiung@intel.com>

> Subject: [edk2] [PATCH edk2-staging 10/20] IntelUndiPkg/XGigUndiDxe: drop

> StdLibC library class reference

> Importance: High

> 

> StdLibc should not be used in drivers (it has dependencies on Shell

> protocols), but in fact, we don't appear to rely on it in the first

> place, so just drop the reference.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -

>  1 file changed, 1 deletion(-)

> 

> diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> index beee8aa8134e..b5747565fbea 100644

> --- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> +++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> @@ -132,7 +132,6 @@ GCC:*_*_*_CC_FLAGS = -DEFI32

>    PrintLib

>    UefiLib

>    HiiLib

> -  StdLibC

> 

>  [LibraryClasses.X64]

> 

> --

> 2.17.1

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryszard Knop Jan. 30, 2019, 5:26 p.m. | #2
That's actually not quite correct - we need this package to build on
IA32. It's named rather unfortunately, since it's not the EDK2 StdLibC,
but rather a package in this repository - see IntelUndiPkg/LibC. It
contains the bare minimum of functionality required to fix missing 64-
bit math/shifts on IA32 and missing memcpy/memset intrinsics. We can't
prevent MSVC from yielding memcpy/memset either, so this was the nasty
solution for build issues. You have included CompilerIntrinsicsLib for
the same reason, too :)

I'm not aware of any X64/IA32 equivalent of your CompilerIntrinsicsLib,
but I'd be happy to be proven wrong here. I'm off for the rest of the
week - I'll continue with reviews and merging early next week.

Thanks, Richard.

On Wed, 2018-11-14 at 18:33 -0800, ard.biesheuvela wrote:
> StdLibc should not be used in drivers (it has dependencies on Shell

> protocols), but in fact, we don't appear to rely on it in the first

> place, so just drop the reference.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>

> ---

>  IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -

>  1 file changed, 1 deletion(-)

> 

> diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> index beee8aa8134e..b5747565fbea 100644

> --- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> +++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> @@ -132,7 +132,6 @@ GCC:*_*_*_CC_FLAGS = -DEFI32

>    PrintLib

>    UefiLib

>    HiiLib

> -  StdLibC

>  

>  [LibraryClasses.X64]

>    


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Andrew Fish via edk2-devel Jan. 30, 2019, 6:34 p.m. | #3
> On Jan 30, 2019, at 9:26 AM, Ryszard Knop <ryszard.knop@linux.intel.com> wrote:

> 

> That's actually not quite correct - we need this package to build on

> IA32. It's named rather unfortunately, since it's not the EDK2 StdLibC,

> but rather a package in this repository - see IntelUndiPkg/LibC. It

> contains the bare minimum of functionality required to fix missing 64-

> bit math/shifts on IA32 and missing memcpy/memset intrinsics. We can't

> prevent MSVC from yielding memcpy/memset either, so this was the nasty

> solution for build issues. You have included CompilerIntrinsicsLib for

> the same reason, too :)

> 


Ryszard,

For IA32/X64 we avoid the compiler intrinsic libs via the coding standard. 
1) If you don't assign something too large at execution time with an = the compiler will not inline memcpy()/memset()
2) BaseLib.h has all the math functions that generate intrinsics that your code can call explicitly: https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/BaseLib.h#L3533 <https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/BaseLib.h#L3533>

UINT64 X=0x100000000;
UINT64 Y=2;

So:
Y = X*Y;
should be:
Y = MultU64x64 (X, Y);

When ARM got added much later and some versions of ARM did not even have a divide instruction we gave up on trying to add more functions into all the existing IA32 code, and add the intrinsic lib. 

If we are going to add an intrinsic lib for x86 then we should probably add it to the MdePkg and it needs to support MSVC and GCC (as far as I can tell clang should work with the GCC intrinsics). 

Thanks,

Andrew Fish


> I'm not aware of any X64/IA32 equivalent of your CompilerIntrinsicsLib,

> but I'd be happy to be proven wrong here. I'm off for the rest of the

> week - I'll continue with reviews and merging early next week.

> 

> Thanks, Richard.

> 

> On Wed, 2018-11-14 at 18:33 -0800, ard.biesheuvela wrote:

>> StdLibc should not be used in drivers (it has dependencies on Shell

>> protocols), but in fact, we don't appear to rely on it in the first

>> place, so just drop the reference.

>> 

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>

>> ---

>> IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -

>> 1 file changed, 1 deletion(-)

>> 

>> diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

>> b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

>> index beee8aa8134e..b5747565fbea 100644

>> --- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

>> +++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

>> @@ -132,7 +132,6 @@ GCC:*_*_*_CC_FLAGS = -DEFI32

>>   PrintLib

>>   UefiLib

>>   HiiLib

>> -  StdLibC

>> 

>> [LibraryClasses.X64]

>> 

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Kinney, Michael D Jan. 30, 2019, 8:58 p.m. | #4
Hi Richard,

It is possible to update C code to prevent the use of compiler
intrinsic functions.  This is what we have done for EDK II modules
that are built for both IA32 and X64.

The one exception is the use of OpenSSL.  We prefer to use that
project "as is" as a git submodule, so modifying the OpenSSL
sources to prevent use of intrinsic functions was not practical.
The CryptoPkg has the minimum set of intrinsic functions required
For OpenSSL to build.

We do recognize that this issue is difficult for developers to
resolve because the techniques require generation of mixed C/asm
output files to track down the C statements that are generating
the intrinsic functions.

Similar to the ARM support for an intrinsic lib, it may be 
reasonable to add a small intrinsic lib for IA32 and X64 that
supports the intrinsic functions that are seen the most often.
May  require an intrinsic lib for each supported tool chain.

This would be a new feature that would take some effort to 
implement and validate.  Can you enter an Bugzilla for this
feature and list the specific intrinsic functions needed for
this driver?

Thanks,

Mike

> -----Original Message-----

> From: Ryszard Knop

> [mailto:ryszard.knop@linux.intel.com]

> Sent: Wednesday, January 30, 2019 9:27 AM

> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-

> devel@lists.01.org; Carsey, Jaben

> <jaben.carsey@intel.com>

> Cc: Kacperski, Kamil <kamil.kacperski@intel.com>; Jin,

> Eric <eric.jin@intel.com>; Orlowski, Pawel

> <pawel.orlowski@intel.com>; Kinney, Michael D

> <michael.d.kinney@intel.com>; Hsiung, Harry L

> <harry.l.hsiung@intel.com>

> Subject: Re: [edk2] [PATCH edk2-staging 10/20]

> IntelUndiPkg/XGigUndiDxe: drop StdLibC library class

> reference

> 

> That's actually not quite correct - we need this

> package to build on

> IA32. It's named rather unfortunately, since it's not

> the EDK2 StdLibC,

> but rather a package in this repository - see

> IntelUndiPkg/LibC. It

> contains the bare minimum of functionality required to

> fix missing 64-

> bit math/shifts on IA32 and missing memcpy/memset

> intrinsics. We can't

> prevent MSVC from yielding memcpy/memset either, so

> this was the nasty

> solution for build issues. You have included

> CompilerIntrinsicsLib for

> the same reason, too :)

> 

> I'm not aware of any X64/IA32 equivalent of your

> CompilerIntrinsicsLib,

> but I'd be happy to be proven wrong here. I'm off for

> the rest of the

> week - I'll continue with reviews and merging early

> next week.

> 

> Thanks, Richard.

> 

> On Wed, 2018-11-14 at 18:33 -0800, ard.biesheuvela

> wrote:

> > StdLibc should not be used in drivers (it has

> dependencies on Shell

> > protocols), but in fact, we don't appear to rely on

> it in the first

> > place, so just drop the reference.

> >

> > Contributed-under: TianoCore Contribution Agreement

> 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel at

> linaro.org>

> > ---

> >  IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -

> >  1 file changed, 1 deletion(-)

> >

> > diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > index beee8aa8134e..b5747565fbea 100644

> > --- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > +++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > @@ -132,7 +132,6 @@ GCC:*_*_*_CC_FLAGS = -DEFI32

> >    PrintLib

> >    UefiLib

> >    HiiLib

> > -  StdLibC

> >

> >  [LibraryClasses.X64]

> >


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryszard Knop Feb. 6, 2019, 9:46 a.m. | #5
Andrew,

Unfortunately, not assigning something too large or using math
functions is not an option for us, as we share a significant amount of
code with Linux/FreeBSD drivers and maintainers of that code don't want
changes similar to the ones below (especially that, for all the other
drivers, intrinsics just work). Intrinsic lib for IA32 and others would
be very much preferred (and one that just works on any architecture, so
that we wouldn't have to add extra arch-specific LibraryClasses).

Thanks, Richard

On Wed, 2019-01-30 at 10:34 -0800, Andrew Fish wrote:
> > On Jan 30, 2019, at 9:26 AM, Ryszard Knop <

> > ryszard.knop@linux.intel.com> wrote:

> > 

> > That's actually not quite correct - we need this package to build

> > on

> > IA32. It's named rather unfortunately, since it's not the EDK2

> > StdLibC,

> > but rather a package in this repository - see IntelUndiPkg/LibC. It

> > contains the bare minimum of functionality required to fix missing

> > 64-

> > bit math/shifts on IA32 and missing memcpy/memset intrinsics. We

> > can't

> > prevent MSVC from yielding memcpy/memset either, so this was the

> > nasty

> > solution for build issues. You have included CompilerIntrinsicsLib

> > for

> > the same reason, too :)

> > 

> 

> Ryszard,

> 

> For IA32/X64 we avoid the compiler intrinsic libs via the coding

> standard. 

> 1) If you don't assign something too large at execution time with an

> = the compiler will not inline memcpy()/memset()

> 2) BaseLib.h has all the math functions that generate intrinsics that

> your code can call explicitly: 

> https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/BaseLib.h#L3533

> 

> UINT64 X=0x100000000;

> UINT64 Y=2;

> 

> So:

> Y = X*Y;

> should be:

> Y = MultU64x64 (X, Y);

> 

> When ARM got added much later and some versions of ARM did not even

> have a divide instruction we gave up on trying to add more functions

> into all the existing IA32 code, and add the intrinsic lib. 

> 

> If we are going to add an intrinsic lib for x86 then we should

> probably add it to the MdePkg and it needs to support MSVC and GCC

> (as far as I can tell clang should work with the GCC intrinsics). 

> 

> Thanks,

> 

> Andrew Fish

> 

> 

> > I'm not aware of any X64/IA32 equivalent of your

> > CompilerIntrinsicsLib,

> > but I'd be happy to be proven wrong here. I'm off for the rest of

> > the

> > week - I'll continue with reviews and merging early next week.

> > 

> > Thanks, Richard.

> > 

> > On Wed, 2018-11-14 at 18:33 -0800, ard.biesheuvela wrote:

> > > StdLibc should not be used in drivers (it has dependencies on

> > > Shell

> > > protocols), but in fact, we don't appear to rely on it in the

> > > first

> > > place, so just drop the reference.

> > > 

> > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>

> > > ---

> > > IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -

> > > 1 file changed, 1 deletion(-)

> > > 

> > > diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > index beee8aa8134e..b5747565fbea 100644

> > > --- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > +++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > @@ -132,7 +132,6 @@ GCC:*_*_*_CC_FLAGS = -DEFI32

> > >   PrintLib

> > >   UefiLib

> > >   HiiLib

> > > -  StdLibC

> > > 

> > > [LibraryClasses.X64]

> > > 

> > 

> > _______________________________________________

> > edk2-devel mailing list

> > edk2-devel@lists.01.org

> > https://lists.01.org/mailman/listinfo/edk2-devel

> 

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryszard Knop Feb. 6, 2019, 10:14 a.m. | #6
Mike,

As mentioned in previous mails, we can't change some of our code to
meet the coding standard. I've filled BZ #1516 with everything we need,
plus what CryptoPkg provides for reference.

Thanks, Richard.

On Wed, 2019-01-30 at 20:58 +0000, Kinney, Michael D wrote:
> Hi Richard,

> 

> It is possible to update C code to prevent the use of compiler

> intrinsic functions.  This is what we have done for EDK II modules

> that are built for both IA32 and X64.

> 

> The one exception is the use of OpenSSL.  We prefer to use that

> project "as is" as a git submodule, so modifying the OpenSSL

> sources to prevent use of intrinsic functions was not practical.

> The CryptoPkg has the minimum set of intrinsic functions required

> For OpenSSL to build.

> 

> We do recognize that this issue is difficult for developers to

> resolve because the techniques require generation of mixed C/asm

> output files to track down the C statements that are generating

> the intrinsic functions.

> 

> Similar to the ARM support for an intrinsic lib, it may be 

> reasonable to add a small intrinsic lib for IA32 and X64 that

> supports the intrinsic functions that are seen the most often.

> May  require an intrinsic lib for each supported tool chain.

> 

> This would be a new feature that would take some effort to 

> implement and validate.  Can you enter an Bugzilla for this

> feature and list the specific intrinsic functions needed for

> this driver?

> 

> Thanks,

> 

> Mike

> 

> > -----Original Message-----

> > From: Ryszard Knop

> > [mailto:ryszard.knop@linux.intel.com]

> > Sent: Wednesday, January 30, 2019 9:27 AM

> > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-

> > devel@lists.01.org; Carsey, Jaben

> > <jaben.carsey@intel.com>

> > Cc: Kacperski, Kamil <kamil.kacperski@intel.com>; Jin,

> > Eric <eric.jin@intel.com>; Orlowski, Pawel

> > <pawel.orlowski@intel.com>; Kinney, Michael D

> > <michael.d.kinney@intel.com>; Hsiung, Harry L

> > <harry.l.hsiung@intel.com>

> > Subject: Re: [edk2] [PATCH edk2-staging 10/20]

> > IntelUndiPkg/XGigUndiDxe: drop StdLibC library class

> > reference

> > 

> > That's actually not quite correct - we need this

> > package to build on

> > IA32. It's named rather unfortunately, since it's not

> > the EDK2 StdLibC,

> > but rather a package in this repository - see

> > IntelUndiPkg/LibC. It

> > contains the bare minimum of functionality required to

> > fix missing 64-

> > bit math/shifts on IA32 and missing memcpy/memset

> > intrinsics. We can't

> > prevent MSVC from yielding memcpy/memset either, so

> > this was the nasty

> > solution for build issues. You have included

> > CompilerIntrinsicsLib for

> > the same reason, too :)

> > 

> > I'm not aware of any X64/IA32 equivalent of your

> > CompilerIntrinsicsLib,

> > but I'd be happy to be proven wrong here. I'm off for

> > the rest of the

> > week - I'll continue with reviews and merging early

> > next week.

> > 

> > Thanks, Richard.

> > 

> > On Wed, 2018-11-14 at 18:33 -0800, ard.biesheuvela

> > wrote:

> > > StdLibc should not be used in drivers (it has

> > dependencies on Shell

> > > protocols), but in fact, we don't appear to rely on

> > it in the first

> > > place, so just drop the reference.

> > > 

> > > Contributed-under: TianoCore Contribution Agreement

> > 1.1

> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel at

> > linaro.org>

> > > ---

> > >  IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -

> > >  1 file changed, 1 deletion(-)

> > > 

> > > diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > index beee8aa8134e..b5747565fbea 100644

> > > --- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > +++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf

> > > @@ -132,7 +132,6 @@ GCC:*_*_*_CC_FLAGS = -DEFI32

> > >    PrintLib

> > >    UefiLib

> > >    HiiLib

> > > -  StdLibC

> > > 

> > >  [LibraryClasses.X64]

> > > 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf
index beee8aa8134e..b5747565fbea 100644
--- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf
+++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf
@@ -132,7 +132,6 @@  GCC:*_*_*_CC_FLAGS = -DEFI32
   PrintLib
   UefiLib
   HiiLib
-  StdLibC
 
 [LibraryClasses.X64]