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

[Ivan Lozano via Phabricator via llvm-commits]

ivanlozano 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
----------------
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.


================
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
----------------
peter.smith wrote:
> Have we got a test case for adding SHF_ARM_PURECODE to the 0 sized .text section?
Yup, this check is implicitly in test/MC/ELF/ARM/execute-only-section.s, which contains a 0 sized .text section.


https://reviews.llvm.org/D48792