Page MenuHomePhabricator

[X86][BranchAlign] Suppress branch alignment for {,_}__tls_get_addr
ClosedPublic

Authored by MaskRay on Jan 16 2020, 2:26 PM.

Details

Summary

The x86-64 General Dynamic TLS code sequence uses prefixes to allow
linker relaxation. Adding segment override prefix or NOPs can break
linker relaxation (ld -pie/-no-pie).

i386 General Dynamic and x86-64 Local Dynamic do not use prefixes, but
for simplicity, just disable auto padding consistently.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 16 2020, 2:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 2:26 PM

Unit tests: pass. 61934 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

LuoYuanke added inline comments.Jan 16 2020, 5:39 PM
llvm/test/CodeGen/X86/align-branch-boundary-suppressions-tls.ll
22

Obviously disable auto padding for TLS is safety, but I am curious what's the problem if we don't disable it? In our previous patch, we check if there is any variant symbol in needAlignInst(...).

MaskRay marked 2 inline comments as done.Jan 16 2020, 6:04 PM
MaskRay added inline comments.
llvm/test/CodeGen/X86/align-branch-boundary-suppressions-tls.ll
22

This only affects x86-64 General Dynamic TLS model, which emits some prefixes.

(DATA16_PREFIX)
(LEA64r)
<--- Diff 21 can place cs (0x2e) here and break the General Dynamic TLS code sequence --->
<MCInst 851> (DATA16_PREFIX)
<MCInst 851> (DATA16_PREFIX)
<MCInst 2450> (REX64_PREFIX)
<MCInst 602 <MCOperand Expr:(__tls_get_addr@PLT)>> CALL64pcrel32

D72225 can prepend another prefix and cause a failure.

skan added inline comments.Jan 16 2020, 6:09 PM
llvm/test/CodeGen/X86/align-branch-boundary-suppressions-tls.ll
22

Could you provide a assemble file to help us reproduce the fail? That will be more helpful.

MaskRay marked 2 inline comments as done.Jan 16 2020, 7:39 PM
MaskRay added inline comments.
llvm/test/CodeGen/X86/align-branch-boundary-suppressions-tls.ll
22
extern __thread int i;
int foo() { return i; }

-fPIC will give you a General Dynamic TLS code sequence. You can repeat the code sequence, and then add some jcc. I think you will be able to reproduce it.

This revision is now accepted and ready to land.Jan 17 2020, 12:45 AM

@skan If we do this, we can make segment-override-prefix patch less complex (not special casing prefixes).

skan accepted this revision.Jan 18 2020, 5:55 PM

LGTM

skan added a comment.Jan 18 2020, 5:59 PM

@skan If we do this, we can make segment-override-prefix patch less complex (not special casing prefixes).

I think we still need to carefully handle tls call in prefix padding patch since our test cases are written by assemble.

This revision was automatically updated to reflect the committed changes.