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

[Jim Grosbach via Phabricator via llvm-commits]

grosbach added a comment.

In https://reviews.llvm.org/D27449#617063, @rengolin wrote:

> Right, I'm happy with the changes from the code side, but there were some concerns on IRC if we should support this at all.


Seems a reasonable thing to support in general. Were the concerns about supporting execute-only conceptually or about the design of this approach?



================
Comment at: include/llvm/MC/SectionKind.h:119
+
+  bool isText() const { return K == Text || K == ExecuteOnly; }
+
----------------
prakhar wrote:
> 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.
Yeesh. This file is a bit of a mess. I'd much rather see ExecuteOnly be an attribute of Text rather than a completely distinct Kind. That's not the way the rest of the stuff is represented though, leading to the comment indentation mess Renato commented on. 

Bottom line, this is fine as-is here, I'm just grumbling about the overall file. :)


================
Comment at: lib/Target/ARM/ARM.td:267
+// Execute-only is also referred to as pure code by GCC
+def FeatureExecuteOnly : SubtargetFeature<"execute-only", "GenExecuteOnly", "true",
+                                        "Don't use data access to code sections">;
----------------
Why use a Feature flag for this? This isn't an attribute of the CPU, but rather a code generation option, right?

Would a function attribute be sufficient? At first look, it seems all the decisions to be made are about how to handle any given function's constant data. That would have the added benefit of making LTO "just work" since function attributes propagate naturally.


================
Comment at: lib/Target/ARM/ARMSubtarget.cpp:173
+  // Execute only support requires movt support
+  if (genExecuteOnly()) {
+    NoMovt = false;
----------------
prakhar wrote:
> 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.
The error should be detected earlier and a proper diagnostic issued if at all possible. Using report_fatal_error() for user-facing diagnostics is an option of last resort and an indicator of a layering failure elsewhere.


https://reviews.llvm.org/D27449