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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
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:
> 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.  
`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`


================
Comment at: ELF/Relocations.cpp:248
+          In.Got->Relocations.push_back(
+              {R_ADDEND, Target->SymbolicRel, Off, 1, &Sym});
+        else
----------------
peter.smith wrote:
> 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);
> 
> ```
Thanks! I noticed the `R_RISCV_TLS_DTPMOD{32,64}` can be omitted but somehow forget to delete it from ARM.cpp ...


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D63333