[PATCH] D95121: [lld-macho][NFC] refactor relocation handling

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/Target.cpp:38
+
+  if (!relocAttrs.hasRelocAttrBit(RelocAttrBits::LOCAL) && !rel.r_extern)
+    error(message("must be extern"));
----------------
how do you feel about naming this method just `hasAttr`? Having "RelocAttrBit" in both the method and the enum name seems a bit repetitive. Plus the "bit" part is a bit (heh) of an implementation detail.


================
Comment at: lld/MachO/Target.h:50-51
+  GOT = 1 << 11,       // Has a Global Offset Table slot
+  TLV = 1 << 12,       // Has a Thread-Local Variable slot
+  TLS = 1 << 13,       // Can reside in Thread-Local Storage
+  MORPH = 1 << 14,     // morph instruction: load into add
----------------
gkm wrote:
> int3 wrote:
> > I think ld64 (and other bits of macOS source) use "TLV" in place of "TLS". Pointers to TLVs are abbreviated as `TLVP`s.
> That's a real distinction, and apparently needs more commentary to avoid confusion:
> TLV = //Thread-Local Variable// and pertains to the datum and its symbol.
> TLS = //Thread-Local Storage// and pertains to the section that contains the TLVs.
I understood the distinction... I'm saying that `TLVP` seems like a more appropriate name for the former and `TLV` for the latter. In particular, in the `SectionType` enum, we have

```
  /// S_THREAD_LOCAL_VARIABLES - Section with thread local variable
  /// structure data.
  S_THREAD_LOCAL_VARIABLES = 0x13u,
  /// S_THREAD_LOCAL_VARIABLE_POINTERS - Section with pointers to thread
  /// local structures.
  S_THREAD_LOCAL_VARIABLE_POINTERS = 0x14u,
```

Based on the above, it seems fitting to me that a reloc which can be present in a S_THREAD_LOCAL_VARIABLES section has attribute TLV. And similarly TLVP seems like a fitting name for a reloc which can point to an address in a S_THREAD_LOCAL_VARIABLES section.

I'm aware that "TLS" is commonly used in the sense that you are describing, but from what I can tell neither ld64 nor dyld use that abbreviation, so it would be nice to hew to that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95121