[llvm] r306927 - Rewrite ARM execute only support to avoid the use of a command line flag and unqualified ARMSubtarget lookup.
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 14 10:00:32 PDT 2017
Hi Luke,
On Fri, Jul 14, 2017 at 7:57 AM Luke Cheeseman <Luke.Cheeseman2 at arm.com>
wrote:
>
> Hi Eric,
>
>
>
> You recently commited:
>
> [llvm] r306927 - Rewrite ARM execute only support to avoid the use of a
> command line flag and unqualified ARMSubtarget lookup.
>
>
>
> This is breaking some of our testing as it changes the attributes of the
> default .text section, namely the section is no longer execute only. This
> causes two .text sections to be created (one that is execute only and one
> that is not); some tools may have issues with this. For example, a linker
> may merge the two .text sections into one segment and remove the PURECODE
> flag in conformance with section 4.3.3 of
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf
> .
>
>
>
Fascinating. I'm not seeing in here where it allows removing of the flag in
there, but it's early and I might be missing it. Also, sounds like you're
missing a compile time option on a few files in whatever build broke :)
> Furthermore, in your patch you made similar changes that were initially
> proposed by Prakhar (namely implementing execute only as a feature flag)
> but were discussed and decided against. See
> https://reviews.llvm.org/D27449?id=80748#inline-237468 .
>
>
>
Yeah, that wasn't the right thing to do.
The original reviewer comment was correct and, unfortunately, this patch
will need to be implemented in another way if this is the desired behavior.
I did check the ABI but couldn't find anything that disallowed having
sections that are both execute only and not.
It's also not obvious on "decided against". Who decided? It definitely
wasn't the original reviewer.
I think the mechanics you want are to define a module flag and error out on
linking where the module flags don't match. I'm not a huge fan of them, but
it is an existing mechanism here. Alternately you can go with the original
reviewer and use a code gen flag. This is easier to undo if we find some
problem later, but does ignore state in the module that you might want to
capture.
Thoughts?
-eric
> Take the following example (part of the regression test suite):
>
>
>
> $ cat test/CodeGen/ARM/execute-only-section.ll
>
> ...
>
> ; CHECK: .section .text,"axy",%progbits,unique,0
>
> ; CHECK-NOT: .section
>
> ; CHECK-NOT: .text
>
> ; CHECK: .globl test_SectionForGlobal
>
> ; CHECK: .type test_SectionForGlobal,%function
>
> define void @test_SectionForGlobal() {
>
> entry:
>
> ret void
>
> }
>
> ...
>
> $ llc test/CodeGen/ARM/execute-only-section.ll -mtriple=thumbv7m
> -mattr=+execute-only -o -
>
> .text
>
> .syntax unified
>
> .eabi_attribute
>
> 67, "2.09" @ Tag_conformance
>
> ....
>
> "test/CodeGen/ARM/execute-only-section.ll"
>
> .section
>
> .text,"axy",%progbits,unique,0
>
> .globl
>
> test_SectionForGlobal @ -- Begin function test_SectionForGlobal
>
> .p2align
>
> 2
>
> .type
>
> test_SectionForGlobal,%function
>
> .code
>
> 16 @ @test_SectionForGlobal
>
> .thumb_func
>
> test_SectionForGlobal:
>
> .fnstart
>
> @ BB#0: @ %entry
>
> ...
>
>
>
> There are now two .text sections, one which is execute only and one which
> is not. This test still passes however as file check finds the execute only
> .text section.
>
>
>
> This is due to this part of the commit:
>
> --- llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.cpp Fri Jun 30 19:55:22
> 2017
> @@ -32,7 +32,7 @@ void ARMElfTargetObjectFile::Initialize(
> const TargetMachine &TM) {
> const ARMBaseTargetMachine &ARM_TM = static_cast<const
> ARMBaseTargetMachine &>(TM);
> bool isAAPCS_ABI = ARM_TM.TargetABI ==
> ARMBaseTargetMachine::ARMABI::ARM_ABI_AAPCS;
> - genExecuteOnly = ARM_TM.getSubtargetImpl()->genExecuteOnly();
> + // genExecuteOnly = ARM_TM.getSubtargetImpl()->genExecuteOnly();
>
> TargetLoweringObjectFileELF::Initialize(Ctx, TM);
> InitializeELF(isAAPCS_ABI);
> @@ -43,16 +43,6 @@ void ARMElfTargetObjectFile::Initialize(
>
> AttributesSection =
> getContext().getELFSection(".ARM.attributes",
> ELF::SHT_ARM_ATTRIBUTES, 0);
> -
> - // Make code section unreadable when in execute-only mode
> - if (genExecuteOnly) {
> - unsigned Type = ELF::SHT_PROGBITS;
> - unsigned Flags = ELF::SHF_EXECINSTR | ELF::SHF_ALLOC |
> ELF::SHF_ARM_PURECODE;
> - // Since we cannot modify flags for an existing section, we create a
> new
> - // section with the right flags, and use 0 as the unique ID for
> - // execute-only text
> - TextSection = Ctx.getELFSection(".text", Type, Flags, 0, "", 0U);
> - }
> }
>
>
>
> Kind Regards,
>
> Luke
>
> ------------------------------
> *From:* llvm-commits <llvm-commits-bounces at lists.llvm.org> on behalf of
> Eric Christopher via llvm-commits <llvm-commits at lists.llvm.org>
> *Sent:* 01 July 2017 03:55:22
> *To:* llvm-commits at lists.llvm.org
> *Subject:* [llvm] r306927 - Rewrite ARM execute only support to avoid the
> use of a command line flag and unqualified ARMSubtarget lookup.
>
> Author: echristo
> Date: Fri Jun 30 19:55:22 2017
> New Revision: 306927
>
> URL: http://llvm.org/viewvc/llvm-project?rev=306927&view=rev
> Log:
> Rewrite ARM execute only support to avoid the use of a command line flag
> and unqualified ARMSubtarget lookup.
>
> Paired with a clang commit to use the new behavior.
>
> Modified:
> llvm/trunk/lib/Target/ARM/ARM.td
> llvm/trunk/lib/Target/ARM/ARMSubtarget.cpp
> llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.cpp
> llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.h
> llvm/trunk/test/CodeGen/ARM/constantfp.ll
> llvm/trunk/test/CodeGen/ARM/execute-only-big-stack-frame.ll
> llvm/trunk/test/CodeGen/ARM/execute-only-section.ll
> llvm/trunk/test/CodeGen/ARM/execute-only.ll
>
> Modified: llvm/trunk/lib/Target/ARM/ARM.td
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARM.td?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARM.td (original)
> +++ llvm/trunk/lib/Target/ARM/ARM.td Fri Jun 30 19:55:22 2017
> @@ -269,6 +269,10 @@ def FeatureLongCalls : SubtargetFeature<
> "Generate calls via indirect call
> "
> "instructions">;
>
> +def FeatureExecuteOnly
> + : SubtargetFeature<"execute-only", "GenExecuteOnly", "true",
> + "Enable the generation of execute only code.">;
> +
> def FeatureReserveR9 : SubtargetFeature<"reserve-r9", "ReserveR9", "true",
> "Reserve R9, making it
> unavailable as "
> "GPR">;
>
> Modified: llvm/trunk/lib/Target/ARM/ARMSubtarget.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMSubtarget.cpp?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMSubtarget.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMSubtarget.cpp Fri Jun 30 19:55:22 2017
> @@ -92,11 +92,6 @@ ARMSubtarget &ARMSubtarget::initializeSu
> return *this;
> }
>
> -/// EnableExecuteOnly - Enables the generation of execute-only code on
> supported
> -/// targets
> -static cl::opt<bool>
> -EnableExecuteOnly("arm-execute-only");
> -
> ARMFrameLowering *ARMSubtarget::initializeFrameLowering(StringRef CPU,
> StringRef FS) {
> ARMSubtarget &STI = initializeSubtargetDependencies(CPU, FS);
> @@ -139,9 +134,8 @@ ARMSubtarget::ARMSubtarget(const Triple
> const std::string &FS,
> const ARMBaseTargetMachine &TM, bool IsLittle)
> : ARMGenSubtargetInfo(TT, CPU, FS), UseMulOps(UseFusedMulOps),
> - GenExecuteOnly(EnableExecuteOnly), CPUString(CPU),
> IsLittle(IsLittle),
> - TargetTriple(TT), Options(TM.Options), TM(TM),
> - FrameLowering(initializeFrameLowering(CPU, FS)),
> + CPUString(CPU), IsLittle(IsLittle), TargetTriple(TT),
> Options(TM.Options),
> + TM(TM), FrameLowering(initializeFrameLowering(CPU, FS)),
> // At this point initializeSubtargetDependencies has been called so
> // we can query directly.
> InstrInfo(isThumb1Only()
>
> Modified: llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.cpp?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.cpp Fri Jun 30 19:55:22
> 2017
> @@ -32,7 +32,7 @@ void ARMElfTargetObjectFile::Initialize(
> const TargetMachine &TM) {
> const ARMBaseTargetMachine &ARM_TM = static_cast<const
> ARMBaseTargetMachine &>(TM);
> bool isAAPCS_ABI = ARM_TM.TargetABI ==
> ARMBaseTargetMachine::ARMABI::ARM_ABI_AAPCS;
> - genExecuteOnly = ARM_TM.getSubtargetImpl()->genExecuteOnly();
> + // genExecuteOnly = ARM_TM.getSubtargetImpl()->genExecuteOnly();
>
> TargetLoweringObjectFileELF::Initialize(Ctx, TM);
> InitializeELF(isAAPCS_ABI);
> @@ -43,16 +43,6 @@ void ARMElfTargetObjectFile::Initialize(
>
> AttributesSection =
> getContext().getELFSection(".ARM.attributes",
> ELF::SHT_ARM_ATTRIBUTES, 0);
> -
> - // Make code section unreadable when in execute-only mode
> - if (genExecuteOnly) {
> - unsigned Type = ELF::SHT_PROGBITS;
> - unsigned Flags = ELF::SHF_EXECINSTR | ELF::SHF_ALLOC |
> ELF::SHF_ARM_PURECODE;
> - // Since we cannot modify flags for an existing section, we create a
> new
> - // section with the right flags, and use 0 as the unique ID for
> - // execute-only text
> - TextSection = Ctx.getELFSection(".text", Type, Flags, 0, "", 0U);
> - }
> }
>
> const MCExpr *ARMElfTargetObjectFile::getTTypeGlobalReference(
> @@ -74,21 +64,27 @@ getDebugThreadLocalSymbol(const MCSymbol
> getContext());
> }
>
> -MCSection *
> -ARMElfTargetObjectFile::getExplicitSectionGlobal(const GlobalObject *GO,
> - SectionKind SK, const
> TargetMachine &TM) const {
> +static bool isExecuteOnlyFunction(const GlobalObject *GO, SectionKind SK,
> + const TargetMachine &TM) {
> + if (const Function *F = dyn_cast<Function>(GO))
> + if (TM.getSubtarget<ARMSubtarget>(*F).genExecuteOnly() && SK.isText())
> + return true;
> + return false;
> +}
> +
> +MCSection *ARMElfTargetObjectFile::getExplicitSectionGlobal(
> + const GlobalObject *GO, SectionKind SK, const TargetMachine &TM)
> const {
> // Set execute-only access for the explicit section
> - if (genExecuteOnly && SK.isText())
> + if (isExecuteOnlyFunction(GO, SK, TM))
> SK = SectionKind::getExecuteOnly();
>
> return TargetLoweringObjectFileELF::getExplicitSectionGlobal(GO, SK,
> TM);
> }
>
> -MCSection *
> -ARMElfTargetObjectFile::SelectSectionForGlobal(const GlobalObject *GO,
> - SectionKind SK, const
> TargetMachine &TM) const {
> +MCSection *ARMElfTargetObjectFile::SelectSectionForGlobal(
> + const GlobalObject *GO, SectionKind SK, const TargetMachine &TM)
> const {
> // Place the global in the execute-only text section
> - if (genExecuteOnly && SK.isText())
> + if (isExecuteOnlyFunction(GO, SK, TM))
> SK = SectionKind::getExecuteOnly();
>
> return TargetLoweringObjectFileELF::SelectSectionForGlobal(GO, SK, TM);
>
> Modified: llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.h?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.h (original)
> +++ llvm/trunk/lib/Target/ARM/ARMTargetObjectFile.h Fri Jun 30 19:55:22
> 2017
> @@ -16,8 +16,6 @@
> namespace llvm {
>
> class ARMElfTargetObjectFile : public TargetLoweringObjectFileELF {
> - mutable bool genExecuteOnly = false;
> -
> protected:
> const MCSection *AttributesSection = nullptr;
>
>
> Modified: llvm/trunk/test/CodeGen/ARM/constantfp.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/constantfp.ll?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/constantfp.ll (original)
> +++ llvm/trunk/test/CodeGen/ARM/constantfp.ll Fri Jun 30 19:55:22 2017
> @@ -5,25 +5,25 @@
> ; RUN: llc -mtriple=thumbv7m -mcpu=cortex-m4 %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-NO-XO %s
>
> -; RUN: llc -mtriple=thumbv7m -arm-execute-only -mcpu=cortex-m4 %s -o - \
> +; RUN: llc -mtriple=thumbv7m -mattr=+execute-only -mcpu=cortex-m4 %s -o -
> \
> ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT
> --check-prefix=CHECK-XO-DOUBLE %s
>
> -; RUN: llc -mtriple=thumbv7meb -arm-execute-only -mcpu=cortex-m4 %s -o - \
> +; RUN: llc -mtriple=thumbv7meb -mattr=+execute-only -mcpu=cortex-m4 %s -o
> - \
> ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT
> --check-prefix=CHECK-XO-DOUBLE-BE %s
>
> -; RUN: llc -mtriple=thumbv7m -arm-execute-only -mcpu=cortex-m4
> -relocation-model=ropi %s -o - \
> +; RUN: llc -mtriple=thumbv7m -mattr=+execute-only -mcpu=cortex-m4
> -relocation-model=ropi %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-XO-ROPI %s
>
> ; RUN: llc -mtriple=thumbv8m.main -mattr=fp-armv8 %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-NO-XO %s
>
> -; RUN: llc -mtriple=thumbv8m.main -arm-execute-only -mattr=fp-armv8 %s -o
> - \
> +; RUN: llc -mtriple=thumbv8m.main -mattr=+execute-only -mattr=fp-armv8 %s
> -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT
> --check-prefix=CHECK-XO-DOUBLE %s
>
> -; RUN: llc -mtriple=thumbv8m.maineb -arm-execute-only -mattr=fp-armv8 %s
> -o - \
> +; RUN: llc -mtriple=thumbv8m.maineb -mattr=+execute-only -mattr=fp-armv8
> %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-XO-FLOAT
> --check-prefix=CHECK-XO-DOUBLE-BE %s
>
> -; RUN: llc -mtriple=thumbv8m.main -arm-execute-only -mattr=fp-armv8
> -relocation-model=ropi %s -o - \
> +; RUN: llc -mtriple=thumbv8m.main -mattr=+execute-only -mattr=fp-armv8
> -relocation-model=ropi %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-XO-ROPI %s
>
> define arm_aapcs_vfpcc float @test_vmov_f32() {
>
> Modified: llvm/trunk/test/CodeGen/ARM/execute-only-big-stack-frame.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/execute-only-big-stack-frame.ll?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/execute-only-big-stack-frame.ll (original)
> +++ llvm/trunk/test/CodeGen/ARM/execute-only-big-stack-frame.ll Fri Jun 30
> 19:55:22 2017
> @@ -1,8 +1,8 @@
> -; RUN: llc < %s -mtriple=thumbv7m -arm-execute-only -O0 %s -o - \
> +; RUN: llc < %s -mtriple=thumbv7m -mattr=+execute-only -O0 %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-SUBW-ADDW %s
> -; RUN: llc < %s -mtriple=thumbv8m.base -arm-execute-only -O0 %s -o - \
> +; RUN: llc < %s -mtriple=thumbv8m.base -mattr=+execute-only -O0 %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-MOVW-MOVT-ADD %s
> -; RUN: llc < %s -mtriple=thumbv8m.main -arm-execute-only -O0 %s -o - \
> +; RUN: llc < %s -mtriple=thumbv8m.main -mattr=+execute-only -O0 %s -o - \
> ; RUN: | FileCheck --check-prefix=CHECK-SUBW-ADDW %s
>
> define i8 @test_big_stack_frame() {
>
> Modified: llvm/trunk/test/CodeGen/ARM/execute-only-section.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/execute-only-section.ll?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/execute-only-section.ll (original)
> +++ llvm/trunk/test/CodeGen/ARM/execute-only-section.ll Fri Jun 30
> 19:55:22 2017
> @@ -1,6 +1,6 @@
> -; RUN: llc < %s -mtriple=thumbv7m -arm-execute-only %s -o - | FileCheck %s
> -; RUN: llc < %s -mtriple=thumbv8m.base -arm-execute-only %s -o - |
> FileCheck %s
> -; RUN: llc < %s -mtriple=thumbv8m.main -arm-execute-only %s -o - |
> FileCheck %s
> +; RUN: llc < %s -mtriple=thumbv7m -mattr=+execute-only %s -o - |
> FileCheck %s
> +; RUN: llc < %s -mtriple=thumbv8m.base -mattr=+execute-only %s -o - |
> FileCheck %s
> +; RUN: llc < %s -mtriple=thumbv8m.main -mattr=+execute-only %s -o - |
> FileCheck %s
>
> ; CHECK: .section .text,"axy",%progbits,unique,0
> ; CHECK-NOT: .section
>
> Modified: llvm/trunk/test/CodeGen/ARM/execute-only.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/execute-only.ll?rev=306927&r1=306926&r2=306927&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/execute-only.ll (original)
> +++ llvm/trunk/test/CodeGen/ARM/execute-only.ll Fri Jun 30 19:55:22 2017
> @@ -1,6 +1,6 @@
> -; RUN: llc -mtriple=thumbv8m.base-eabi -arm-execute-only %s -o - |
> FileCheck --check-prefix=CHECK --check-prefix=CHECK-T2BASE %s
> -; RUN: llc -mtriple=thumbv7m-eabi -arm-execute-only %s -o - |
> FileCheck --check-prefix=CHECK --check-prefix=CHECK-T2 %s
> -; RUN: llc -mtriple=thumbv8m.main-eabi -arm-execute-only %s -o - |
> FileCheck --check-prefix=CHECK --check-prefix=CHECK-T2 %s
> +; RUN: llc -mtriple=thumbv8m.base-eabi -mattr=+execute-only %s -o - |
> FileCheck --check-prefix=CHECK --check-prefix=CHECK-T2BASE %s
> +; RUN: llc -mtriple=thumbv7m-eabi -mattr=+execute-only %s -o - |
> FileCheck --check-prefix=CHECK --check-prefix=CHECK-T2 %s
> +; RUN: llc -mtriple=thumbv8m.main-eabi -mattr=+execute-only %s -o - |
> FileCheck --check-prefix=CHECK --check-prefix=CHECK-T2 %s
>
> @var = global i32 0
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170714/353a503b/attachment.html>
More information about the llvm-commits
mailing list