This is an archive of the discontinued LLVM Phabricator instance.

[ELF][ARM] Merge handleARMTlsRelocation() into handleTlsRelocation()
ClosedPublic

Authored by MaskRay on Jun 14 2019, 6:49 AM.

Details

Summary

ARM and RISC-V do not support TLS relaxations. However, for General
Dynamic and Local Dynamic models, if we are producing an executable and
the symbol is non-preemptable, we know it must be defined and the
R_ARM_TLS_DTPMOD32/R_RISCV_TLS_DTPMOD{32,64} dynamic relocation can be
omitted because it is always 1.

Merge handleARMTlsRelocation() into handleTlsRelocation(). This
requires to add logic to R_TLSGD_PC and R_TLSLD_PC. As a bonus, the
additional logic in R_TLSGD_PC code can be shared by the TLS support for
RISC-V (D63220).

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jun 14 2019, 6:49 AM

I've got some stylistic suggestions; otherwise this looks to do the same thing as the previous code.

ELF/Relocations.cpp
180 ↗(On Diff #204760)

I think CanRelax would be more appropriate. To me Relax implies the action of relaxation rather than whether we can take the action.

189 ↗(On Diff #204760)

I think this reduces to !Config->Shared as this implies !Sym.IsPreemptible (see computeIsPreemptible).

189 ↗(On Diff #204760)

I'd prefer not to use IsLocalExec to avoid confusing it with local exec model TLS, which I believe is just TPREL relative relocations that can be resolved statically.

In this context setting the module id to 1 is used in General and Local Dynamic models.

248 ↗(On Diff #204760)

Is it possible to remove the following from Arch/ARM.cpp:

case R_ARM_TLS_DTPMOD32:
  write32le(Loc, 1);
  break;

As this used to be handled in handleARMTlsRelocation with:

AddTlsReloc(In.Got->getTlsIndexOff(), Target->TlsModuleIndexRel,
            NeedDynId ? nullptr : &Sym, NeedDynId);
MaskRay updated this revision to Diff 204776.Jun 14 2019, 8:51 AM

Address peter.smith's comments

MaskRay marked 2 inline comments as done.Jun 14 2019, 8:53 AM
MaskRay added inline comments.
ELF/Relocations.cpp
189 ↗(On Diff #204760)

bool IsLocalExec = !Sym.IsPreemptible && !Config->Shared; can be written as

bool IsLocalExec = Sym.isDefined() && !Config->Shared;

but it can't reduce to:

bool IsLocalExec = !Config->Shared;

The symbol may be a Shared, in that case the symbol isn't considered local.

IsLocalExec does seem confusing. I'll rename it to IsLocalInExecutable

248 ↗(On Diff #204760)

Thanks! I noticed the R_RISCV_TLS_DTPMOD{32,64} can be omitted but somehow forget to delete it from ARM.cpp ...

MaskRay updated this revision to Diff 204787.Jun 14 2019, 9:19 AM

Delete the word relocations from a comment.

MaskRay marked 4 inline comments as done.Jun 17 2019, 11:22 PM

Happy with the changes, thanks for the update.

ruiu accepted this revision.Jun 20 2019, 6:18 AM

LGTM

This revision is now accepted and ready to land.Jun 20 2019, 6:18 AM
This revision was automatically updated to reflect the committed changes.