Page MenuHomePhabricator

[LLD][ELF] Add --force-tls-variant2 option
AbandonedPublic

Authored by andrewng on Oct 20 2020, 6:29 AM.

Details

Summary

The --force-tls-variant2 option can be used to force TLS variant 2 for
targets that normally use a form of TLS variant 1.

Diff Detail

Event Timeline

andrewng created this revision.Oct 20 2020, 6:29 AM
andrewng requested review of this revision.Oct 20 2020, 6:29 AM
MaskRay requested changes to this revision.Oct 20 2020, 11:24 AM

I have checked with a few folks. ARM has never used TLS variant 2 (nor does GNU ld support such an option).
Every target doing TLS variant 1/2 has its unique special case. The generic --force-tls-variant2 does not seem to make lots of sense.
For example, on AArch64, this requires a reinterpretation of R_AARCH64_TLSLE_ADD_TPREL_LO12 and other relocation types' ranges.

Such non-standard TLS deviation happened once for a more widely project, Android bionic. Despite its popularity, it hindered regular AArch64 and thus I have tried very hard to help them migrate from the LLD hack. I have made extensive TLS improvement in LLD (with help from Ryan Prichard on Android side), added .reloc directives to the integrated assembler, and eventually removed the hack in D62055, and let Android resolve the issue on their ld.so. For yours, I think you can fix them on the ld.so's side.

This revision now requires changes to proceed.Oct 20 2020, 11:24 AM
edd added a comment.Oct 21 2020, 7:45 AM

I work with Andrew.

Though the test case exercises the change with ARM code, we can neither confirm nor deny that we're interested specifically in this target.

We're also not in a position to change the dynamic linker/loader. I can't say why that is.

We have to be this vague for business reasons, sorry :( It might mean that the reviewers here don't have enough information for acceptance. If that's the case, so be it; we'll have to keep our local hacks. Just let us know if that's the case and we can close this out.

I really appreciate the other areas Andrew has contributed to.G
For this one, it is a non-ABI-conforming variant (whose purpose will make people confused (as I've pointed out)) which will work with no existing ld.so implementations people can find, implemented by neither of the linker we want to be a replacement (GNU ld,gold).
I have asked 3 other folks who have experience in linker and loaders. They are all skeptical about the change.
What makes the case worse is that you can neither confirm nor deny the detail.
(I did want to help. And in a very similar case for Android bionic, I've helped them migrate off an LLD hack.)
This really makes the option a private vendor extension that nobody else can reasonably contribute on this matter.
They cannot make judgement fron the existing material as it is tightly coupled with your private vendor implementation.
Putting together, I think this option cannot be accepted.
On the good side, the place you modify is very stable and you usually will not get merge conflict.

Putting together, I think this option cannot be accepted.
On the good side, the place you modify is very stable and you usually will not get merge conflict.

OK, no problem and thanks for taking the time to review. To be honest, I had similar concerns but we're in a tricky situation as Edd has explained. Would you consider taking this patch without the --force-tls-variant2 option, i.e. just the addition of the TlsVariant enum and associated changes?

Putting together, I think this option cannot be accepted.
On the good side, the place you modify is very stable and you usually will not get merge conflict.

OK, no problem and thanks for taking the time to review. To be honest, I had similar concerns but we're in a tricky situation as Edd has explained.

Would you consider taking this patch without the --force-tls-variant2 option, i.e. just the addition of the TlsVariant enum and associated changes?

Sorry, I am afraid I won't. Building an abstraction and duplicating a switch in Driver.cpp just seem to make the code harder to read. This piece of code is quite stable and it should not cause you much (if any) rebase trouble.

andrewng abandoned this revision.Oct 22 2020, 10:23 AM