Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: To support CLANGPDB build

["Marvin Häuser" <mhaeuser@posteo.de>]

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