[ARM] Set execute-only flags in .text.

[ivanlozano]

Summary

The initial .text section generated in object files was missing the SHF_ARM_PURECODE flag when being built with the -mexecute-only flag. All code sections of an ELF must have the flag set for the final .text section to be execute-only, otherwise the flag gets removed.

This change restores some code that performed this from rL289784, though the subtarget information is accessed differently.

This also sets the flag for an empty .text section in later link steps where the -mexecute-only flag is no longer provided, but at least one code section has the flag set.

A HasData flag is added to MCSection to aid in the determination that the section is empty. A virtual setTargetSectionFlags is added to MCELFObjectTargetWriter to allow subclasses to set target specific section flags to be added to sections.

[peter.smith]

I think that setting SHF_ARM_PURECODE for the initial .text section is the right thing to do; the flag is a declaration that the section does not read from the .text section, and that property is satisfied by an empty section. For reference the code you've restored was removed in rL306927 "Rewrite ARM execute only support to avoid the use of a command line flag and unqualified ARMSubtarget lookup." I think the unqualified ARMSubtarget lookup might need a bit more exploration; for example what happens if this is the link step of a LTO build where the functions have ARMSubtargets with SHF_ARM_PURECODE but the command line to the link step has no code generation flags I think that the ARMSubtarget is likely to not have the ARM::FeatureExecuteOnly. If this is the case then we may need an alternative approach. At a glance would it be possible to always add SHF_ARM_PURECODE to the initial .text section?

[ivanlozano]

Yea I wasn't sure exactly which approach to take here. Initially I thought that setting the TextSection to always include SHF_ARM_PURECODE was the way to go since I hadn't seen that section being used when generating object files from source. However I discovered from running the test suite that this section is used when assembling machine code and outputting an object-file with llvm-mc. This erroneously set the SHF_ARM_PURECODE flag when no execute-only flag was provided. This leads me to believe there may be other scenarios where this section is used as well.

With regard to the LTO question, I'm unfamiliar with how it works so that's not something I've considered. I am definitely open to suggestions if we need to avoid the Subtarget lookup.

[peter.smith]

As an example for LTO, or just using a single bitcode file I'm thinking of something like

__attribute__((noinline)) int func1(void) {
    return 123456789;
}

__attribute__((noinline)) int func2(void) {
    return 987654321;
}

int main(void) {
    return func2() + func1();
}

When I compile this with

clang --target=thumb-linux-gnueabihf -emit-llvm -O1 -mexecute-only -o t.bc t.c -mcpu=cortex-m3 -c

Then the bitcode file will have the execute-only target feature embedded in the per function attributes. For example:

; Function Attrs: noinline norecurse nounwind readnone
define dso_local i32 @func1() local_unnamed_addr #0 {
entry:
  ret i32 123456789
}
...
attributes #0 = { noinline, ..., "target-features"="+armv7-m,+execute-only,+hwdiv,+thumb-mode,-crc,-dotprod,-dsp,-hwdiv-arm,-ras" "unsafe-fp-math"="false" "use-soft-float"="false" }

We can then generate code for the bitcode file without passing in -mexecute-only. This is similar to the final "link-step" in a LTO build where the compilation options might not be given by the build system:

clang t.bc -c --target=thumb-linux-gnueabihf -mcpu=cortex-m3

We can see that purecode has still been generated for the .text section:

[ 2] .text             PROGBITS        00000000 000034 000000 00  AX  0   0  4
[ 3] .text             PROGBITS        00000000 000034 00001e 00 AXp  0   0  2

This is with an unpatched clang so my initial 0 sized section doesn't have purecode. If I'm right I'm expecting that the target feature of the initial MCSubtarget won't have execute-only as I didn't pass it on the command line so I'd expect to see the zero sized text section to not have SHF_ARM_PURECODE even when the above patch is applied. I've not got any great ideas of how to solve that right now, perhaps do the change of the TextSection at a later stage when we know if there is at least one function with the execute-only target-feature.

[ivanlozano]

Yea this is what I'm seeing as well with the patched version unfortunately. I'll poke around the code and see if there's anywhere appropriate where we might be able to change the initial TextSection at a later stage.

