* [PATCH v1 0/1] StandaloneMmPkg: To support CLANGPDB build
@ 2021-10-14 3:12 jiyangx.yang
2021-10-14 3:12 ` [PATCH v1 1/1] " Jiyang Yang
0 siblings, 1 reply; 8+ messages in thread
From: jiyangx.yang @ 2021-10-14 3:12 UTC (permalink / raw)
To: devel
Cc: Jiyang Yang, Ard Biesheuvel, Sami Mujawar, Jiewen Yao,
Supreeth Venkatesh, Vitaly Cheptsov, Marvin Häuser,
Steven Shi
the flag "-fpie" is passed for all builds with a GCC family toolchain,
including CLANGPDB, but CLANGPDB does not support this flag, it will
report "clang: error: unsupported option '-fpie' for target
'x86_64-unknown-windows-gnu'". So we add the CLANGPDB option "-fno-pie"
later to overwrite it.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Steven Shi <steven.shi@intel.com>
Signed-off-by: Jiyang Yang <jiyangx.yang@intel.com>
Jiyang Yang (1):
StandaloneMmPkg/StandaloneMmCoreEntryPoint: To support CLANGPDB build
StandaloneMmPkg/Core/StandaloneMmCore.inf | 2 ++
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 1 +
2 files changed, 3 insertions(+)
--
2.26.2.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build
2021-10-14 3:12 [PATCH v1 0/1] StandaloneMmPkg: To support CLANGPDB build jiyangx.yang
@ 2021-10-14 3:12 ` Jiyang Yang
2021-10-14 8:05 ` Marvin Häuser
0 siblings, 1 reply; 8+ messages in thread
From: Jiyang Yang @ 2021-10-14 3:12 UTC (permalink / raw)
To: devel
Cc: Jiyang Yang, Ard Biesheuvel, Sami Mujawar, Jiewen Yao,
Supreeth Venkatesh, Vitaly Cheptsov, Marvin Häuser,
Steven Shi
the flag "-fpie" is passed for all builds with a GCC family toolchain,
including CLANGPDB, but CLANGPDB does not support this flag, it will
report "clang: error: unsupported option '-fpie' for target
'x86_64-unknown-windows-gnu'". So we add the CLANGPDB option "-fno-pie"
later to overwrite it.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Steven Shi <steven.shi@intel.com>
Signed-off-by: Jiyang Yang <jiyangx.yang@intel.com>
---
StandaloneMmPkg/Core/StandaloneMmCore.inf | 2 ++
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 1 +
2 files changed, 3 insertions(+)
diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index 56042b7b39f4..3213142523f4 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -79,3 +79,5 @@
[BuildOptions]
GCC:*_*_*_CC_FLAGS = -fpie
GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
+ CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
+ CLANGPDB:*_*_*_DLINK_FLAGS =
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 1762586cfa02..ef69e07d2c07 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -56,3 +56,4 @@
[BuildOptions]
GCC:*_*_*_CC_FLAGS = -fpie
+ CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
--
2.26.2.windows.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build
2021-10-14 3:12 ` [PATCH v1 1/1] " Jiyang Yang
@ 2021-10-14 8:05 ` Marvin Häuser
2021-10-14 9:02 ` [edk2-devel] " Steven Shi
0 siblings, 1 reply; 8+ messages in thread
From: Marvin Häuser @ 2021-10-14 8:05 UTC (permalink / raw)
To: Jiyang Yang, devel
Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Supreeth Venkatesh,
Vitaly Cheptsov, Steven Shi
Hey Jiyang,
NO! Please do not. :)
Yes, this fixes build, but the AARCH64 core (I did not check ARM)
depends on self-relocation as it is loaded in-place at a location
unknown at compile-time. PIE helps ensure there are no relocations in
.text among other things. I know CLANGPDB does not support ARM/AARCH64
yet, but if it is added, this may generate binaries with more dangerous
relocations, which means the chance of executing an instruction that
requires relocation without relocating first (relocation is done in C
code now!) is significantly higher. We do not need PIE for IA32 or X64
at all (or more specifically, we only need it for ARM-based
architectures as of now), so I prefer my patch which makes that
explicit. Though we can theoretically use your solution when limited to
non-ARM architectures if you really dislike my patch that much.
I'd prefer to hear from the ARM core maintainers before making any move.
Best regards,
Marvin
On 14.10.21 05:12, Jiyang Yang wrote:
> the flag "-fpie" is passed for all builds with a GCC family toolchain,
> including CLANGPDB, but CLANGPDB does not support this flag, it will
> report "clang: error: unsupported option '-fpie' for target
> 'x86_64-unknown-windows-gnu'". So we add the CLANGPDB option "-fno-pie"
> later to overwrite it.
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Steven Shi <steven.shi@intel.com>
> Signed-off-by: Jiyang Yang <jiyangx.yang@intel.com>
> ---
> StandaloneMmPkg/Core/StandaloneMmCore.inf | 2 ++
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> index 56042b7b39f4..3213142523f4 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> @@ -79,3 +79,5 @@
> [BuildOptions]
> GCC:*_*_*_CC_FLAGS = -fpie
> GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
> + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
> + CLANGPDB:*_*_*_DLINK_FLAGS =
> diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> index 1762586cfa02..ef69e07d2c07 100644
> --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> @@ -56,3 +56,4 @@
>
> [BuildOptions]
> GCC:*_*_*_CC_FLAGS = -fpie
> + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build
2021-10-14 8:05 ` Marvin Häuser
@ 2021-10-14 9:02 ` Steven Shi
2021-10-14 9:08 ` Marvin Häuser
0 siblings, 1 reply; 8+ messages in thread
From: Steven Shi @ 2021-10-14 9:02 UTC (permalink / raw)
To: devel@edk2.groups.io, mhaeuser@posteo.de, Yang, JiyangX
Cc: Ard Biesheuvel, Sami Mujawar, Yao, Jiewen, Supreeth Venkatesh,
Vitaly Cheptsov
[-- Attachment #1: Type: text/plain, Size: 4860 bytes --]
Hi Marvin,
How about we limit the -fno-pie option only apply on IA32 and X64 like below?
diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
[BuildOptions]
GCC:*_*_*_CC_FLAGS = -fpie
GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
+ CLANGPDB:*_*_ IA32_CC_FLAGS= -fno-pie
+ CLANGPDB:*_*_ X64_CC_FLAGS= -fno-pie
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
[BuildOptions]
GCC:*_*_*_CC_FLAGS = -fpie
+ CLANGPDB:*_*_ IA32_CC_FLAGS= -fno-pie
+ CLANGPDB:*_*_ X64_CC_FLAGS= -fno-pie
Thanks
Steven Shi
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
> Häuser
> Sent: Thursday, October 14, 2021 4:05 PM
> To: Yang, JiyangX <jiyangx.yang@intel.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Supreeth
> Venkatesh <supreeth.venkatesh@arm.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>; Shi, Steven <steven.shi@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support
> CLANGPDB build
>
> Hey Jiyang,
>
> NO! Please do not. :)
> Yes, this fixes build, but the AARCH64 core (I did not check ARM)
> depends on self-relocation as it is loaded in-place at a location
> unknown at compile-time. PIE helps ensure there are no relocations in
> .text among other things. I know CLANGPDB does not support
> ARM/AARCH64
> yet, but if it is added, this may generate binaries with more dangerous
> relocations, which means the chance of executing an instruction that
> requires relocation without relocating first (relocation is done in C
> code now!) is significantly higher. We do not need PIE for IA32 or X64
> at all (or more specifically, we only need it for ARM-based
> architectures as of now), so I prefer my patch which makes that
> explicit. Though we can theoretically use your solution when limited to
> non-ARM architectures if you really dislike my patch that much.
>
> I'd prefer to hear from the ARM core maintainers before making any move.
>
> Best regards,
> Marvin
>
> On 14.10.21 05:12, Jiyang Yang wrote:
> > the flag "-fpie" is passed for all builds with a GCC family toolchain,
> > including CLANGPDB, but CLANGPDB does not support this flag, it will
> > report "clang: error: unsupported option '-fpie' for target
> > 'x86_64-unknown-windows-gnu'". So we add the CLANGPDB option "-fno-
> pie"
> > later to overwrite it.
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
> > Cc: Sami Mujawar <sami.mujawar@arm.com<mailto:sami.mujawar@arm.com>>
> > Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com<mailto:supreeth.venkatesh@arm.com>>
> > Cc: Vitaly Cheptsov <vit9696@protonmail.com<mailto:vit9696@protonmail.com>>
> > Cc: Marvin Häuser <mhaeuser@posteo.de<mailto:mhaeuser@posteo.de>>
> > Cc: Steven Shi <steven.shi@intel.com<mailto:steven.shi@intel.com>>
> > Signed-off-by: Jiyang Yang <jiyangx.yang@intel.com<mailto:jiyangx.yang@intel.com>>
> > ---
> > StandaloneMmPkg/Core/StandaloneMmCore.inf | 2
> ++
> >
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmC
> oreEntryPoint.inf | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > index 56042b7b39f4..3213142523f4 100644
> > --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > @@ -79,3 +79,5 @@
> > [BuildOptions]
> > GCC:*_*_*_CC_FLAGS = -fpie
> > GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
> > + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
> > + CLANGPDB:*_*_*_DLINK_FLAGS =
> > diff --git
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> CoreEntryPoint.inf
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> CoreEntryPoint.inf
> > index 1762586cfa02..ef69e07d2c07 100644
> > ---
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> CoreEntryPoint.inf
> > +++
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> CoreEntryPoint.inf
> > @@ -56,3 +56,4 @@
> >
> > [BuildOptions]
> > GCC:*_*_*_CC_FLAGS = -fpie
> > + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
>
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 11360 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build
2021-10-14 9:02 ` [edk2-devel] " Steven Shi
@ 2021-10-14 9:08 ` Marvin Häuser
2021-10-14 9:09 ` Ard Biesheuvel
2021-10-14 13:14 ` Steven Shi
0 siblings, 2 replies; 8+ messages in thread
From: Marvin Häuser @ 2021-10-14 9:08 UTC (permalink / raw)
To: Shi, Steven, devel@edk2.groups.io, Yang, JiyangX
Cc: Ard Biesheuvel, Sami Mujawar, Yao, Jiewen, Supreeth Venkatesh,
Vitaly Cheptsov
Hey Steven,
As I said, I prefer my patch, but this would work too of course.
I talked about the PIE stuff with Ard before, so maybe he has an opinion
on this? :)
(Small correction for my last e-mail, of course we are not *guaranteed*
there are *no* relocations in .text, but they'd all point to GOT (or
whatever else the target uses for PIE), and references will probably be
relative; for ARM architectures I remember Ard talking about specific
kinds of relocations being avoided entirely).
Best regards,
Marvin
On 14.10.21 11:02, Shi, Steven wrote:
>
> Hi Marvin,
>
> How about we limit the -fno-pie option only apply on IA32 and X64 like
> below?
>
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> b/StandaloneMmPkg/Core/StandaloneMmCore.inf
>
> [BuildOptions]
>
> GCC:*_*_*_CC_FLAGS = -fpie
>
> GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
>
> + CLANGPDB:*_*_ *IA32*_CC_FLAGS= -fno-pie
>
> + CLANGPDB:*_*_ *X64*_CC_FLAGS= -fno-pie
>
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
>
> [BuildOptions]
>
> GCC:*_*_*_CC_FLAGS = -fpie
>
> + CLANGPDB:*_*_ *IA32*_CC_FLAGS= -fno-pie
>
> + CLANGPDB:*_*_ *X64*_CC_FLAGS= -fno-pie
>
> Thanks
>
> Steven Shi
>
> > -----Original Message-----
>
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
>
> > Häuser
>
> > Sent: Thursday, October 14, 2021 4:05 PM
>
> > To: Yang, JiyangX <jiyangx.yang@intel.com>; devel@edk2.groups.io
>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
>
> > <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Supreeth
>
> > Venkatesh <supreeth.venkatesh@arm.com>; Vitaly Cheptsov
>
> > <vit9696@protonmail.com>; Shi, Steven <steven.shi@intel.com>
>
> > Subject: Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support
>
> > CLANGPDB build
>
> >
>
> > Hey Jiyang,
>
> >
>
> > NO! Please do not. :)
>
> > Yes, this fixes build, but the AARCH64 core (I did not check ARM)
>
> > depends on self-relocation as it is loaded in-place at a location
>
> > unknown at compile-time. PIE helps ensure there are no relocations in
>
> > .text among other things. I know CLANGPDB does not support
>
> > ARM/AARCH64
>
> > yet, but if it is added, this may generate binaries with more dangerous
>
> > relocations, which means the chance of executing an instruction that
>
> > requires relocation without relocating first (relocation is done in C
>
> > code now!) is significantly higher. We do not need PIE for IA32 or X64
>
> > at all (or more specifically, we only need it for ARM-based
>
> > architectures as of now), so I prefer my patch which makes that
>
> > explicit. Though we can theoretically use your solution when limited to
>
> > non-ARM architectures if you really dislike my patch that much.
>
> >
>
> > I'd prefer to hear from the ARM core maintainers before making any move.
>
> >
>
> > Best regards,
>
> > Marvin
>
> >
>
> > On 14.10.21 05:12, Jiyang Yang wrote:
>
> > > the flag "-fpie" is passed for all builds with a GCC family toolchain,
>
> > > including CLANGPDB, but CLANGPDB does not support this flag, it will
>
> > > report "clang: error: unsupported option '-fpie' for target
>
> > > 'x86_64-unknown-windows-gnu'". So we add the CLANGPDB option "-fno-
>
> > pie"
>
> > > later to overwrite it.
>
> > >
>
> > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org
> <mailto:ardb+tianocore@kernel.org>>
>
> > > Cc: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>>
>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
>
> > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com
> <mailto:supreeth.venkatesh@arm.com>>
>
> > > Cc: Vitaly Cheptsov <vit9696@protonmail.com
> <mailto:vit9696@protonmail.com>>
>
> > > Cc: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
>
> > > Cc: Steven Shi <steven.shi@intel.com <mailto:steven.shi@intel.com>>
>
> > > Signed-off-by: Jiyang Yang <jiyangx.yang@intel.com
> <mailto:jiyangx.yang@intel.com>>
>
> > > ---
>
> > > StandaloneMmPkg/Core/StandaloneMmCore.inf | 2
>
> > ++
>
> > >
>
> > StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmC
>
> > oreEntryPoint.inf | 1 +
>
> > > 2 files changed, 3 insertions(+)
>
> > >
>
> > > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
>
> > b/StandaloneMmPkg/Core/StandaloneMmCore.inf
>
> > > index 56042b7b39f4..3213142523f4 100644
>
> > > --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
>
> > > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
>
> > > @@ -79,3 +79,5 @@
>
> > > [BuildOptions]
>
> > > GCC:*_*_*_CC_FLAGS = -fpie
>
> > > GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
>
> > > + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
>
> > > + CLANGPDB:*_*_*_DLINK_FLAGS =
>
> > > diff --git
>
> > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
>
> > CoreEntryPoint.inf
>
> > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
>
> > CoreEntryPoint.inf
>
> > > index 1762586cfa02..ef69e07d2c07 100644
>
> > > ---
>
> > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
>
> > CoreEntryPoint.inf
>
> > > +++
>
> > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
>
> > CoreEntryPoint.inf
>
> > > @@ -56,3 +56,4 @@
>
> > >
>
> > > [BuildOptions]
>
> > > GCC:*_*_*_CC_FLAGS = -fpie
>
> > > + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
>
> >
>
> >
>
> >
>
> >
>
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build
2021-10-14 9:08 ` Marvin Häuser
@ 2021-10-14 9:09 ` Ard Biesheuvel
2021-10-14 9:13 ` Marvin Häuser
2021-10-14 13:14 ` Steven Shi
1 sibling, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2021-10-14 9:09 UTC (permalink / raw)
To: Marvin Häuser
Cc: Shi, Steven, devel@edk2.groups.io, Yang, JiyangX, Ard Biesheuvel,
Sami Mujawar, Yao, Jiewen, Supreeth Venkatesh, Vitaly Cheptsov
On Thu, 14 Oct 2021 at 11:08, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hey Steven,
>
> As I said, I prefer my patch, but this would work too of course.
> I talked about the PIE stuff with Ard before, so maybe he has an opinion
> on this? :)
>
> (Small correction for my last e-mail, of course we are not *guaranteed*
> there are *no* relocations in .text, but they'd all point to GOT (or
> whatever else the target uses for PIE), and references will probably be
> relative; for ARM architectures I remember Ard talking about specific
> kinds of relocations being avoided entirely).
>
Hello all,
As I understand it, we are talking about a native PE/COFF toolchain
here that does not rely on GenFw for ELF to PE/COFF conversion, right?
If so, there is no way the self-relocation is going to work anyway, so
whether we pass -fpie or not for AArch64 is immaterial here.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build
2021-10-14 9:09 ` Ard Biesheuvel
@ 2021-10-14 9:13 ` Marvin Häuser
0 siblings, 0 replies; 8+ messages in thread
From: Marvin Häuser @ 2021-10-14 9:13 UTC (permalink / raw)
To: devel, ardb
Cc: Shi, Steven, Yang, JiyangX, Ard Biesheuvel, Sami Mujawar,
Yao, Jiewen, Supreeth Venkatesh, Vitaly Cheptsov
Hey Ard,
Thanks for commenting!
On 14.10.21 11:09, Ard Biesheuvel wrote:
> On Thu, 14 Oct 2021 at 11:08, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> Hey Steven,
>>
>> As I said, I prefer my patch, but this would work too of course.
>> I talked about the PIE stuff with Ard before, so maybe he has an opinion
>> on this? :)
>>
>> (Small correction for my last e-mail, of course we are not *guaranteed*
>> there are *no* relocations in .text, but they'd all point to GOT (or
>> whatever else the target uses for PIE), and references will probably be
>> relative; for ARM architectures I remember Ard talking about specific
>> kinds of relocations being avoided entirely).
>>
> Hello all,
>
> As I understand it, we are talking about a native PE/COFF toolchain
> here that does not rely on GenFw for ELF to PE/COFF conversion, right?
Yep.
> If so, there is no way the self-relocation is going to work anyway, so
> whether we pass -fpie or not for AArch64 is immaterial here.
It's not quite, because with -fpie it errors (also maybe PE/COFF PIE
will be supported some day?). We can also pass -fno-pie and have some
other way to kill compilation for CLANGPDB ARM/AARCH64, but it must not
just silently succeed when this is eventually supported.
Best regards,
Marvin
>
>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build
2021-10-14 9:08 ` Marvin Häuser
2021-10-14 9:09 ` Ard Biesheuvel
@ 2021-10-14 13:14 ` Steven Shi
1 sibling, 0 replies; 8+ messages in thread
From: Steven Shi @ 2021-10-14 13:14 UTC (permalink / raw)
To: Marvin Häuser, devel@edk2.groups.io, Yang, JiyangX
Cc: Ard Biesheuvel, Sami Mujawar, Yao, Jiewen, Supreeth Venkatesh,
Vitaly Cheptsov
Marvin,
It's fine to use your below patch to fix this issue. Please help to check-in it.
https://edk2.groups.io/g/devel/message/78894?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CStandaloneMmPkg%3A+Support+CLANGPDB+X64+builds%2C20%2C2%2C0%2C84754068
Thanks
Steven Shi
> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Thursday, October 14, 2021 5:08 PM
> To: Shi, Steven <steven.shi@intel.com>; devel@edk2.groups.io; Yang,
> JiyangX <jiyangx.yang@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Supreeth
> Venkatesh <supreeth.venkatesh@arm.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support
> CLANGPDB build
>
> Hey Steven,
>
> As I said, I prefer my patch, but this would work too of course.
> I talked about the PIE stuff with Ard before, so maybe he has an opinion
> on this? :)
>
> (Small correction for my last e-mail, of course we are not *guaranteed*
> there are *no* relocations in .text, but they'd all point to GOT (or
> whatever else the target uses for PIE), and references will probably be
> relative; for ARM architectures I remember Ard talking about specific
> kinds of relocations being avoided entirely).
>
> Best regards,
> Marvin
>
> On 14.10.21 11:02, Shi, Steven wrote:
> >
> > Hi Marvin,
> >
> > How about we limit the -fno-pie option only apply on IA32 and X64 like
> > below?
> >
> > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> >
> > [BuildOptions]
> >
> > GCC:*_*_*_CC_FLAGS = -fpie
> >
> > GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
> >
> > + CLANGPDB:*_*_ *IA32*_CC_FLAGS= -fno-pie
> >
> > + CLANGPDB:*_*_ *X64*_CC_FLAGS= -fno-pie
> >
> > diff --git
> >
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> CoreEntryPoint.inf
> >
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> CoreEntryPoint.inf
> >
> > [BuildOptions]
> >
> > GCC:*_*_*_CC_FLAGS = -fpie
> >
> > + CLANGPDB:*_*_ *IA32*_CC_FLAGS= -fno-pie
> >
> > + CLANGPDB:*_*_ *X64*_CC_FLAGS= -fno-pie
> >
> > Thanks
> >
> > Steven Shi
> >
> > > -----Original Message-----
> >
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Marvin
> >
> > > Häuser
> >
> > > Sent: Thursday, October 14, 2021 4:05 PM
> >
> > > To: Yang, JiyangX <jiyangx.yang@intel.com>; devel@edk2.groups.io
> >
> > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> >
> > > <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Supreeth
> >
> > > Venkatesh <supreeth.venkatesh@arm.com>; Vitaly Cheptsov
> >
> > > <vit9696@protonmail.com>; Shi, Steven <steven.shi@intel.com>
> >
> > > Subject: Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support
> >
> > > CLANGPDB build
> >
> > >
> >
> > > Hey Jiyang,
> >
> > >
> >
> > > NO! Please do not. :)
> >
> > > Yes, this fixes build, but the AARCH64 core (I did not check ARM)
> >
> > > depends on self-relocation as it is loaded in-place at a location
> >
> > > unknown at compile-time. PIE helps ensure there are no relocations in
> >
> > > .text among other things. I know CLANGPDB does not support
> >
> > > ARM/AARCH64
> >
> > > yet, but if it is added, this may generate binaries with more dangerous
> >
> > > relocations, which means the chance of executing an instruction that
> >
> > > requires relocation without relocating first (relocation is done in C
> >
> > > code now!) is significantly higher. We do not need PIE for IA32 or X64
> >
> > > at all (or more specifically, we only need it for ARM-based
> >
> > > architectures as of now), so I prefer my patch which makes that
> >
> > > explicit. Though we can theoretically use your solution when limited to
> >
> > > non-ARM architectures if you really dislike my patch that much.
> >
> > >
> >
> > > I'd prefer to hear from the ARM core maintainers before making any
> move.
> >
> > >
> >
> > > Best regards,
> >
> > > Marvin
> >
> > >
> >
> > > On 14.10.21 05:12, Jiyang Yang wrote:
> >
> > > > the flag "-fpie" is passed for all builds with a GCC family toolchain,
> >
> > > > including CLANGPDB, but CLANGPDB does not support this flag, it will
> >
> > > > report "clang: error: unsupported option '-fpie' for target
> >
> > > > 'x86_64-unknown-windows-gnu'". So we add the CLANGPDB option "-
> fno-
> >
> > > pie"
> >
> > > > later to overwrite it.
> >
> > > >
> >
> > > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org
> > <mailto:ardb+tianocore@kernel.org>>
> >
> > > > Cc: Sami Mujawar <sami.mujawar@arm.com
> <mailto:sami.mujawar@arm.com>>
> >
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
> >
> > > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com
> > <mailto:supreeth.venkatesh@arm.com>>
> >
> > > > Cc: Vitaly Cheptsov <vit9696@protonmail.com
> > <mailto:vit9696@protonmail.com>>
> >
> > > > Cc: Marvin Häuser <mhaeuser@posteo.de
> <mailto:mhaeuser@posteo.de>>
> >
> > > > Cc: Steven Shi <steven.shi@intel.com <mailto:steven.shi@intel.com>>
> >
> > > > Signed-off-by: Jiyang Yang <jiyangx.yang@intel.com
> > <mailto:jiyangx.yang@intel.com>>
> >
> > > > ---
> >
> > > > StandaloneMmPkg/Core/StandaloneMmCore.inf | 2
> >
> > > ++
> >
> > > >
> >
> > >
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmC
> >
> > > oreEntryPoint.inf | 1 +
> >
> > > > 2 files changed, 3 insertions(+)
> >
> > > >
> >
> > > > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> >
> > > b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> >
> > > > index 56042b7b39f4..3213142523f4 100644
> >
> > > > --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> >
> > > > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> >
> > > > @@ -79,3 +79,5 @@
> >
> > > > [BuildOptions]
> >
> > > > GCC:*_*_*_CC_FLAGS = -fpie
> >
> > > > GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
> >
> > > > + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
> >
> > > > + CLANGPDB:*_*_*_DLINK_FLAGS =
> >
> > > > diff --git
> >
> > >
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> >
> > > CoreEntryPoint.inf
> >
> > >
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> >
> > > CoreEntryPoint.inf
> >
> > > > index 1762586cfa02..ef69e07d2c07 100644
> >
> > > > ---
> >
> > >
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> >
> > > CoreEntryPoint.inf
> >
> > > > +++
> >
> > >
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> >
> > > CoreEntryPoint.inf
> >
> > > > @@ -56,3 +56,4 @@
> >
> > > >
> >
> > > > [BuildOptions]
> >
> > > > GCC:*_*_*_CC_FLAGS = -fpie
> >
> > > > + CLANGPDB:*_*_*_CC_FLAGS = -fno-pie
> >
> > >
> >
> > >
> >
> > >
> >
> > >
> >
> > >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-14 15:51 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-14 3:12 [PATCH v1 0/1] StandaloneMmPkg: To support CLANGPDB build jiyangx.yang
2021-10-14 3:12 ` [PATCH v1 1/1] " Jiyang Yang
2021-10-14 8:05 ` Marvin Häuser
2021-10-14 9:02 ` [edk2-devel] " Steven Shi
2021-10-14 9:08 ` Marvin Häuser
2021-10-14 9:09 ` Ard Biesheuvel
2021-10-14 9:13 ` Marvin Häuser
2021-10-14 13:14 ` Steven Shi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox