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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've left some brief comments. As this is hitting the generic parts of MC it may be worth casting the net wider to look for some more reviewers?



================
Comment at: lib/MC/ELFObjectWriter.cpp:1096
     }
+
+    // For ARM, we need to add SHF_ARM_PURECODE to an empty TextSection if any
----------------
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.
``` 


================
Comment at: test/MC/ARM/elf-execute-only-section.ll:1
+; RUN: llc < %s -mtriple=thumbv8m.base-eabi -mattr=+execute-only -filetype=obj %s -o - | \
+; RUN: llvm-readelf -s | FileCheck %s
----------------
Have we got a test case for adding SHF_ARM_PURECODE to the 0 sized .text section?


https://reviews.llvm.org/D48792