* Re: [PATCH v1 1/1] [CryptoPkg] Add CC flags for MSVC ARM on IntrinsicLib
2020-10-07 18:41 ` [PATCH v1 1/1] [CryptoPkg] Add CC flags for MSVC ARM on IntrinsicLib Matthew Carlson
@ 2020-10-07 18:44 ` Ard Biesheuvel
2020-10-08 0:26 ` [edk2-devel] " Yao, Jiewen
2020-10-08 11:13 ` Leif Lindholm
2020-10-08 11:33 ` IntrinsicsLib - Was: " Leif Lindholm
2 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2020-10-07 18:44 UTC (permalink / raw)
To: matthewfcarlson, devel
Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu, Guomin Jiang, Leif Lindholm
On 10/7/20 8:41 PM, matthewfcarlson@gmail.com wrote:
> From: Matthew Carlson <matthewfcarlson@gmail.com>
>
> This adds compiler flags for AARCH64 and ARM for the Visual Studio
> compilers to the IntrinsicLib inf.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2821
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
Hi Matt,
Please don't put the package name in [] in the subject line. That will
cause 'git am' to strip it when applying the patch.
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> index fcbb93316cf7..ad05b3a000c4 100644
> --- a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +++ b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -64,4 +64,6 @@
> MSFT:RELEASE_*_IA32_CC_FLAGS == /nologo /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /FIAutoGen.h /EHs-c- /GR- /GF
> MSFT:DEBUG_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> MSFT:RELEASE_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
> - INTEL:*_*_*_CC_FLAGS = /Oi-
> + MSFT:DEBUG_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> + MSFT:RELEASE_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
> + INTEL:*_*_*_CC_FLAGS = /Oi-
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] [CryptoPkg] Add CC flags for MSVC ARM on IntrinsicLib
2020-10-07 18:44 ` Ard Biesheuvel
@ 2020-10-08 0:26 ` Yao, Jiewen
0 siblings, 0 replies; 6+ messages in thread
From: Yao, Jiewen @ 2020-10-08 0:26 UTC (permalink / raw)
To: devel@edk2.groups.io, ard.biesheuvel@arm.com,
matthewfcarlson@gmail.com
Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Leif Lindholm
Need ARM or compiler expert to give R-B.
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Thursday, October 8, 2020 2:45 AM
> To: matthewfcarlson@gmail.com; devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>;
> Leif Lindholm <leif@nuviainc.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] [CryptoPkg] Add CC flags for MSVC
> ARM on IntrinsicLib
>
> On 10/7/20 8:41 PM, matthewfcarlson@gmail.com wrote:
> > From: Matthew Carlson <matthewfcarlson@gmail.com>
> >
> > This adds compiler flags for AARCH64 and ARM for the Visual Studio
> > compilers to the IntrinsicLib inf.
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2821
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
>
> Hi Matt,
>
> Please don't put the package name in [] in the subject line. That will
> cause 'git am' to strip it when applying the patch.
>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
>
> > ---
> > CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> > index fcbb93316cf7..ad05b3a000c4 100644
> > --- a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> > +++ b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> > @@ -64,4 +64,6 @@
> > MSFT:RELEASE_*_IA32_CC_FLAGS == /nologo /c /WX /GS- /W4
> /Gs32768 /D UNICODE /O1b2 /FIAutoGen.h /EHs-c- /GR- /GF
> > MSFT:DEBUG_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4
> /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> > MSFT:RELEASE_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4
> /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
> > - INTEL:*_*_*_CC_FLAGS = /Oi-
> > + MSFT:DEBUG_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4
> /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> > + MSFT:RELEASE_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4
> /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
> > + INTEL:*_*_*_CC_FLAGS = /Oi-
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] [CryptoPkg] Add CC flags for MSVC ARM on IntrinsicLib
2020-10-07 18:41 ` [PATCH v1 1/1] [CryptoPkg] Add CC flags for MSVC ARM on IntrinsicLib Matthew Carlson
2020-10-07 18:44 ` Ard Biesheuvel
@ 2020-10-08 11:13 ` Leif Lindholm
2020-10-08 11:33 ` IntrinsicsLib - Was: " Leif Lindholm
2 siblings, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2020-10-08 11:13 UTC (permalink / raw)
To: matthewfcarlson
Cc: devel, Jiewen Yao, Jian J Wang, Xiaoyu Lu, Guomin Jiang,
Ard Biesheuvel
I have some minor technical comments on this patch, and then I'll
follow up with a separate reply on the topic of how we appear to have
duplicated (at least) intrinsics libraries with overlapping
functionality in the tree...
On Wed, Oct 07, 2020 at 11:41:45 -0700, matthewfcarlson@gmail.com wrote:
> From: Matthew Carlson <matthewfcarlson@gmail.com>
>
> This adds compiler flags for AARCH64 and ARM for the Visual Studio
> compilers to the IntrinsicLib inf.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2821
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> ---
> CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> index fcbb93316cf7..ad05b3a000c4 100644
> --- a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +++ b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -64,4 +64,6 @@
> MSFT:RELEASE_*_IA32_CC_FLAGS == /nologo /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /FIAutoGen.h /EHs-c- /GR- /GF
> MSFT:DEBUG_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> MSFT:RELEASE_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
> - INTEL:*_*_*_CC_FLAGS = /Oi-
This whitespace-only change is unrelated to the patch context. Please
submit it separately, or drop altogether.
(On a separate note, that line is the only correctly indented - 2
spaces - line in the whole block; if anything should change it should
be all the others. So if resubmitting, please use 2 spaces for added lines.)
> + MSFT:DEBUG_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> + MSFT:RELEASE_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
More a comment than a request: CryptoPkg.dsc itself supports the NOOPT
build target, and the majority of the platforms in edk2-platforms do
as well - if adding these, could we have a line for the NOOPT target
as well?
Best Regards,
Leif
> + INTEL:*_*_*_CC_FLAGS = /Oi-
> --
> 2.28.0.vfs.0.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* IntrinsicsLib - Was: [CryptoPkg] Add CC flags for MSVC ARM on IntrinsicLib
2020-10-07 18:41 ` [PATCH v1 1/1] [CryptoPkg] Add CC flags for MSVC ARM on IntrinsicLib Matthew Carlson
2020-10-07 18:44 ` Ard Biesheuvel
2020-10-08 11:13 ` Leif Lindholm
@ 2020-10-08 11:33 ` Leif Lindholm
2 siblings, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2020-10-08 11:33 UTC (permalink / raw)
To: matthewfcarlson
Cc: devel, Ard Biesheuvel, Andrew Fish, Laszlo Ersek,
Michael D Kinney
So, here is the second email I promised... (Dropping CryptoPkg
maintainers, adding stewards).
We have some (to my mind) spurious rules in TianoCore - some
documented in the Coding Style document, and some documented nowhere
yet continuously enforced - with the common factor of attempting to
prevent calls out to compiler intrinsic functions being generated for
things such as:
- structure assignment or comparison
- *some* 64-bit arithmetic and logic operations
Now, on the ARM side we bit the bullet in the early days and
Implemented ArmPkg/Library/CompilerIntrinsicsLib to cope with
this. But we still end up with partial duplication of this nested
underneath CryptoPkg (and working around parts of the CryptoPkg one in
the ArmPkg one).
Now, the various bits of ArmPkg should start migrating into MdePkg
over time, and the CompilerIntrinsicsLibrary as part of that.
Could we reconsider this *partial* ban on intrinsic-generating valid C
in Ia32/X64, and create a single unified intrinsics library?
/
Leif
On Wed, Oct 07, 2020 at 11:41:45 -0700, matthewfcarlson@gmail.com wrote:
> From: Matthew Carlson <matthewfcarlson@gmail.com>
>
> This adds compiler flags for AARCH64 and ARM for the Visual Studio
> compilers to the IntrinsicLib inf.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2821
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> ---
> CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> index fcbb93316cf7..ad05b3a000c4 100644
> --- a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +++ b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -64,4 +64,6 @@
> MSFT:RELEASE_*_IA32_CC_FLAGS == /nologo /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /FIAutoGen.h /EHs-c- /GR- /GF
> MSFT:DEBUG_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> MSFT:RELEASE_*_X64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
> - INTEL:*_*_*_CC_FLAGS = /Oi-
> + MSFT:DEBUG_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Zi /Gm
> + MSFT:RELEASE_*_AARCH64_CC_FLAGS == /nologo /c /WX /GS- /X /W4 /Gs32768 /D UNICODE /O1b2s /Gy /FIAutoGen.h /EHs-c- /GR- /GF
> + INTEL:*_*_*_CC_FLAGS = /Oi-
> --
> 2.28.0.vfs.0.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread