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

[Fangrui Song via Phabricator via libc-commits]

MaskRay added inline comments.


================
Comment at: libc/loader/linux/x86_64/start.cpp:38
+
+// TODO: The function is x86_64 specific. Move it to config/linux/app.h
+// and generalize it.
----------------
This should be implemented directly in a generic place, instead of adding a TODO and moving all content immediately when implementing aarch64.


================
Comment at: libc/loader/linux/x86_64/start.cpp:46
+  uintptr_t misAlign = tlsSize % app.tls.align;
+  tlsSize += (misAlign ? app.tls.align - misAlign : 0);
+
----------------
This formula can be written as: `tlsSize = (app.tls.size + app.tls.align) & -app.tls.align`
However, there is a larger issue. See below.


================
Comment at: libc/loader/linux/x86_64/start.cpp:51
+  // entry.
+  size_t tlsSizeWithAddr = tlsSize + sizeof(uintptr_t);
+
----------------
The size should be 2 words plus sizeof(pthread) plus p_memsz(PT_TLS) plus (headroom left for padding).

Initially dtv[0] = 1 (one module), dtv[1] = DTP offset. The pthread structure is placed at the end of dtv.


================
Comment at: libc/loader/linux/x86_64/start.cpp:60
+  if (mmapRetVal < 0 && static_cast<uintptr_t>(mmapRetVal) > -app.pageSize)
+    __llvm_libc::syscall(SYS_exit, 1);
+  uintptr_t *tlsAddr = reinterpret_cast<uintptr_t *>(mmapRetVal);
----------------
This can call an internal crash function instead of SYS_exit.

mmapRetVal < 0 is sufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82700