Page MenuHomePhabricator

[lld] Support TLS in RISC-V
ClosedPublic

Authored by PkmX on Oct 26 2017, 1:27 AM.

Details

Reviewers
ruiu
asb
espindola
Summary

This patch adds support for thread-local storage (TLS). All four TLS models: global/local-dynamic (both are the same in RISC-V), initial-exec and local-exec are supported, but TLS relaxation is not implemented yet.

In RISC-V (in glibc's implementation at least), the thread pointer (tp/x4) points to the end of TCB and the beginning of the static TLS block, and each DTV points to 0x800 past the start of the TLS block.

Diff Detail

Event Timeline

PkmX created this revision.Oct 26 2017, 1:27 AM
ruiu edited edge metadata.Oct 26 2017, 2:41 PM

Generally looking good.

ELF/Arch/RISCV.cpp
21

I think there's no reason to use a preprocessor macro. Why don't you use constexpr?

54–57

It is probably easier to read if you write in this way:

if (Config->Is64) {
} else {
}
PkmX updated this revision to Diff 120543.Oct 26 2017, 11:38 PM
PkmX marked 2 inline comments as done.
PkmX updated this revision to Diff 120796.Oct 30 2017, 4:26 AM

De-template RISCV class.

ruiu added inline comments.Oct 30 2017, 1:40 PM
ELF/Arch/RISCV.cpp
21

Can you make this static?

ELF/InputSection.cpp
751–752

Hmm, was that just mis-designed?

PkmX updated this revision to Diff 120941.Oct 30 2017, 11:56 PM
PkmX marked an inline comment as done.
  • Mark TLS_DTP_OFFSET as internal linkage.
PkmX updated this revision to Diff 121839.Nov 6 2017, 11:13 PM

Rebase onto latest master

PkmX updated this revision to Diff 127858.Dec 21 2017, 3:23 AM

Rebase onto latest master.

PkmX updated this revision to Diff 133981.Feb 12 2018, 7:33 PM

Rebase onto master.

PkmX updated this revision to Diff 158928.Aug 2 2018, 11:51 PM

Rebased. NFC from last revision.

jrtc27 added a subscriber: jrtc27.Oct 30 2018, 7:38 AM
jrtc27 added inline comments.
ELF/InputSection.cpp
752

Perhaps we should have a TlsVariant1 boolean member (or a TlsVariant enum, but that seems overkill when there are only two variants), so we can avoid this?

Other than my single minor point (and D39323), is there anything blocking this from being accepted?

Oh, since D39323 needs rebasing, this will also need to be rebased again.

ELF/Arch/RISCV.cpp
47–48

Should probably move this into the if/else below?

PkmX added a comment.Oct 30 2018, 8:12 AM

Other than my single minor point (and D39323), is there anything blocking this from being accepted?

I will upload rebased diffs later.

ELF/InputSection.cpp
752

I think there are three variants now:

  • Variant 1: TP points to beginning of TCB which has a fixed size, followed by TLS blocks.
  • Variant 2: TP points to beginning of TCB with unspecified size, and TLS blocks are placed before TP.
  • Variant 3: TP points to the end of TCB with unspecified size, and TLS blocks follows TP. This is essentially a reversed version of variant 2.

I agree there should probably an enum to distinguish these variants.

jrtc27 added inline comments.Oct 30 2018, 8:37 AM
ELF/InputSection.cpp
752

Well, nobody that I know if is calling it variant 3, they're reusing the variant 1 machinery (as are we), so I think that would just add to the confusion. This is the same as PPC64 except with a zero TlsTpOffset (and a smaller DTP offset), and we don't call that variant 3.

PkmX added inline comments.Oct 30 2018, 8:51 AM
ELF/InputSection.cpp
752

Yeah, I suppose you can also say that it is variant 1 with a zero-sized TCB for the purpose of calculating TP offsets.

Still we need to be a separate flag/enum to distinguish that. Using TcbSize == 0 is sort of a hack here.

ruiu added a comment.Oct 30 2018, 1:09 PM

For lld/ELF, we generally write tests in assembly rather than YAML. If you can write these tests in assembly, please do.

For lld/ELF, we generally write tests in assembly rather than YAML. If you can write these tests in assembly, please do.

Unfortunately llvm-mc doesn't yet support any of these (unless a commit has passed me by, despite being subscribed to anything touching lib/Target/RISCV). I'm personally implementing some of this myself, but for now if we want tests we need to use YAML...

ruiu added a comment.Oct 30 2018, 1:18 PM

Did you create these tests files from assembly files? If so, I'd add assembly as comments to keep them, so that we can rewrite them to assembly without re-inventing the same assembly files in the future.

PkmX added a comment.Nov 1 2018, 12:27 AM

Did you create these tests files from assembly files? If so, I'd add assembly as comments to keep them, so that we can rewrite them to assembly without re-inventing the same assembly files in the future.

The original assembly are already in the comments.

As @jrtc27 mentioned, LLVM MC does not support RISC-V's TLS pseudo instructions yet.

ruiu accepted this revision.Nov 1 2018, 11:24 AM

LGTM

ELF/Arch/RISCV.cpp
353–357

I think we should inline TLS_DTP_OFFSET with a comment saying that 0x800 is the offset to the DTV. We write lots of magic numbers with comments in this function already.

This revision is now accepted and ready to land.Nov 1 2018, 11:24 AM
PkmX updated this revision to Diff 172308.Nov 1 2018, 11:36 PM
PkmX marked an inline comment as done.

Rebased onto trunk.

Ping; still a couple of tiny points.

ELF/Arch/RISCV.cpp
47–48

This is still outstanding.

353–357

PkmX, the idea is to replace TLS_DTP_OFFSET with the constant literal, not declare it here.

@PkmX Ping, any chance you could address the remaining few points, please?

PkmX updated this revision to Diff 194179.Apr 8 2019, 11:20 AM
  • Rebased onto master
  • Moved GotRel into if (Config->Is64)
  • Use a literal constant for TLS_DTP_OFFSET.
  • Rewrite all tests in MC. However this depends on D55667 to assemble la.tls.ie and la.tls.gd, which is not accepted yet.
ruiu accepted this revision.Apr 10 2019, 3:29 AM

LGTM

lenary added a subscriber: lenary.Aug 15 2019, 2:49 AM

Am I correct in thinking that this functionality has already been upstreamed into LLD, and that this patch can now been abandoned?

ruiu added a comment.Aug 19 2019, 2:43 AM

This has been merged on July 2 as http://reviews.llvm.org/rL364813

MaskRay closed this revision.Aug 19 2019, 2:55 AM