This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Refactor per-target TLS layout configuration. NFC.

Authored by rprichard on Oct 30 2018, 3:18 PM.



There are really three different kinds of TLS layouts:

  • A fixed TLS-to-TP offset. On architectures like PowerPC, MIPS, and RISC-V, the thread pointer points to a fixed offset from the start of the executable's TLS segment. The offset is 0x7000 for PowerPC and MIPS, which allows a signed 16-bit offset to reach 0x1000 of per-thread implementation data and 0xf000 of the application's TLS segment. The size and layout of the TCB isn't relevant to the static linker and might not be known.
  • A fixed TCB size. This is the format documented as "variant 1" in Ulrich Drepper's TLS spec. The thread pointer points to a 2-word TCB followed by the executable's TLS segment. The first word is always the DTV pointer. Used on ARM. The thread pointer must be aligned to the TLS segment's alignment, possibly creating alignment padding.
  • Variant 2. This format predates variant 1 and is also documented in Drepper's TLS spec. It allocates the executable's TLS segment before the thread pointer, apparently for backwards-compatibility. It's used on x86 and SPARC.

Factor out an lld::elf::getTlsTpOffset() function for use in a
follow-up patch for Android. The TcbSize/TlsTpOffset fields are only used
in getTlsTpOffset, so replace them with a switch on Config->EMachine.

Diff Detail

rLLD LLVM Linker

Event Timeline

rprichard created this revision.Oct 30 2018, 3:18 PM

This change would make it possible to model RISC-V TLS without the (Config->EMachine == EM_RISCV) special case in (InputSection.cpp). RISC-V expects a fixed offset of 0.

Hmm, I'm not sure I like making TlsTpOffset negative, the name reads as meaning the offset of the thread pointer, which is *positive*, and in general linkers, loaders and libcs like to define it as a positive constant (along with others, such as PPC64TocOffset and DynamicThreadPointerOffset in PPC64.cpp). Personally I'd leave TlsTpOffset as being positive, rename getTlsTpOffset (maybe getTlsOffsetFromTp?) and make it negate TlsTpOffset. Thoughts? Otherwise looks correct to me.

ruiu added inline comments.Oct 30 2018, 4:14 PM

I don't think you need to define TlsLayoutKind. I'd just dispatch based on Config->EMachine in this function.

jrtc27 added inline comments.Oct 30 2018, 4:21 PM

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.

jrtc27 requested changes to this revision.Oct 30 2018, 4:22 PM
This revision now requires changes to proceed.Oct 30 2018, 4:22 PM
ruiu added inline comments.Oct 30 2018, 4:29 PM

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.

rprichard added inline comments.Oct 30 2018, 5:32 PM

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.

rprichard updated this revision to Diff 171855.Oct 30 2018, 6:34 PM

Replace the TargetInfo TLS layout fields with a switch on Config->EMachine.

rprichard retitled this revision from [ELF] Refactor TLS layout TargetInfo config. NFC. to [ELF] Refactor per-target TLS layout configuration. NFC..Oct 30 2018, 6:39 PM
rprichard edited the summary of this revision. (Show Details)
ruiu accepted this revision.Oct 31 2018, 8:02 AM


Nice! The new code is much easier to understand IMO.

jrtc27 accepted this revision.Oct 31 2018, 8:07 AM

Yeah, if we're not using a field for which TLS variant the target wants, this is the clearest way to do it, putting all the logic in one place rather than some in the target and some in InputSection.

This revision is now accepted and ready to land.Oct 31 2018, 8:07 AM
ruiu added inline comments.Oct 31 2018, 8:11 AM

I think you can make this a file-scope static function.

PkmX accepted this revision.Oct 31 2018, 9:09 AM

LGTM. I will update D39324 to reflect this change.

rprichard updated this revision to Diff 172001.Oct 31 2018, 1:20 PM

Make getTlsTpOffset a file-scope static function.

ruiu added a comment.Oct 31 2018, 1:43 PM

Still LGTM, please commit.

This revision was automatically updated to reflect the committed changes.