[PATCH] D53905: [ELF] Refactor TLS layout TargetInfo config. NFC.

[Ryan Prichard via Phabricator via llvm-commits]

rprichard added inline comments.


================
Comment at: ELF/InputSection.cpp:572-573
+  // target-specific adjustment to produce a thread-pointer-relative offset.
+  switch (Target->TlsLayout) {
+    case TlsLayoutKind::FixedOffset:
+      return Target->TlsTpOffset;
----------------
ruiu wrote:
> jrtc27 wrote:
> > ruiu wrote:
> > > I don't think you need to define TlsLayoutKind. I'd just dispatch based on `Config->EMachine` in this function.
> > That works, and there are certainly other parts of LLD that do things like that, but you could say the same thing about `TlsTpOffset` and `TcbSize` themselves... I personally like to see `Config->EMachine` used as little as possible and put the logic in `ELF/Arch/$ARCH.cpp`, but of course as maintainer you ultimately decide where to draw the line for when to abstract, and it's not exactly a big deal (though does then disregard the original motivation for this patch).
> > 
> > Also, on a separate issue, the case labels (and bodies) should be deindented one level to be flush with the `switch`.
> `TlsTpOffset` and `TcbSize` are just member variables that doesn't have any logic, but `TlsLayoutKind` is a new thing with which we compute some value, and we use that member variable only in this function. So I think eliminating `TlsLayoutKind` and directly use `Config->EMachine` matches the taste of lld's code. I do understand your motivation to write target-dependent code in files under `Arch/`, but we are not too serious about doing that. We have target-dependent code in many other places if it makes code easier to read.
> 
> As to the indentation depth, `case` should be at the same nesting level as `switch` in the LLVM coding style.
Using `Config->EMachine` makes sense to me. I'm wondering if we should keep the `TcbSize` / `TlsTpOffset` fields or move them into `getTlsTpOffset`. I'm leaning toward removing the fields so that all the TP-to-TLS-segment logic is in one place.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53905