[MC][X86] Disable branch align in non-text section

[skan]

Summary

The instruction in non-text section can not be executed, so they will not affect performance.
In addition, their encoding values are treated as data, so we should not touch them.

[LuoYuanke]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

570

This function in only called in emitInstructionBegin and emitInstructionEnd, so there is no bug? But you want to double check that it only happen in text section. Right?

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

570

No. We can emit instruction in a non-text section, see align-branch-section-type.s

[skan]

Ping

[LuoYuanke]

LGTM. @MaskRay Do you have time to review?

[MaskRay]

Non-text sections with instructions seem invalid cases to me, so I am not sure we need the check. If you can find a legitimate case where LLVM emits instructions, I think this patch will make sense.

llvm/test/MC/X86/align-branch-section-type.s

17

The SHF_EXCLUDE and SHF_TLS tests don't make sense at all.

[skan]

In D77971#1979750, @MaskRay wrote:

Non-text sections with instructions seem invalid cases to me, so I am not sure we need the check. If you can find a legitimate case where LLVM emits instructions, I think this patch will make sense.

llvm/test/MC/X86/reloc-bss.s

# RUN: not --crash llvm-mc -filetype=obj -triple=x86_64-linux-gnu %s 2>&1 | FileCheck %s
# CHECK: LLVM ERROR: cannot have fixups in virtual section!

.section        .init_array,"awT",@nobits

.hidden patatino
.globl  patatino
patatino:
  movl __init_array_start, %eax

[skan]

And test

llvm/test/MC/X86/relax-offset.s

  .section .text1
 .skip after-before,0x0
.Lint80_keep_stack:

  .section .text2
before:
 jmp .Lint80_keep_stack
after:

Section .text1 and .text2 are not executable, so they are not recognized as text section in LLVM, but .text2 do contains instruction

[skan]

The encoding value of instruction are data, so I think we can place instructions in non-text section as if they are data (At least LLVM allows us to do so currently.)

[MaskRay]

In D77971#1979843, @skan wrote:

And test

llvm/test/MC/X86/relax-offset.s

  .section .text1
 .skip after-before,0x0
.Lint80_keep_stack:

  .section .text2
before:
 jmp .Lint80_keep_stack
after:

Section .text1 and .text2 are not executable, so they are not recognized as text section in LLVM, but .text2 do contains instruction

For invalid user input, we should not trigger report_fatal_error actually. See Eli's comment at https://reviews.llvm.org/D72194#1957329 I am preparing a patch to fix this particular problem.

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior. Then erroring out is as good as not erroring out. For an "undefined behavior" I would prefer a diagnostic if a user can reasonably cause the error, but this probably requires some testing on some larger code bases. I hope no project depends on the erroneous behavior. If we don't error, I would prefer the simple choice: don't do anything specially.

Edit: created D78138

[skan]

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior. Then erroring out is as good as not erroring out. For an "undefined behavior" I would prefer a diagnostic if a user can reasonably cause the error, but this probably requires some testing on some larger code bases. I hope no project depends on the erroneous behavior. If we don't error, I would prefer the simple choice: don't do anything specially.

So, to make things easier, could I commit the change in align-branch-section-size.s in a NFC first? @MaskRay

[MaskRay]

In D77971#1982624, @skan wrote:

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior. Then erroring out is as good as not erroring out. For an "undefined behavior" I would prefer a diagnostic if a user can reasonably cause the error, but this probably requires some testing on some larger code bases. I hope no project depends on the erroneous behavior. If we don't error, I would prefer the simple choice: don't do anything specially.

So, to make things easier, could I commit the change in align-branch-section-size.s in a NFC first? @MaskRay

llvm/test/MC/X86/align-branch-section-size.s looks good. I might suggest "ax" originally...

You can delete the label foo:, which isn't used.

llvm/test/MC/X86/align-branch-section-size.s

6 ↗

