Page MenuHomePhabricator

[ELF][RISCV] Support GD/LD/IE/LE TLS models
ClosedPublic

Authored by MaskRay on Jun 12 2019, 9:56 AM.

Details

Summary

RISC-V psABI doesn't specify TLS relaxation. Though the code sequences
are not relaxed, dynamic relocations DTPMOD/DTPREL can be omitted if the
TLS model can be relaxed to Local-Exec.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jun 12 2019, 9:56 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 204432.Jun 12 2019, 11:13 PM

Support all 4 models

MaskRay updated this revision to Diff 204438.Jun 13 2019, 12:33 AM
MaskRay retitled this revision from [WIP][ELF][RISCV] Support GD/LD TLS models to [ELF][RISCV] Support GD/LD/IE/LE TLS models.
MaskRay edited the summary of this revision. (Show Details)
MaskRay added reviewers: jrtc27, PkmX, ruiu.
MaskRay removed subscribers: jrtc27, PkmX.

Support GD/LD/IE/LE TLS models

MaskRay updated this revision to Diff 204441.Jun 13 2019, 12:46 AM

Delete an unrelated change (moving In.RelaDyn)

ruiu accepted this revision.Jun 13 2019, 12:47 AM

LGTM

This revision is now accepted and ready to land.Jun 13 2019, 12:47 AM
jrtc27 requested changes to this revision.Jun 13 2019, 1:55 AM
jrtc27 added inline comments.
ELF/Arch/RISCV.cpp
77 ↗(On Diff #204441)

I think it would be better to fold this into the if.

ELF/Relocations.cpp
224 ↗(On Diff #204441)

The RISC-V behaviour is identical to ARM if I remember correctly, so we should be reusing that code rather than reimplementing it again here.

test/ELF/riscv-tls-ld.s
1 ↗(On Diff #204441)

This file is the same as GD just with different check prefixes, no? RISC-V’s local dynamic is the same as global dynamic as far as relocations are concerned.

This revision now requires changes to proceed.Jun 13 2019, 1:55 AM
MaskRay marked 2 inline comments as done.Jun 13 2019, 2:12 AM
MaskRay added inline comments.
ELF/Relocations.cpp
224 ↗(On Diff #204441)

R_ARM_TLS_GD32 <-> R_RISCV_TLS_GD_HI20
R_ARM_TLS_LDM32 <-> R_RISCV_TLS_GD_HI20 with a local symbol .LANCHOR0
R_ARM_TLS_IE32 <-> R_RISCV_TLS_GOT_HI20
R_ARM_TLS_LE32 <-> R_RISCV_TPREL_{HI20,LO12_I,LO12_S}

If this is what you mean :)

However, there is a big difference: RISC-V doesn't support relaxation. So I have to make sure no R_RELAX_TLS_* code is called.

Having the two variables is the least intrusive approach I can think of. (I complained the lack of linker relaxation turns out to make lld's generic TLS handling uglier. Here it is :))

test/ELF/riscv-tls-ld.s
1 ↗(On Diff #204441)

No exactly the same.

riscv-tls-gd.s checks preemptable symbols. This file checks .LANCHOR0. I do struggle to make the tests simpler while covering useful/possible scenarios.

MaskRay updated this revision to Diff 204454.Jun 13 2019, 2:15 AM

Fold Config->Is64 ? :

MaskRay added a comment.EditedJun 13 2019, 2:20 AM

For the curious. I used the tests to compare the behaviors of lld and ld.bfd and found 2 problems that ld.bfd have :) https://sourceware.org/bugzilla/show_bug.cgi?id=24673 https://sourceware.org/bugzilla/show_bug.cgi?id=24676

I think it is fine to never have GD->IE, GD->LE, LD->LE, if we can get deprecate GD/LD (R_RISCV_TLS_GD_HI20) and migrate to TLSDESC (https://github.com/riscv/riscv-elf-psabi-doc/issues/94). However, IE->LE is still useful. It would be nice if psABI specified that.

MaskRay updated this revision to Diff 204688.Jun 13 2019, 7:59 PM

Add DF_STATIC_TLS
Explain LD references a local symbol

MaskRay marked 2 inline comments as done.Jun 14 2019, 7:50 AM
MaskRay added inline comments.
ELF/Relocations.cpp
224 ↗(On Diff #204441)

Sorry, I think you probably meant we can reuse handleARMTlsRelocation() for RISC-V. I have compared it with handleTlsRelocation() and find it may be worth merging the two functions. Created D63333 for that idea. With that patch, the Relocations.cpp change in this patch will be trivial.

- bool Relax = Config->EMachine != EM_ARM;
+ bool Relax = Config->EMachine != EM_ARM && Config->EMachine != EM_RISCV;
MaskRay marked an inline comment as done.Jun 20 2019, 7:13 AM
jrtc27 accepted this revision.Jun 20 2019, 7:59 AM

One minor pedantic stylistic comment but otherwise this seems fine to me.

ELF/Arch/RISCV.cpp
342 ↗(On Diff #205808)

This order is inconsistent with the LO12_[IS] variants. Personally I'd leave HI20/LO12_* as the final case statement before the block, and put the TLS/TPREL statements between PCREL and the plain HI20/LO12_*. This also has the benefit of reducing churn in the diff by not modifying existing lines unnecessarily.

This revision is now accepted and ready to land.Jun 20 2019, 7:59 AM
MaskRay updated this revision to Diff 205832.Jun 20 2019, 8:58 AM

Place HI20/LO12_[IS] at the bottom

This revision was automatically updated to reflect the committed changes.