[libc-commits] [PATCH] D82700: [libc] Setup TLS in x86_64 loader.

[Anthony Steinhauser via Phabricator via libc-commits]

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


================
Comment at: libc/config/linux/app.h:16
+
+struct TLS {
+  uintptr_t address;
----------------
It would be good to add source-code comments about the semantics of those two structs and their fields.


================
Comment at: libc/loader/linux/x86_64/start.cpp:25
+
+#ifdef SYS_mmaps2
+static constexpr long mmapSyscallNumber = SYS_mmap2;
----------------
There should probably be SYS_mmap2 instead of SYS_mmaps2.


================
Comment at: libc/loader/linux/x86_64/start.cpp:46
+  uintptr_t misAlign = tlsSize % app.tls.align;
+  tlsSize += (misAlign ? app.tls.align - misAlign : 0);
+
----------------
MaskRay wrote:
> This formula can be written as: `tlsSize = (app.tls.size + app.tls.align) & -app.tls.align`
> However, there is a larger issue. See below.
The existing comment about tlsSize = (app.tls.size + app.tls.align) & -app.tls.align seems relevant to me. It assumes that app.tls.align is a power of two which is a valid assumption, but might be mentioned in libc/config/linux/app.h comments.


================
Comment at: libc/loader/linux/x86_64/start.cpp:59
+      MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+  if (mmapRetVal < 0 && static_cast<uintptr_t>(mmapRetVal) > -app.pageSize)
+    __llvm_libc::syscall(SYS_exit, 1);
----------------
Why "if (mmapRetVal == MAP_FAILED)" is not a sufficient check?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82700