[PATCH] D63333: [ELF][ARM] Merge handleARMTlsRelocation() into handleTlsRelocation()

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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



================
Comment at: ELF/Relocations.cpp:180
 
+  bool Relax = Config->EMachine != EM_ARM;
+
----------------
I think CanRelax would be more appropriate. To me Relax implies the action of relaxation rather than whether we can take the action. 


================
Comment at: ELF/Relocations.cpp:189
+  // will be no dynamic loader to resolve them at load-time.
+  bool IsLocalExec = !Sym.IsPreemptible && !Config->Shared;
+
----------------
I think this reduces to !Config->Shared as this implies !Sym.IsPreemptible (see computeIsPreemptible).


================
Comment at: ELF/Relocations.cpp:189
+  // will be no dynamic loader to resolve them at load-time.
+  bool IsLocalExec = !Sym.IsPreemptible && !Config->Shared;
+
----------------
peter.smith wrote:
> I think this reduces to !Config->Shared as this implies !Sym.IsPreemptible (see computeIsPreemptible).
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.  


================
Comment at: ELF/Relocations.cpp:248
+          In.Got->Relocations.push_back(
+              {R_ADDEND, Target->SymbolicRel, Off, 1, &Sym});
+        else
----------------
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);

```


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63333/new/

https://reviews.llvm.org/D63333