[ivanlozano]

The updated patch adds support for the case peter.smith brought up by checking for an empty .text section that's paired with SHF_ARM_PURECODE sections.

There's another case which I'm not sure how to address. When compiling the default .text section, the section will not have SHF_ARM_PURECODE set even if -mexecute-only is provided. This can be illustrated in the simple case:

.text
  bx lr

Passing this along to the compiler:

./clang -target armv6t2-eabi -mexecute-only t.s -c

results in a .text section that does not have the flag set:

[ 2] .text             PROGBITS        00000000 000034 000004 00  AX  0   0  4

Maybe this is expected behavior, but explicitly setting the flags for a section named .text fails to set the flags as well

.section .text, "axy" 
  bx lr

I think the section flags for .text are being set in ELFAsmParser::ParseSectionDirectiveText. It's not clear to me what the fix would look like though. (Maybe a change in ARMAsmParser? I'm unfamiliar with how the AsmParser works though).

[ivanlozano]

I'll put together a separate patch (probably next week) for the .text assembly issue to keep this patch from growing more complicated, and to have the discussion in a dedicated thread. I've got an idea for how to handle it which basically boils down to passing execute-only to cc1as and making a couple changes to ARMAsmParser and MCTargetOptions, but there are some details that need to be hammered out still.

Unless anyone feels strongly that should be included in this patch, this is ready for review and comments. Thanks!

[peter.smith]

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?

lib/MC/ELFObjectWriter.cpp

1096 ↗

