[PATCH] D92959: [ELF][PPC64] Detect missing R_PPC64_TLSGD/R_PPC64_TLSLD and disable TLS relaxation

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few comments/suggestions.



================
Comment at: lld/ELF/Relocations.cpp:1541
 
+  if (config->emachine == EM_PPC64) {
+    // R_PPC64_TLSGD/R_PPC64_TLSLD is required to mark `bl __tls_get_addr` for
----------------
You probably don't need to execute the body when `sec.file->ppc64DisableTLSRelax` is already `true`?
My concern is that scanning over all relocations twice can be slow.


================
Comment at: lld/ELF/Relocations.cpp:1554
+        hasMarker = true;
+        break;
+      case R_PPC64_GOT_TLSGD16:
----------------
I'd probably exit the loop early when we have a "marker".
I guess it might look nicer if you move this code to a helper function like `checkPPC64Relaxations`.

```
if (config->emachine == EM_PPC64)
  checkPPC64Relaxations(...)
```


================
Comment at: lld/ELF/Relocations.cpp:1570
+      warn(toString(&sec) + ": disable TLS relaxation due to missing "
+                            "R_PPC64_TLSGD/R_PPC64_TLSLD");
+    }
----------------



================
Comment at: lld/test/ELF/ppc64-tls-missing-gdld.s:20
+
+# WARN: warning: {{.*}}.o:(.text): disable TLS relaxation due to missing R_PPC64_TLSGD/R_PPC64_TLSLD
+
----------------
It reads like the relaxation was disabled for a particular place.
I.e the message contains the file name and the section name (what is good), but I think the message could mention that the relaxation was disabled for a whole object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92959