Page MenuHomePhabricator

[lld][ELF][SPARC] Support TLS GD relocations
Needs ReviewPublic

Authored by LemonBoy on May 20 2021, 11:57 AM.

Details

Summary

Implement the regular GD sequence and the two relaxation forms, to IE or LE.

Diff Detail

Event Timeline

LemonBoy created this revision.May 20 2021, 11:57 AM
LemonBoy requested review of this revision.May 20 2021, 11:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2021, 11:57 AM
jrtc27 added inline comments.May 20 2021, 12:17 PM
lld/ELF/Arch/SPARCV9.cpp
240–245

R_SPARC_TLS_IE_HI22/LO10? This is wrong, but I guess this is a hack that kinda works because IE isn't actually implemented yet? Which probably should happen before landing GD support... LD is less clear, but also seemingly unimplemented, only basic LE.

255

Do you mean add %g7, %r2, %r3?

lld/ELF/Driver.cpp
929 ↗(On Diff #346815)

Please keep this sorted

lld/ELF/Relocations.cpp
289–292

Should this be more like hexagonNeedsTLSSymbol/hexagonTLSSymbolUpdate and combined with it, which has similar weirdness?

296–297

Why would it need a GOT entry? SPARC doesn't even use a .got.plt, it synthesises the addresses directly in the PLT entries, but even on .got.plt architectures calls don't use the real GOT other than MIPS abicalls.

LemonBoy added inline comments.May 20 2021, 1:47 PM
lld/ELF/Arch/SPARCV9.cpp
240–245

I wouldn't call this a hack, as long as the relocation type matches it's fine to use R_SPARC_HI22 or R_SPARC_LO10 here.
I have a patch for LD, I'll send it once this is merged. I don't need IE at the moment but I'll eventually look into it.

255

Yes, it's a typo I carried over from Gold as that's the only reference for the relaxation sequences.

lld/ELF/Driver.cpp
929 ↗(On Diff #346815)

Will do, I didn't notice they were sorted.

lld/ELF/Relocations.cpp
289–292

No, I find this approach much cleaner than the late patching done for Hexagon.
I also found that adding a PLT entry that late will fail to generate a PLT section if there was none in the input files.

296–297

Ah, good catch, that was a leftover from some previous experiments.

LemonBoy updated this revision to Diff 346846.May 20 2021, 1:47 PM

Address review comments.

jrtc27 added inline comments.May 20 2021, 1:49 PM
lld/ELF/Arch/SPARCV9.cpp
255

The reference is the original Ulrich Drepper ELF TLS spec https://www.akkadia.org/drepper/tls.pdf

lld/ELF/Relocations.cpp
289–292

Hm, it's a bit ugly. Maybe @MaskRay has ideas. Does that mean Hexagon is buggy, too?

jrtc27 added inline comments.May 20 2021, 1:52 PM
lld/ELF/Arch/SPARCV9.cpp
240–245

It's a bit of an abuse. I don't think it should be committed in this form. You should at least add the IE cases needed to support GD->IE so you can use the right relocations here even if you're not adding full IE support, though full IE support should be pretty straightforward to add too other than the tedium of test writing.

Is IE supported now? I think the correct order is LE, IE, GD/LD.

lld/ELF/Arch/SPARCV9.cpp
229

If you just want to make sparcv9 work, you don't need to implement TLS optimizations.
It is additional complexity without many benefits.

lld/ELF/Relocations.cpp
294

Avoid assert if the !call_sym path is reachable. You can call fatal

lld/test/ELF/sparcv9-tls-gd.s
2

Just use #. Avoid !

You can use riscv-tls-gd.s or ppc-tls-gd.s as an example.

For simplicity, you can test just -no-pie or -pie, instead of both.

6

If a shared object is used as an input in a subsequent link, you need --soname=whatever to fix the DT_SONAME, otherwise the DT_NEEDED string will be %t, which is varying on different machines. Grep soname= in this directory.

MaskRay added inline comments.May 20 2021, 9:11 PM
lld/ELF/Arch/SPARCV9.cpp
61

Can you check whether gotHeaderEntriesNum/gotPltHeaderEntriesNum should be changed?

MaskRay added inline comments.May 20 2021, 9:15 PM
lld/ELF/Relocations.cpp
293

tlsGetAddr

Is IE supported now? I think the correct order is LE, IE, GD/LD.

D102908 implements IE & its relaxation, once that's merged I'll rebase this patch.
LD is on the roadmap as I need it to link programs against glibc.
There's also one more GOT relocation that's blocked on D102575, the LLD's side is quite simple.

lld/ELF/Arch/SPARCV9.cpp
61

AFAICS the systemv ABI doesn't reserve any GOT entry (and .got.plt is not used). The only "quirk", 4 reserved PLT entries, is already taken into account.

lld/ELF/Relocations.cpp
294

Both llvm-mc and GAS add a __tls_get_addr symbol, hence the use of assert.
I'll switch to fatal as that's perhaps more robust against malformed input.

lld/test/ELF/sparcv9-tls-gd.s
2

Will do, I used ! as the other SPARC test cases were using it.

6

Does that matter here? I'm not checking any offset in the RELOC checks.

LemonBoy updated this revision to Diff 347242.May 23 2021, 4:06 AM

Rebased on top of D102908.
Correct computation of relaxed tp-relative offset.
Turn add into or/xor during the relaxation.