| Message ID | 20181212103308.8099-3-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | b048a2204db99c61784b5027c8ade18c4fe6be6d |
| Headers | show |
| Series | fixes for CLANG35 on ARM | expand |
On 12/12/18 11:33, Ard Biesheuvel wrote: > PE/COFF only has a very limited id space for runtime relocations, and > so it defines only a single relocation for movw/movt instruction pairs, > which can be combined to load a 32-bit symbol reference into a register. > For this to work as expected, these instructions must always appear in > the same order and adjacently, and this is something few compilers take > into account, unless they target PE/COFF explicitly (and this is not the > case for our ELF based toolchains) > > For Clang 3.6 and later, we can pass the -mno-movt option to suppress > movw/movt pairs entirely, which works around the issue. Unfortunately, > for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0) > and mutually incompatible between 3.5 and 3.6. > > Since it is desirable for the CLANG35 toolchain to be usable on newer > versions of Clang as well (given that it is the only non-LTO alternative > to CLANG38), let's work around this issue in a way that permits versions > 3.5 and newer of Clang to be used with the CLANG35 profile. > > So pass the -mkernel flag instead (and drop the -mno-unaligned-access > in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which > Clang complains about). This also inhibits movw/movt generation, along > with some other changes (e.g., long calls) which do affect code generation > but not in a undesirable manner. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > BaseTools/Conf/tools_def.template | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template > index ac2b95e0f5ba..2ba833e1fb06 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64 > *_CLANG35_ARM_ASM_FLAGS = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments > *_CLANG35_ARM_DLINK_FLAGS = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS) > *_CLANG35_ARM_DLINK2_FLAGS = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 > -*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a > +*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a -mkernel -Qunused-arguments > *_CLANG35_ARM_PP_FLAGS = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > *_CLANG35_ARM_RC_FLAGS = DEF(GCC_ARM_RC_FLAGS) > *_CLANG35_ARM_VFRPP_FLAGS = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > The commit message speaks about adding -mkernel, and removing -mno-unaligned-access (elsewhere). In the patch, I only see -mkernel (plus an un-announced -Qunused-arguments option). Is the commit message slightly out-of-date relative to the code? (Or perhaps the other way around?) Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, 12 Dec 2018 at 12:50, Laszlo Ersek <lersek@redhat.com> wrote: > > On 12/12/18 11:33, Ard Biesheuvel wrote: > > PE/COFF only has a very limited id space for runtime relocations, and > > so it defines only a single relocation for movw/movt instruction pairs, > > which can be combined to load a 32-bit symbol reference into a register. > > For this to work as expected, these instructions must always appear in > > the same order and adjacently, and this is something few compilers take > > into account, unless they target PE/COFF explicitly (and this is not the > > case for our ELF based toolchains) > > > > For Clang 3.6 and later, we can pass the -mno-movt option to suppress > > movw/movt pairs entirely, which works around the issue. Unfortunately, > > for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0) > > and mutually incompatible between 3.5 and 3.6. > > > > Since it is desirable for the CLANG35 toolchain to be usable on newer > > versions of Clang as well (given that it is the only non-LTO alternative > > to CLANG38), let's work around this issue in a way that permits versions > > 3.5 and newer of Clang to be used with the CLANG35 profile. > > > > So pass the -mkernel flag instead (and drop the -mno-unaligned-access > > in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which > > Clang complains about). This also inhibits movw/movt generation, along > > with some other changes (e.g., long calls) which do affect code generation > > but not in a undesirable manner. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > BaseTools/Conf/tools_def.template | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template > > index ac2b95e0f5ba..2ba833e1fb06 100755 > > --- a/BaseTools/Conf/tools_def.template > > +++ b/BaseTools/Conf/tools_def.template > > @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64 > > *_CLANG35_ARM_ASM_FLAGS = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments > > *_CLANG35_ARM_DLINK_FLAGS = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS) > > *_CLANG35_ARM_DLINK2_FLAGS = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 > > -*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a > > +*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a -mkernel -Qunused-arguments > > *_CLANG35_ARM_PP_FLAGS = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > > *_CLANG35_ARM_RC_FLAGS = DEF(GCC_ARM_RC_FLAGS) > > *_CLANG35_ARM_VFRPP_FLAGS = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > > > > The commit message speaks about adding -mkernel, and removing > -mno-unaligned-access (elsewhere). In the patch, I only see -mkernel > (plus an un-announced -Qunused-arguments option). > > Is the commit message slightly out-of-date relative to the code? (Or > perhaps the other way around?) > Ah yes. The -mno-unaligned-access became redundant, but instead of removing it, i ended up adding the -Qunused-arguments to stop Clang from complaining about it. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12/12/18 12:51, Ard Biesheuvel wrote: > On Wed, 12 Dec 2018 at 12:50, Laszlo Ersek <lersek@redhat.com> wrote: >> >> On 12/12/18 11:33, Ard Biesheuvel wrote: >>> PE/COFF only has a very limited id space for runtime relocations, and >>> so it defines only a single relocation for movw/movt instruction pairs, >>> which can be combined to load a 32-bit symbol reference into a register. >>> For this to work as expected, these instructions must always appear in >>> the same order and adjacently, and this is something few compilers take >>> into account, unless they target PE/COFF explicitly (and this is not the >>> case for our ELF based toolchains) >>> >>> For Clang 3.6 and later, we can pass the -mno-movt option to suppress >>> movw/movt pairs entirely, which works around the issue. Unfortunately, >>> for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0) >>> and mutually incompatible between 3.5 and 3.6. >>> >>> Since it is desirable for the CLANG35 toolchain to be usable on newer >>> versions of Clang as well (given that it is the only non-LTO alternative >>> to CLANG38), let's work around this issue in a way that permits versions >>> 3.5 and newer of Clang to be used with the CLANG35 profile. >>> >>> So pass the -mkernel flag instead (and drop the -mno-unaligned-access >>> in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which >>> Clang complains about). This also inhibits movw/movt generation, along >>> with some other changes (e.g., long calls) which do affect code generation >>> but not in a undesirable manner. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> BaseTools/Conf/tools_def.template | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template >>> index ac2b95e0f5ba..2ba833e1fb06 100755 >>> --- a/BaseTools/Conf/tools_def.template >>> +++ b/BaseTools/Conf/tools_def.template >>> @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64 >>> *_CLANG35_ARM_ASM_FLAGS = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments >>> *_CLANG35_ARM_DLINK_FLAGS = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS) >>> *_CLANG35_ARM_DLINK2_FLAGS = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 >>> -*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a >>> +*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a -mkernel -Qunused-arguments >>> *_CLANG35_ARM_PP_FLAGS = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) >>> *_CLANG35_ARM_RC_FLAGS = DEF(GCC_ARM_RC_FLAGS) >>> *_CLANG35_ARM_VFRPP_FLAGS = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) >>> >> >> The commit message speaks about adding -mkernel, and removing >> -mno-unaligned-access (elsewhere). In the patch, I only see -mkernel >> (plus an un-announced -Qunused-arguments option). >> >> Is the commit message slightly out-of-date relative to the code? (Or >> perhaps the other way around?) >> > > Ah yes. The -mno-unaligned-access became redundant, but instead of > removing it, i ended up adding the -Qunused-arguments to stop Clang > from complaining about it. > OK, thanks! With the commit message updated accordingly: Acked-by: Laszlo Ersek <lersek@redhat.com> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, 12 Dec 2018 at 11:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > PE/COFF only has a very limited id space for runtime relocations, and > so it defines only a single relocation for movw/movt instruction pairs, > which can be combined to load a 32-bit symbol reference into a register. > For this to work as expected, these instructions must always appear in > the same order and adjacently, and this is something few compilers take > into account, unless they target PE/COFF explicitly (and this is not the > case for our ELF based toolchains) > > For Clang 3.6 and later, we can pass the -mno-movt option to suppress > movw/movt pairs entirely, which works around the issue. Unfortunately, > for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0) > and mutually incompatible between 3.5 and 3.6. > > Since it is desirable for the CLANG35 toolchain to be usable on newer > versions of Clang as well (given that it is the only non-LTO alternative > to CLANG38), let's work around this issue in a way that permits versions > 3.5 and newer of Clang to be used with the CLANG35 profile. > > So pass the -mkernel flag instead (and drop the -mno-unaligned-access > in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which > Clang complains about). This also inhibits movw/movt generation, along > with some other changes (e.g., long calls) which do affect code generation > but not in a undesirable manner. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > BaseTools/Conf/tools_def.template | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template > index ac2b95e0f5ba..2ba833e1fb06 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64 > *_CLANG35_ARM_ASM_FLAGS = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments > *_CLANG35_ARM_DLINK_FLAGS = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS) > *_CLANG35_ARM_DLINK2_FLAGS = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 > -*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a > +*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a -mkernel -Qunused-arguments Alternatively, we could switch to armv6 code generation here (except for the .S pieces which us Thumb2 instructions), which also gets rid of the movw/movt pairs. > *_CLANG35_ARM_PP_FLAGS = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > *_CLANG35_ARM_RC_FLAGS = DEF(GCC_ARM_RC_FLAGS) > *_CLANG35_ARM_VFRPP_FLAGS = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > -- > 2.19.2 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Dec 12, 2018 at 01:30:10PM +0100, Ard Biesheuvel wrote: > On Wed, 12 Dec 2018 at 11:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > > PE/COFF only has a very limited id space for runtime relocations, and > > so it defines only a single relocation for movw/movt instruction pairs, > > which can be combined to load a 32-bit symbol reference into a register. > > For this to work as expected, these instructions must always appear in > > the same order and adjacently, and this is something few compilers take > > into account, unless they target PE/COFF explicitly (and this is not the > > case for our ELF based toolchains) > > > > For Clang 3.6 and later, we can pass the -mno-movt option to suppress > > movw/movt pairs entirely, which works around the issue. Unfortunately, > > for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0) > > and mutually incompatible between 3.5 and 3.6. > > > > Since it is desirable for the CLANG35 toolchain to be usable on newer > > versions of Clang as well (given that it is the only non-LTO alternative > > to CLANG38), let's work around this issue in a way that permits versions > > 3.5 and newer of Clang to be used with the CLANG35 profile. > > > > So pass the -mkernel flag instead (and drop the -mno-unaligned-access > > in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which > > Clang complains about). This also inhibits movw/movt generation, along > > with some other changes (e.g., long calls) which do affect code generation > > but not in a undesirable manner. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > BaseTools/Conf/tools_def.template | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template > > index ac2b95e0f5ba..2ba833e1fb06 100755 > > --- a/BaseTools/Conf/tools_def.template > > +++ b/BaseTools/Conf/tools_def.template > > @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64 > > *_CLANG35_ARM_ASM_FLAGS = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments > > *_CLANG35_ARM_DLINK_FLAGS = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS) > > *_CLANG35_ARM_DLINK2_FLAGS = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 > > -*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a > > +*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a -mkernel -Qunused-arguments > > Alternatively, we could switch to armv6 code generation here (except > for the .S pieces which us Thumb2 instructions), which also gets rid > of the movw/movt pairs. Please don't. For the version Laszlo is happy with: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> / Leif > > *_CLANG35_ARM_PP_FLAGS = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > > *_CLANG35_ARM_RC_FLAGS = DEF(GCC_ARM_RC_FLAGS) > > *_CLANG35_ARM_VFRPP_FLAGS = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) > > -- > > 2.19.2 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template index ac2b95e0f5ba..2ba833e1fb06 100755 --- a/BaseTools/Conf/tools_def.template +++ b/BaseTools/Conf/tools_def.template @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64 *_CLANG35_ARM_ASM_FLAGS = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments *_CLANG35_ARM_DLINK_FLAGS = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS) *_CLANG35_ARM_DLINK2_FLAGS = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 -*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a +*_CLANG35_ARM_PLATFORM_FLAGS = -march=armv7-a -mkernel -Qunused-arguments *_CLANG35_ARM_PP_FLAGS = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) *_CLANG35_ARM_RC_FLAGS = DEF(GCC_ARM_RC_FLAGS) *_CLANG35_ARM_VFRPP_FLAGS = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
PE/COFF only has a very limited id space for runtime relocations, and so it defines only a single relocation for movw/movt instruction pairs, which can be combined to load a 32-bit symbol reference into a register. For this to work as expected, these instructions must always appear in the same order and adjacently, and this is something few compilers take into account, unless they target PE/COFF explicitly (and this is not the case for our ELF based toolchains) For Clang 3.6 and later, we can pass the -mno-movt option to suppress movw/movt pairs entirely, which works around the issue. Unfortunately, for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0) and mutually incompatible between 3.5 and 3.6. Since it is desirable for the CLANG35 toolchain to be usable on newer versions of Clang as well (given that it is the only non-LTO alternative to CLANG38), let's work around this issue in a way that permits versions 3.5 and newer of Clang to be used with the CLANG35 profile. So pass the -mkernel flag instead (and drop the -mno-unaligned-access in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which Clang complains about). This also inhibits movw/movt generation, along with some other changes (e.g., long calls) which do affect code generation but not in a undesirable manner. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- BaseTools/Conf/tools_def.template | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.19.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel