Page MenuHomePhabricator

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

Authored by skan on Apr 12 2020, 6:46 AM.

Details

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.

Diff Detail

Event Timeline

skan created this revision.Apr 12 2020, 6:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2020, 6:46 AM
skan edited the summary of this revision. (Show Details)Apr 12 2020, 6:53 AM
skan added reviewers: MaskRay, reames, LuoYuanke.
LuoYuanke added inline comments.Apr 12 2020, 6:31 PM
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 marked an inline comment as done.Apr 12 2020, 6:49 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
570

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

LGTM. @MaskRay Do you have time to review?

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 added a comment.Apr 14 2020, 12:21 AM

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 added a comment.Apr 14 2020, 12:31 AM

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 added a comment.Apr 14 2020, 12:37 AM

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

skan added a subscriber: annita.zhang.
MaskRay added a comment.EditedApr 14 2020, 11:15 AM

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 added a comment.Apr 14 2020, 7:00 PM

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

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 marked an inline comment as done.Apr 15 2020, 1:16 AM

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 updated this revision to Diff 257621.Apr 15 2020, 1:32 AM
skan edited the summary of this revision. (Show Details)

Rebase

MaskRay added inline comments.Apr 15 2020, 10:19 AM
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 added a comment.Apr 15 2020, 7:32 PM

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?

skan edited the summary of this revision. (Show Details)Apr 15 2020, 8:55 PM

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 added a comment.Apr 15 2020, 9:28 PM

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 added inline comments.Apr 15 2020, 10:27 PM
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
MaskRay retitled this revision from [MC][X86][NFC] Disable branch align in non-text section to [MC][X86] Disable branch align in non-text section.Apr 15 2020, 11:04 PM
skan updated this revision to Diff 257973.Apr 16 2020, 12:20 AM

Address review comments

MaskRay added inline comments.Apr 16 2020, 9:31 AM
llvm/test/MC/X86/align-branch-section-type.s
11

These lines don't need to be indented..

skan updated this revision to Diff 258219.Apr 16 2020, 6:54 PM

Address review comments

skan added a comment.Apr 17 2020, 7:00 AM

@MaskRay Is this acceptable?

MaskRay accepted this revision.Apr 17 2020, 7:43 AM

Looks great!

This revision is now accepted and ready to land.Apr 17 2020, 7:43 AM
This revision was automatically updated to reflect the committed changes.