(On Diff #256850)

Such test is fragile.

# CHECK: Name: text1
# CHECK: AddressAlignment:
# CHECK:                   32{$}

may be slightly better.

[skan]

In D77971#1982735, @MaskRay wrote:

You can delete the label foo:, which isn't used.

label foo is used by jmp.

llvm/test/MC/X86/align-branch-section-size.s

6 ↗

(On Diff #256850)

It's strange to write like this. It may check something like

AddressAlignment:  1
32

Lots of tests use the format CHECK: AddressAlignment: 1, such as test/tools/llvm-readobj/ELF/sections.test. So I prefer to not change.

[skan]

Rebase

[MaskRay]

llvm/test/MC/X86/align-branch-section-size.s

6 ↗

(On Diff #256850)

Sorry, I meant:

# CHECK:      Name: text1
# CHECK:      AddressAlignment:
# CHECK-NEXT:                   32{$}

Now I think this is better:

# CHECK-LABEL: Name: text1
# CHECK:       AddressAlignment: 32

LABEL is checked before other directives, so we can have good diagnostics when things go off.

[skan]

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

I asked some experts about this, and got the answer that putting instructions in non-executable section is legal and their encoding values will be treated as data. Do you have other concern about this patch?

[MaskRay]

In D77971#1985717, @skan wrote:

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

I asked some experts about this, and got the answer that putting instructions in non-executable section is legal and their encoding values will be treated as data. Do you have other concern about this patch?

Can you add some information to the description? Ideally there should be an example that instructions in a non-executable section are useful.

Personally I like this resolution because that means we don't need to extend D78138 (bss like sections) to non-executable sections.

[skan]

In D77971#1985752, @MaskRay wrote:

In D77971#1985717, @skan wrote:

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

I asked some experts about this, and got the answer that putting instructions in non-executable section is legal and their encoding values will be treated as data. Do you have other concern about this patch?

Can you add some information to the description? Ideally there should be an example that instructions in a non-executable section are useful.

Personally I like this resolution because that means we don't need to extend D78138 (bss like sections) to non-executable sections.

I added some information about why we need this check in the summary. It's difficult for me to give some useful examples, but I know putting instructions in non-executable sections are semantically correct.

[MaskRay]

llvm/test/MC/X86/align-branch-section-type.s

4

Don't indent the file...

Use ## for comments

6

I have suggested the following in a previous comment:

# CHECK-LABEL: Name: text
# CHECK:       AddressAlignment: 32

[skan]

Address review comments

[MaskRay]

llvm/test/MC/X86/align-branch-section-type.s

10

These lines don't need to be indented..

[skan]

Address review comments

[skan]

@MaskRay Is this acceptable?

[MaskRay]

Looks great!

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

bool X86AsmBackend::canPadInst(const MCInst &Inst, MCObjectStreamer &OS) const {
   return true;
 }
 
 bool X86AsmBackend::canPadBranches(MCObjectStreamer &OS) const {
   if (!OS.getAllowAutoPadding())
     return false;
   assert(allowAutoPadding() && "incorrect initialization!");
 
+  // We only pad in text section.
+  if (!OS.getCurrentSectionOnly()->getKind().isText())
+    return false;
+
   // To be Done: Currently don't deal with Bundle cases.
   if (OS.getAssembler().isBundlingEnabled())
     return false;
 
   // Branches only need to be aligned in 32-bit or 64-bit mode.
   if (!(STI.hasFeature(X86::Mode64Bit) || STI.hasFeature(X86::Mode32Bit)))
     return false;
 

llvm/test/MC/X86/align-branch-section-type.s

+# RUN: llvm-mc -filetype=obj -triple x86_64 --x86-align-branch-boundary=32 --x86-align-branch=ret %s | llvm-readobj -S | FileCheck %s
+
+## Check we only pad in a text section
+
+  # CHECK-LABEL:  Name: text
+  # CHECK:        AddressAlignment: 32
+  .section text, "ax"
+  ret
+
+  # CHECK-LABEL:  Name: excluded
+  # CHECK:        AddressAlignment: 1
+  .section excluded, "e"
+  ret
+
+  # CHECK-LABEL:  Name: tls
+  # CHECK:        AddressAlignment: 1
+  .section tls, "awT"
+  ret