This is an archive of the discontinued LLVM Phabricator instance.

[lld] [ELF/AArch64] Add support to some GD/LE/IS TLS relocation
ClosedPublic

Authored by zatrazz on Feb 4 2016, 11:26 AM.

Details

Reviewers
ruiu
rafael
Summary

This patch add some TLS relocation and relaxation to TLS on AArch64.
Some Global-Dynamic relocation are handled by optimizing them to
Local-Exec (Initial-Exec is not yet supported). They are:

  • R_AARCH64_TLSDESC_ADR_PAGE21
  • R_AARCH64_TLSDESC_LD64_LO12_NC
  • R_AARCH64_TLSDESC_ADD_LO12_NC
  • R_AARCH64_TLSDESC_CALL

Also some Init-Exec is optimized to Local-Exec if possible. They are:

  • R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21
  • R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC

Finally some Local-Exec relocation are handled in relocateOne:

  • R_AARCH64_TLSLE_ADD_TPREL_HI12
  • R_AARCH64_TLSLE_ADD_TPREL_LO12_NC

This work is mainly for compiler bootstrap, where static binaries is
showing good progress (although shared object still lacking support
from both TLS aarch64 backend and some other issues).

Diff Detail

Event Timeline

zatrazz updated this revision to Diff 46933.Feb 4 2016, 11:26 AM
zatrazz retitled this revision from to [lld] [ELF/AArch64] Add support to some GD/LE/IS TLS relocation.
zatrazz updated this object.
zatrazz added reviewers: ruiu, rafael.
zatrazz set the repository for this revision to rL LLVM.
zatrazz added subscribers: llvm-commits, rengolin.
ruiu added inline comments.Feb 4 2016, 1:26 PM
ELF/Target.cpp
1311

Can you rename this function to updateAArch64Addr? (That is not directly related to your change, but having Adr and Add are too subtle.)

1421

tpOff -> TPOff

but I think you don't need this variable -- instead you can just

uint64_t V = llvm::alignTo(TcbSize, Out<ELF64LE>::TlsPhdr->p_align) + SA;
1424

Hm, I don't know if updateAArch64Add worth to be defined in the first place. It may be better to inline it because it is short and you used it only twice.

1445–1448

Is this the same as isTlsGlobalDynamicRel? If so, is there any reason to not use the function here?

emaste added a subscriber: emaste.Feb 4 2016, 2:12 PM
zatrazz updated this revision to Diff 47466.Feb 10 2016, 9:20 AM
zatrazz removed rL LLVM as the repository for this revision.

Updated patch with fixes from comments.

ruiu added inline comments.Feb 10 2016, 10:51 AM
ELF/Target.cpp
1365–1366

Why did you change this?

1439

s/to to/to/
s/a an/an/

1462

Please add {}

1465

Insert space after '='.

1501

Please follow the LLVM naming convention.

tpOff -> TPOff

1531

Ditto

1539

s/unsigned int/unsigned/

zatrazz added inline comments.Feb 10 2016, 11:18 AM
ELF/Target.cpp
1311

I will change that.

1365–1366

Because on AArch64 ELF specification states that R_AARCH64_ADR_PREL_PG_HI21 and R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21 should check if the value is within 2^-32 and 2^32. My understanding is currently checking against 2^-33 and 2^33.

1421

This seems better, I will change that.

1424

I though about it as well, but I decided to add to be explicit the relocation is handling a add instruction directly (although the name already says it). Anyway, related to inlining I will add an inline modifier to function definition.

1439

I will change that.

1445–1448

Indeed, I will change that.

1462

Ok.

1465

Ok.

1501

Right, I will change that.

1539

Ok.

ruiu added inline comments.Feb 10 2016, 11:20 AM
ELF/Target.cpp
1365–1366

Please fix the issue in a separate patch.

zatrazz updated this revision to Diff 47496.Feb 10 2016, 11:33 AM

Updated patch with fixes from previous comment.

ruiu added inline comments.Feb 10 2016, 12:31 PM
ELF/Target.cpp
1365–1366

So, please leave "33" as is in this patch.

1462

I didn't mean here, but after if.

case R_AARCH64_TLSDESC_ADD_LO12_NC:
case R_AARCH64_TLSDESC_CALL:
  if (canBePreempted(S, true)) {
    relocateTlsGdToIe(Type, Loc, BufEnd, P, SA);
  } else {
    ...
  }
  return 0;
1464

I'd replace this function call with

error("Unsupported TLS optimization");

and remove relocateTlsGdToIe function. It is better than leaving the stub function.

grimar added a subscriber: grimar.Feb 11 2016, 3:14 AM
grimar added inline comments.
ELF/Target.cpp
1365–1366

Btw, checkInt<> calls

template<unsigned N>
inline bool isInt(int64_t x) {
  return N >= 64 || (-(INT64_C(1)<<(N-1)) <= x && x < (INT64_C(1)<<(N-1)));
}

so that gives us -4294967296 <= x && x < 4294967296 which is valid check [32,32), no ?

zatrazz updated this revision to Diff 47630.Feb 11 2016, 4:52 AM

Updated patch based on previous comment.

zatrazz added inline comments.Feb 11 2016, 10:55 AM
ELF/Target.cpp
1365–1366

You are correct, I will drop this modification.

ruiu accepted this revision.Feb 11 2016, 4:09 PM
ruiu edited edge metadata.

LGTM with a few nits.

ELF/Target.cpp
1434

(V & 0xFFF) -> V & 0xFFF

1470

llvm_unreachable -> fatal

1471

Since fatal is a noreturn function, you don't need this else clause.

if (canBePreempted(S, true))
  fatal(...)
uint64_t X = ...
This revision is now accepted and ready to land.Feb 11 2016, 4:09 PM
zatrazz closed this revision.Feb 12 2016, 5:47 AM