[PATCH] D31274: Remove an optimization to set the TLS module index to 1.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I haven't got a good answer for why !isPreemptible() is there. My best guess is that it started as isPreemptible(), there was originally no !Config->Pic, when the !Config->Pic got added this made the !isPreemptible() redundant in that case. The isPreemptible() is important in the Target->TlsOffsetRel case as even for a shared library the linker will know the offset of the Symbol in the TLS block if the symbol isn't preemptible.

In summary, for the module index I think !Config-Pic covers all the cases that !isPreemptible() would so it can be removed.

I've done some more investigation into the problem with global dynamic TLS that I found yesterday. I can trace it back to r288107, this changed the GotSection::writeTo() to only use relocations. The previous ARM TLS code was relying on the generic GotSection entry writing code to write the offset from the module index. This means that handleNoRelaxTlsRelocation needs to add a relocation to the Target->TlsOffsetRel as well as the module index. The Mips TLS should be unaffected as it has its own MipsGotSection that will handle this case.

    if (Target->isTlsGlobalDynamicRel(Type)) {
      if (Got->addDynTlsEntry(Body) &&
          (Body.isPreemptible() || Config->EMachine == EM_ARM)) {
        uintX_t Off = Got->getGlobalDynOffset(Body);
        addModuleReloc(Body, Got, Off, false);
        if (Body.isPreemptible())
          In<ELFT>::RelaDyn->addReloc({Target->TlsOffsetRel, Got,
                                       Off + (uintX_t)sizeof(uintX_t), false,
                                       &Body, 0});
  // Next 3 lines are new
        else if (Config->EMachine == EM_ARM)
          Got->Relocations.push_back({R_ABS, Target->TlsOffsetRel,
                Off + (uintX_t)sizeof(uintX_t), 0, &Body });
      }
      C.Relocations.push_back({Expr, Type, Offset, Addend, &Body});
      return 1;
    }

And an addition of R_ARM_TLS_DTPOFF32: to the ARMTargetInfo::relocateOne: (same as R_ARM_TLS_TPOFF32).

I mention this just in case it affects your refactoring. I'll post a patch with test case soon, hopefully next week.


https://reviews.llvm.org/D31274