[PATCH] D57749: [LLD][ELF] - Set DF_STATIC_TLS flag for i386 target.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: ELF/Arch/X86.cpp:80
+      Type == R_386_TLS_GOTIE)
+    Config->HasStaticTlsModel = true;
+
----------------
ruiu wrote:
> I don't remember if this function is called from a multi-threaded part of lld. If that's the case, mutating a variable is not considered thread-safe, even if you are just mutating a boolean variable and set it to the same value from everywhere.
Yes, you're right. We call it for `relocateNonAlloc`, it should be multithreaded.

Actually, I do not really like to have such code here.

I am using it for the following place now:
https://github.com/llvm-mirror/lld/blob/master/ELF/Relocations.cpp#L997

And I thought about just doing a static helper right above `scanReloc` like 

```
static bool isStaticTlsReloc(RelType Type) {
  switch (Target) {
   case TargetA:
   return Type == Reloc1 ...;
  ...
   }
}

```
Cons would be that we will have a switch for targets and target specific relocations there,
though we already have a helper for MIPS in that file:
https://github.com/llvm-mirror/lld/blob/master/ELF/Relocations.cpp#L298

An alternative can be adding a `Target::isStaticTlsReloc(RelType Type)` method,
what is probably better, though I am not sure it is not too much for this little feature.

What would you prefer?


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

https://reviews.llvm.org/D57749