Implements @tlsld (LD to LE) and @tlsgd (GD to LE) optimizations.
Patch does not implement the GD->IE case for @tlsgd, I will implement it in separate patch.
This one partially intersects with http://reviews.llvm.org/D14713.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| ELF/InputSection.cpp | ||
|---|---|---|
| 103 | It is not obvious that Rels.begin() is a randomly-accessible iterator in this context, so this is a bit alarming. Probably it is better to write size_t Idx = -1;
for (const RelType &RI : Rels) {
++Idx;
...
} | |
| 114 | Instead of adding this condition, can you move this if (Body.isTLS() && Target->isTlsOptimized(Type, &Body)) {
....
}block above this line? | |
| ELF/Target.cpp | ||
| 383 | return (Type == R_X86_64_TLSLD) || (Type == R_X86_64_DTPOFF32) || -> return Type == R_X86_64_TLSLD || Type == R_X86_64_DTPOFF32 || (We can assume that everybody knows == take higher precedence over || at least.) | |
| 394–395 | What are these instructions? | |
| 402 | Do not put a space before '!'. | |
| 427–428 | Feels like these error checks are becoming more annoying than useful. I remember you mentioned to remove them. I agree with that. If these relocations are guaranteed to be used with only specific set of instructions, we don't always need to check for that condition (if users throw in garbage, it may output garbage). If you remove this error check, you can remove BufStart from argument list? | |
| ELF/InputSection.cpp | ||
|---|---|---|
| 103 | I need that to be able to skip relocations, not for counting ID. Please see below next parts of code: // The next relocation should be against __tls_get_addr, so skip it. ++RelNdx; | |
| 114 | Thats probably will not work. SymbolBody &Body = *File->getSymbolBody(SymIndex)->repl(); To do that we need to check/handle locals first that dont have it. That is done below: if (SymIndex < SymTab->sh_info) {
...
}What about if I remake isTlsLocalDynamicReloc() to isNoOptTlsLocalDynamicReloc() (NotOptimized + TlsLocalDynamicReloc) ? That should work everywhere in code. | |
| ELF/Target.cpp | ||
| 383 | Ok | |
| 394–395 | Its mentioned on p.23 of http://www.akkadia.org/drepper/tls.pdf. That is just prefixes that used to fill the whole sequence. For some reason not no-ops operations should be used. | |
| 402 | Ok | |
| 427–428 | Yes, BufStart is not needed then. Lets do that. | |
| ELF/InputSection.cpp | ||
|---|---|---|
| 103 | gold implements that via having and setting bool flag skip_call_tls_get_addr_ that skips the relocation if true. bfd uses the approach similiar to what I used. With using flag that could be done more strict I guess (we can check relocation type and symbol name). But I think there is no chance to avoid that at all. That relocation exist but should not be proccessed. | |
| ELF/InputSection.cpp | ||
|---|---|---|
| 103 | It is okay to update the index in side the loop. What I meant by too subtle is that it is not obvious that "I" may be updated by relocateTlsOptimize. It is passed by a reference that makes it almost impossible to imagine that case. How about this. Change the signature of relocateTlsOptimize so that it returns an integer. If the next entry needs to be skipped, it returns 1. Otherwise 0. You can add that return value to I. | |
| ELF/InputSection.cpp | ||
|---|---|---|
| 142 | Please add a comment here. // By optimizing TLS relocations, it is sometimes needed to skip relocations // that immediately follow TLS relocations. This function knows how many slots // we need to skip. | |
| ELF/Target.cpp | ||
| 469 | Please do not use int8_t just because it returns a small integer. If no specific size is in mind, use int. | |
| 469 | Add a comment // This function applies a TLS relocation with an optimization as described // in the Ulrich's document. As a result of rewriting instructions at the relocation // target, relocations immediately follow the TLS relocation (which would be applied // to rewritten instructions) need to be skipped. // This function returns a number of relocations that need to be skipped. | |
| 472–483 | switch (Type) {
case R_X86_64_GOTTPOFF:
relocateTlsIeToLe(...);
return 0;
case R_X86_64_TLSLD:
relocateTlsLdToLe(...);
return 1;
case ...
} | |
| ELF/Target.cpp | ||
|---|---|---|
| 472 | Done. | |
It is not obvious that Rels.begin() is a randomly-accessible iterator in this context, so this is a bit alarming. Probably it is better to write
size_t Idx = -1; for (const RelType &RI : Rels) { ++Idx; ... }