[llvm] r306927 - Rewrite ARM execute only support to avoid the use of a command line flag and unqualified ARMSubtarget lookup.
Luke Cheeseman via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 27 05:29:48 PDT 2017
Hi Eric,
Sorry for the delay in responding. I have looked into this more and have a few more topics for discussion. Initially, Prakhar had created a new default .text section with the PURECODE flag set that was used instead of the default .text section. This was in ARMElfTargetObjectFile::Initialize:
+ // 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);
+ }
This snippet was removed and so leads to the generation of two .text sections as the exectue-only functions can't live in the normal .text section (when compiling with -mexecute-only).
I see what you mean about there being no clear decision against implementing the execute-only feature in this way, just alternatives that were suggested and selected. In particular using a backend option.
I have looked into your rewrite and can see that it should allow more flexibilty for LTO as the execute-only attribute is at a function level. I am perhaps not the correct person to speak about execute-only and LTO, I found previous discussions which mention marking each function with a function attribute that details whether it is execute-only. This seems like an interesting way of gaining the flexibilty.
For now I would like to discuss the empty .text section. A linker can remove the empty .text section (given the correct conditions) that mean it will not cause issues. However, I was thinking that it would perhaps be better not to generate it at all. I've noticed a similar effect when compiling with -ffunction-sections, each function gets emitted into its own section and there is a left over unused .text section. Perhaps it would be useful to have some option to control whether empty sections are emitted to the object file ot not. Another solution is to be able to set the flags on the default .text sections, this would only help with the -mexecute-only case; this way the default .text section would be used and we would avoid generating a second .text section with the correct flags.
Do you have any thoughts or alternatives?
Thanks,
Luke
________________________________
From: Eric Christopher <echristo at gmail.com>
Sent: 14 July 2017 18:00:32
To: Luke Cheeseman; llvm-commits at lists.llvm.org; Christof Douma
Cc: nd
Subject: Re: [llvm] r306927 - Rewrite ARM execute only support to avoid the use of a command line flag and unqualified ARMSubtarget lookup.
Hi Luke,
On Fri, Jul 14, 2017 at 7:57 AM Luke Cheeseman <Luke.Cheeseman2 at arm.com<mailto: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<mailto:llvm-commits-bounces at lists.llvm.org>> on behalf of Eric Christopher via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>>
Sent: 01 July 2017 03:55:22
To: llvm-commits at lists.llvm.org<mailto: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<mailto: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/20170927/67058b1b/attachment-0001.html>
More information about the llvm-commits
mailing list