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

[Renato Golin via Phabricator via llvm-commits]

rengolin added reviewers: peter.smith, jmolloy.
rengolin added a comment.

Hi Prakhar,

Thanks for working on this, it's a nice functionality. I have a bunch of inline comments, but some generic ones, too.

> Hence, execute-only support is only available for targets that support MOVW/MOVT (ARMv7 and above).

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.

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

I'm also adding more people to review this with experience in ELF and constant pools.

cheers,
--renato



================
Comment at: include/llvm/CodeGen/TargetLoweringObjectFileImpl.h:36
   bool UseInitArray;
-  mutable unsigned NextUniqueID = 0;
+  mutable unsigned NextUniqueID = 1;
 
----------------
Initialising with 0 is a bit obvious, but 1 is not. Can you add a comment, please?


================
Comment at: include/llvm/MC/SectionKind.h:32
+           /// ExecuteOnly, Text section that is not readable.
+           ExecuteOnly,
+
----------------
No TABs, please. Use clang-format on your patch to make sure it adheres to the coding standard.


================
Comment at: include/llvm/MC/SectionKind.h:119
+
+  bool isText() const { return K == Text || K == ExecuteOnly; }
+
----------------
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.


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


================
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),
----------------
I'm worried about this hijack. Does that mean it won't be used by Thumb2 (v6T2+) any more?


================
Comment at: lib/Target/ARM/ARMSubtarget.cpp:173
+  // Execute only support requires movt support
+  if (genExecuteOnly()) {
+    NoMovt = false;
----------------
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.


================
Comment at: lib/Target/ARM/ARMSubtarget.cpp:175
+    NoMovt = false;
+    assert(genExecuteOnly() && hasV8MBaselineOps());
+  }
----------------
This assert is wrong. It should be something like:

    assert(hasV8MBaselineOps() && "Execute-only supported by ARMv8M only");

And it should only be an assert if there are no combination of user flags that can get you here.

If there are, this should be an error.


================
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);
+  }
----------------
Where is the TextSection initialised if it's not pure? Shouldn't this move to the same place?


================
Comment at: test/CodeGen/ARM/constantfp.ll:89
+
+define arm_aapcs_vfpcc float @lower_const_f32_xo() {
+; CHECK-NO-XO-LABEL: lower_const_f32_xo
----------------
There are plenty of other tests in this file that need to make sure no loads from labels occurred. Please, add the relevant checks.


================
Comment at: test/CodeGen/ARM/execute-only-big-stack-frame.ll:10
+; CHECK-SUBW-ADDW-LABEL: test_big_stack_frame:
+; CHECK-SUBW-ADDW-NOT:   ldr {{r[0-9]+}}, {{.?LCPI[0-9]+_[0-9]+}}
+; CHECK-SUBW-ADDW:       sub.w sp, sp, #65536
----------------
Whenever adding a NOT pattern, you have to be as generic as possible. It's possible that a label load with a different pattern shows up and you won't pick it up.

Just use:

    CHECK-SUBW-ADDW-NOT: ldr


https://reviews.llvm.org/D27449