[PATCH] D27449: [ARM] Implement execute-only support in CodeGen

[Prakhar Bahuguna via Phabricator via llvm-commits]

prakhar marked 7 inline comments as done.
prakhar added a comment.

> Actually, MOVT was introduced in ARMv6T2, but execute-only seems to be an M thing, mostly ARMv7M and ARMv8M. Maybe advertise it that way instead of MOVT-dependent.

Execute-only code can be technically generated for any target that supports the MOVT instruction (so Thumb2 and v8-M Baseline targets, which includes v6T2), but is intended for M-class targets that have memory controllers supporting execute-only regions. I'll update the commit message with this.

> I wonder how compatible this is with the GCC implementation. Is this how GCC lays out the ELF object? Can we link GCC objects with LLVM ones and expect perfect cooperation (ex. globals & symbols)?

At the object level, there should be no issues with GCC compatibility and the objects should link without issue. For assembly code, LLVM does not yet support GAS's syntax of allowing numeric flags to be used, so I've created patch D27451 <https://reviews.llvm.org/D27451> for this.

> The ARM Compiler 6 documentation for execute-only http://infocenter.arm.com/help/topic/com.arm.doc.dui0774g/vvi1452508645836.html says Link Time Optimization does not honor the armclang -mexecute-only option. If you use the armclang -flto or -Omax options, then the compiler cannot generate execute-only code. Does this still apply, and if it does what should the action be, error message, user beware?

armclang does not appear to generate any warning or error if -flto is specified alongside -mexecute-only.

> Personally I think it would be better to use the GCC pure-code naming conventions for the test cases and comments. The execute-only name has been favoured by the commercial compilers such as ARM Compiler 5 and ARM Compiler 6. However I think that the majority of the people looking through the code base will be familiar with the GCC pure-code name. Standardising on pure-code would make it easier to equate the support in GCC and llvm and enhance expectations of compatibility. I think that this only means changing comments to refer to pure-code rather than execute-only and changing the file names of the tests to use pure-code rather than execute-only. Or perhaps a comment that equates execute-only with pure-code. I think it is valuable to have both -mexecute-only and -mpure-code as a migration path for both commercial and GCC users.

Execute-only has already been used in AC5 and AC6 as well as in IAR's compiler, and is a more intuitive name whose purpose is immediately understood. The -mpure-code flag is maintained as an alias for -mexecute-only in clang for compatibility in GCC, and the section header flag is referred to as SHF_ARM_PURECODE as is the case with GCC. The fact that the feature is named execute-only within LLVM is purely an implementation detail, and shouldn't be an issue barring any significant opposition. I can add a source code comment in to explain this if necessary.



================
Comment at: include/llvm/MC/SectionKind.h:32
+           /// ExecuteOnly, Text section that is not readable.
+           ExecuteOnly,
+
----------------
rengolin wrote:
> No TABs, please. Use clang-format on your patch to make sure it adheres to the coding standard.
This isn't a tab character. Unless you meant that ExecuteOnly should be placed at the same indent level as Text?


================
Comment at: include/llvm/MC/SectionKind.h:119
+
+  bool isText() const { return K == Text || K == ExecuteOnly; }
+
----------------
rengolin wrote:
> To me it seems `ExecuteOnly` is orthogonal to `isText`. The problem is that we're using `ReadOnly` on the same solution space.
> 
> I think we should keep the accessors as simple as possible and use `isText() || isExecuteOnly()` in code.
As implemented here, an execute-only section is essentially a specialised text section for all purposes, with the added restriction that reads should not be made from it. I'm not sure the two are orthogonal.


================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:318
   }
+  // Use 0 as the unique ID for execute-only text
+  if (Kind.isExecuteOnly())
----------------
rengolin wrote:
> Does this mean there is only one pure section on each module?
Yes, that's the intention.


================
Comment at: lib/Target/ARM/ARMInstrThumb2.td:3386
+// available in both v8-M.Baseline and Thumb2 targets
+def t2BR_JT : t2basePseudoInst<(outs),
           (ins GPR:$target, GPR:$index, i32imm:$jt),
----------------
rengolin wrote:
> I'm worried about this hijack. Does that mean it won't be used by Thumb2 (v6T2+) any more?
Thumb2 is a superset of V8MBaselineOps, so this won't be an issue.


================
Comment at: lib/Target/ARM/ARMSubtarget.cpp:173
+  // Execute only support requires movt support
+  if (genExecuteOnly()) {
+    NoMovt = false;
----------------
rengolin wrote:
> I'm worried about the silent behaviour here.
> 
> If movt is disabled by some other part or user flag, than this should be an error, not a silent change.
Yes, this should be an error. I've changed this to report an error if NoMovt is enabled or the target doesn't support MOVT. I'll also add a diagnostic to clang for this.


================
Comment at: lib/Target/ARM/ARMTargetObjectFile.cpp:49
+    // Use 0 as the unique ID for execute-only text
+    TextSection = Ctx.getELFSection(".text", Type, Flags, 0, "", 0U);
+  }
----------------
rengolin wrote:
> Where is the TextSection initialised if it's not pure? Shouldn't this move to the same place?
We cannot change the flags of an existing section, so we create a new text section with the flags set in addition to the default one.


https://reviews.llvm.org/D27449