[PATCH] D48792: [ARM] Set execute-only flags in .text.

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: lib/MC/ELFObjectWriter.cpp:1096
     }
+
+    // For ARM, we need to add SHF_ARM_PURECODE to an empty TextSection if any
----------------
ivanlozano wrote:
> peter.smith wrote:
> > It might be possible to simplify the code below and make it a bit less Arm specific. My thinking is that instead of testing for (Section.getFlags() & SHF_ARM_PURECODE) test for Section.getKind().isExecuteOnly() then you won't need to guard the whole thing by getEMachine() == ELF::EM_ARM.
> > 
> > There is probably not much point in testing if the TextSection already has the flag as adding it when it already has it is cheap and harmless.
> > 
> > I've not a great idea about how to remove the setFlags with the ARM specific ELF::SHF_ARM_PURECODE flag. It may be possible to add a hook into the backend to get the appropriate flag or add some kind of hook to add it. I've not got a strong opinion here, but others may have.
> > 
> > I think it is worth making the comment explain why the .text section has to be made execute only.
> > ```
> > The mix of execute-only and non-execute-only at link time is non-execute-only. To avoid the empty implicitly created .text section that a user cannot access from making the .text section non-execute-only; we mark it execute-only if it is empty and there is at least one execute-only section in the object.
> > ``` 
> Ah this is much simpler, thanks!
> 
> I've moved the ARM check to it's own if-statement. Other architectures could be added in a similar fashion, but if there's a preference from other reviewers to add a more generic solution that avoids individual checks I'm open to it.
You could move it to a virtual of "add target specific flags" and put it in lib/Target/ARM/MCTargetDesc/ARMELFObjectWriter.cpp?


================
Comment at: lib/MC/ELFObjectWriter.cpp:1123
+      }
+    }
   }
----------------
ivanlozano wrote:
> eugenis wrote:
> > My understanding of MC infrastructure is a bit fuzzy, but this looks like a strange fixup where in most cases (but not always) the section would have correct flags set up already.
> > 
> > Also, is hasInstructions() correct predicate? What if there are data fragments in .text?
> > 
> > Would it be possible (and would it solve the issue?) to track "contains non-instructions" property on any section in a target-independent way, and then set SHF_ARM_PURECODE later in ARM-specific code when we are ready to emit the binary? The way we would not need to access the target when an empty text section is created.
> > 
> You're probably right that we need to search for data fragments as well. The empty .text contains an empty MCDataFragment and MCAlignFragment, so just looking for the absence of these is insufficient.
> 
> If we go the route of a tracking property, what would be the best place to set this value? MCObjectStreamer::EmitBytes looks like a good candidate, though I'm not sure if this is the only place it needs to be set. Alternatively, we could define hasData() as a function that iterates through the fragment list and checks if any of the data fragments have a non-zero size.
> 
> There's some other architecture-specific code already in this file (EM_MIPS), so I initially figured it'd be alright to include this here. I'm not opposed to moving this to ARM-specific code, but I could use some pointers as to where that might be. Alternatively we could skip emitting empty .text sections on every architecture, though I'm not sure if this would cause other issues.
> 
> 
Avoiding the empty .text sections could cause a bit of a compatibility issue, but it might be worth doing.

I added a comment above as to where you could add the target specific code.

I think the idea of tracking pure code at some level is a great idea. I don't know if it's worth tracking in the general MC layer versus additions in the ARM side of things. Reduce the general complexity and make the ARM backend handle it.

Thoughts?


https://reviews.llvm.org/D48792