(On Diff #158867)

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.

test/MC/ARM/elf-execute-only-section.ll

2

Have we got a test case for adding SHF_ARM_PURECODE to the 0 sized .text section?

[ivanlozano]

lib/MC/ELFObjectWriter.cpp

1096 ↗

(On Diff #158867)

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.

test/MC/ARM/elf-execute-only-section.ll

2

Yup, this check is implicitly in test/MC/ELF/ARM/execute-only-section.s, which contains a 0 sized .text section.

[peter.smith]

Thanks for the update. This looks good to me. It may be worth waiting a few days before committing to see if there are any objections/other opinions given that this touches some code shared between multiple backends.

[eugenis]

lib/MC/ELFObjectWriter.cpp

1123 ↗

(On Diff #160630)

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.

[ivanlozano]

lib/MC/ELFObjectWriter.cpp

1123 ↗

(On Diff #160630)

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.

[ivanlozano]

Added the additional HasData property and check to see if data has been emitted to the .text section.

[ivanlozano]

Friendly ping for anyone familiar with the MC infrastructure to take a look at this. Thanks.

[peter.smith]

I've added a couple of comments. One other thing to think about is the -S output from the compiler. If I compile a trivial test case with -mexecute-only I get the output:

	.text
	.syntax unified
	.eabi_attribute	67, "2.09"	@ Tag_conformance
...
	.section	.text,"axy",%progbits,unique,0
	.globl	func                    @ -- Begin function func
...

I'm not sure how easy it will be to change .text to .section .text, "axy", %progbits. In theory if this is assembled with MC and this patch it should come out with execute only (may be worth a test case). Re-assembling with GNU as will likely create a .text section without SHF_ARM_PURECODE but I don't think that this isn't likely enough in practice to worry too much about.

lib/MC/ELFObjectWriter.cpp

1121 ↗

(On Diff #162026)

If the TextSection is completely empty it should have 0 fragments going into MCAssembler::layout(). The layout function then creates a single empty MCDataFragment. I'm thinking it might be more precise and reliable to set a flag like HasNoUserFragments in layout(). That flag can be tested for here.

lib/MC/MCObjectStreamer.cpp

499 ↗

(On Diff #162026)

I think that this would catch the majority of cases where data is written but could we be sure it will catch all of them. For example it is possible to write something like:

.text
.balign 32

That will create an MCAlignFragment that doesn't go via emitBytes(). Although one would expect that the alignment padding here wouldn't be read I guess we couldn't be 100% sure.

[ivanlozano]

In D48792#1217179, @peter.smith wrote:

I've added a couple of comments. One other thing to think about is the -S output from the compiler. If I compile a trivial test case with -mexecute-only I get the output:

	.text
	.syntax unified
	.eabi_attribute	67, "2.09"	@ Tag_conformance
...
	.section	.text,"axy",%progbits,unique,0
	.globl	func                    @ -- Begin function func
...

I'm not sure how easy it will be to change .text to .section .text, "axy", %progbits. In theory if this is assembled with MC and this patch it should come out with execute only (may be worth a test case). Re-assembling with GNU as will likely create a .text section without SHF_ARM_PURECODE but I don't think that this isn't likely enough in practice to worry too much about.

I tested this with llc and Clang, the output section in both cases has the flag set correctly:

.section        .text,"axy",%progbits,unique,0

I'll go ahead and add a test for this.

lib/MC/ELFObjectWriter.cpp

1121 ↗

(On Diff #162026)

This seems reasonable, I'll put this together and see that it works as expected.

lib/MC/MCObjectStreamer.cpp

499 ↗

(On Diff #162026)

Yea I'm also not sure if there isn't somewhere else this needs to be set. However at least in the .balign case it is emitting the appropriate section flags when I test it. The resulting .text in the object file is still size 0 and marked execute-only.

I think .balign only pads if the size isn't already a multiple of N, and .align and .p2align function similarly.

[ivanlozano]

Added a new check condition to test/CodeGen/ARM/execute-only.ll to make sure the bare ".text" section is not emitted in assembly output that should be execute-only.

lib/MC/ELFObjectWriter.cpp

1121 ↗

(On Diff #162026)

Unfortunately by the time we get to MCAssembler::layout(), the .text section already contains empty fragments.

Looking at MCObjectFileInfo::initELFMCObjectFileInfo where the text section is created, it contains an empty MCDataFragment upon creation, so I don't think the absence of fragments is something we can look for. We could iterate through each fragment and check to make sure they're all size 0, but this seems clunky.

[peter.smith]

Thanks for the update. I still think this looks good to me.

I'm out of quick ideas for how to do this in a better way. The only other ways that I can think of are to not pre-create the .text section and create in on demand if needed; or find some way of deleting it afterwards. These may be possible but will be more disruptive. I think that this will handle the LTO case where the "link-step" clang invocation doesn't get passed the -fexecute-only flag, which IIRC was the main reason not to just create the TextSection with the execute-only flag in the first place based on the presence of -fexecute-only.

[ivanlozano]

In D48792#1219092, @peter.smith wrote:

Thanks for the update. I still think this looks good to me.

I'm out of quick ideas for how to do this in a better way. The only other ways that I can think of are to not pre-create the .text section and create in on demand if needed; or find some way of deleting it afterwards. These may be possible but will be more disruptive. I think that this will handle the LTO case where the "link-step" clang invocation doesn't get passed the -fexecute-only flag, which IIRC was the main reason not to just create the TextSection with the execute-only flag in the first place based on the presence of -fexecute-only.

Thanks! Yea ideally I'd like to avoid this being a more disruptive change than it needs to be; not pre-creating the .text section seems like a heavier lift that may warrant it's own patch (or set of patches) from someone more well versed in this code.

[ivanlozano]

Just to follow up on the earlier issue I mentioned where handwritten assembly sections named .text were not getting the execute-only flag applied ( D48792#1186500 ), it seems I was mistaken. Defining the section as below does result in the execute-only flags being set properly in the object file.

.section .text,"axy",%progbits,unique,0;

[echristo]

lib/MC/ELFObjectWriter.cpp

1123 ↗

(On Diff #160630)

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?

1096 ↗

(On Diff #158867)

You could move it to a virtual of "add target specific flags" and put it in lib/Target/ARM/MCTargetDesc/ARMELFObjectWriter.cpp?

[ivanlozano]

Added a setTargetSectionFlags virtual method to MCELFObjectTargetWriter and moved the logic that modifies the TextSection flags over to the ARMELFObjectWriter.

lib/MC/ELFObjectWriter.cpp

1123 ↗

(On Diff #160630)

If we avoid emitting empty .text sections, it causes ~100 tests to fail (at least when I tried this quickly). It's probably best to handle it in a separate patch-set to ensure the impact can be properly considered.

[echristo]

lib/MC/MCObjectStreamer.cpp

500 ↗

(On Diff #164110)

Given that the only target that cares about hasData is the ARM backend you could probably override this there and only worry about setting it within that backend?

[ivanlozano]

Added additional comments to clarify to the source code additions.

lib/MC/MCObjectStreamer.cpp

500 ↗

(On Diff #164110)

Chatted offline with echristo and decided this might add a bit of complexity to the patch (we'd need to define an ARM-specific MCSectionELF to sink the entire hasData flag into the ARM backend). So I'm adding a note to the HasData flag that it's only used by the ARM backend right now.

[echristo]

LGTM

[ivanlozano]

Thanks echristo!

I think the folks who've needed to look at this have done so and given it the OK. If someone with commit access could please submit this I'd very much appreciate it, thanks!

[echristo]

I'll get it tomorrow if no one does before.

lib/Target/ARM/ARMTargetObjectFile.cpp

 //===----------------------------------------------------------------------===//
 //                               ELF Target
 //===----------------------------------------------------------------------===//
 
 void ARMElfTargetObjectFile::Initialize(MCContext &Ctx,
                                         const TargetMachine &TM) {
   const ARMBaseTargetMachine &ARM_TM = static_cast<const ARMBaseTargetMachine &>(TM);
   bool isAAPCS_ABI = ARM_TM.TargetABI == ARMBaseTargetMachine::ARMABI::ARM_ABI_AAPCS;
-  //  genExecuteOnly = ARM_TM.getSubtargetImpl()->genExecuteOnly();
+  bool genExecuteOnly = ARM_TM.getMCSubtargetInfo()->hasFeature(ARM::FeatureExecuteOnly);
 
   TargetLoweringObjectFileELF::Initialize(Ctx, TM);
   InitializeELF(isAAPCS_ABI);
 
   if (isAAPCS_ABI) {
     LSDASection = nullptr;
   }
+
+  // Make code section unreadable when in execute-only mode
+  if (genExecuteOnly) {
+    unsigned  Type = ELF::SHT_PROGBITS;
+    unsigned Flags = ELF::SHF_EXECINSTR | ELF::SHF_ALLOC | ELF::SHF_ARM_PURECODE;
+    // Since we cannot modify flags for an existing section, we create a new
+    // section with the right flags, and use 0 as the unique ID for
+    // execute-only text
+    TextSection = Ctx.getELFSection(".text", Type, Flags, 0, "", 0U);
+  }
 }
 
 const MCExpr *ARMElfTargetObjectFile::getTTypeGlobalReference(
     const GlobalValue *GV, unsigned Encoding, const TargetMachine &TM,
     MachineModuleInfo *MMI, MCStreamer &Streamer) const {
   if (TM.getMCAsmInfo()->getExceptionHandlingType() != ExceptionHandling::ARM)
     return TargetLoweringObjectFileELF::getTTypeGlobalReference(
         GV, Encoding, TM, MMI, Streamer);

test/MC/ARM/elf-execute-only-section.ll

+; RUN: llc < %s -mtriple=thumbv8m.base-eabi -mattr=+execute-only -filetype=obj %s -o - | \
+; RUN: llvm-readelf -s | FileCheck %s
+; RUN: llc < %s -mtriple=thumbv8m.main-eabi -mattr=+execute-only -filetype=obj %s -o - | \
+; RUN: llvm-readelf -s | FileCheck %s
+; RUN: llc < %s -mtriple=thumbv7m-eabi -mattr=+execute-only -filetype=obj %s -o - | \
+; RUN: llvm-readelf -s | FileCheck %s
+
+; CHECK-NOT: {{.text[ ]+PROGBITS[ ]+[0-9]+ [0-9]+ [0-9]+ [0-9]+ AX[^p]}}
+; CHECK: {{.text[ ]+PROGBITS[ ]+[0-9]+ [0-9]+ [0-9]+ [0-9]+ AXp}}
+define void @test_func() {
+entry:
+  ret void
+}