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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

In https://reviews.llvm.org/D27449#617363, @grosbach wrote:

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


About the concept of unreadable code areas in general, and what benefits this really brings to security and performance of deeply embedded devices.

I honestly think this is a feature that ARM hardware needs support and I'm not a hardware expert to decide what features people need on such an individualised market. With the change itself being reasonably non-intrusive, I can't see why we'd block such a feature on those grounds only.

cheers,
--renato



================
Comment at: include/llvm/MC/SectionKind.h:119
+
+  bool isText() const { return K == Text || K == ExecuteOnly; }
+
----------------
grosbach wrote:
> 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. :)
Yeah, so, after I saw *all* the other options, I realised this split was the least of our worries. :)

My knowledge of this code is limited, so I'm not sure I could suggest a way to make this separation consistent everywhere else, and I'd certainly dislike proposing something ARM-specific here.


================
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">;
----------------
grosbach wrote:
> 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.
Oh, I missed that one! Indeed, this is a codegen attribute, which is dependent on the platform, but not enough to be a sub-target feature.

I think the confusion is that Clang emits "-target-feature" "+execute-only", but those are different things.


================
Comment at: lib/Target/ARM/ARMSubtarget.cpp:173
+  // Execute only support requires movt support
+  if (genExecuteOnly()) {
+    NoMovt = false;
----------------
grosbach wrote:
> 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.
There is an error in Clang (D27450) that detects this. Is there a better way to do this in LLVM itself?


https://reviews.llvm.org/D27449