* [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