[libc] Align the stack pointer in the start function.

The loader TLS test for x86_64, which now passes, has been enabled.
A future change should enable the test for aarch64 as well.

Reviewed By: jeffbailey

Differential Revision: https://reviews.llvm.org/D121091

libc/config/linux/CMakeLists.txt

@@ -2,4 +2,6 @@ add_header(
   app_h
   HDR
     app.h
+  DEPENDS
+    libc.src.__support.common
 )

libc/config/linux/app.h

@@ -9,6 +9,8 @@
 #ifndef LLVM_LIBC_CONFIG_LINUX_APP_H
 #define LLVM_LIBC_CONFIG_LINUX_APP_H
 
+#include "src/__support/architectures.h"
+
 #include <stdint.h>
 
 namespace __llvm_libc {
@@ -26,11 +28,39 @@ struct TLS {
   uintptr_t align;
 };
 
+#if defined(LLVM_LIBC_ARCH_X86_64) || defined(LLVM_LIBC_ARCH_AARCH64)
+// At the language level, argc is an int. But we use uint64_t as the x86_64
+// ABI specifies it as an 8 byte value. Likewise, in the ARM64 ABI, arguments
+// are usually passed in registers.  x0 is a doubleword register, so this is
+// 64 bit for aarch64 as well.
+typedef uint64_t ArgcType;
+
+// At the language level, argv is a char** value. However, we use uint64_t as
+// ABIs specify the argv vector be an |argc| long array of 8-byte values.
+typedef uint64_t ArgVEntryType;
+#else
+#error "argc and argv types are not defined for the target platform."
+#endif
+
+struct Args {
+  ArgcType argc;
+
+  // A flexible length array would be more suitable here, but C++ doesn't have
+  // flexible arrays: P1039 proposes to fix this. So, for now we just fake it.
+  // Even if argc is zero, "argv[argc] shall be a null pointer"
+  // (ISO C 5.1.2.2.1) so one is fine. Also, length of 1 is not really wrong as
+  // |argc| is guaranteed to be atleast 1, and there is an 8-byte null entry at
+  // the end of the argv array.
+  ArgVEntryType argv[1];
+};
+
 // Data structure which captures properties of a linux application.
 struct AppProperties {
   // Page size used for the application.
   uintptr_t pageSize;
 
+  Args *args;
+
   // The properties of an application's TLS.
   TLS tls;
 

libc/loader/linux/aarch64/start.cpp

@@ -27,17 +27,6 @@ AppProperties app;
 
 using __llvm_libc::app;
 
-struct Args {
-  // In the ARM64 ABI, arguments are usually passed in registers.  x0 is a
-  // doubleword register, so this is 64 bit.
-  uint64_t argc;
-
-  // C++ Doesn't have flexible arrays: P1039 proposes to fix this, but for now
-  // we just fake it.  Even if argc is zero, "argv[argc] shall be a null
-  // pointer" (ISO C 5.1.2.2.1) so one is fine.
-  uint64_t argv[1];
-};
-
 // TODO: Would be nice to use the aux entry structure from elf.h when available.
 struct AuxEntry {
   uint64_t type;
@@ -45,19 +34,16 @@ struct AuxEntry {
 };
 
 extern "C" void _start() {
-  uintptr_t *frame_ptr =
-      reinterpret_cast<uintptr_t *>(__builtin_frame_address(0));
-
   // Skip the Frame Pointer and the Link Register
   // https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst
   // Section 6.2.3
-
-  Args *args = reinterpret_cast<Args *>(frame_ptr + 2);
+  app.args = reinterpret_cast<__llvm_libc::Args *>(
+      reinterpret_cast<uintptr_t *>(__builtin_frame_address(0)) + 2);
 
   // After the argv array, is a 8-byte long NULL value before the array of env
   // values. The end of the env values is marked by another 8-byte long NULL
   // value. We step over it (the "+ 1" below) to get to the env values.
-  uint64_t *env_ptr = args->argv + args->argc + 1;
+  uint64_t *env_ptr = app.args->argv + app.args->argc + 1;
   uint64_t *env_end_marker = env_ptr;
   while (*env_end_marker)
     ++env_end_marker;
@@ -77,7 +63,7 @@ extern "C" void _start() {
 
   // TODO: Init TLS
 
-  __llvm_libc::syscall(SYS_exit,
-                       main(args->argc, reinterpret_cast<char **>(args->argv),
-                            reinterpret_cast<char **>(env_ptr)));
+  __llvm_libc::syscall(SYS_exit, main(app.args->argc,
+                                      reinterpret_cast<char **>(app.args->argv),
+                                      reinterpret_cast<char **>(env_ptr)));
 }

libc/loader/linux/x86_64/start.cpp

@@ -74,20 +74,6 @@ void initTLS() {
 
 using __llvm_libc::app;
 
-struct Args {
-  // At the language level, argc is an int. But we use uint64_t as the x86_64
-  // ABI specifies it as an 8 byte value.
-  uint64_t argc;
-
-  // At the language level, argv is a char** value. However, we use uint64_t as
-  // the x86_64 ABI specifies the argv vector be an |argc| long array of 8-byte
-  // values. Even though a flexible length array would be more suitable here, we
-  // set the array length to 1 to avoid a compiler warning about it being a C99
-  // extension. Length of 1 is not really wrong as |argc| is guaranteed to be
-  // atleast 1, and there is an 8-byte null entry at the end of the argv array.
-  uint64_t argv[1];
-};
-
 // TODO: Would be nice to use the aux entry structure from elf.h when available.
 struct AuxEntry {
   uint64_t type;
@@ -95,18 +81,30 @@ struct AuxEntry {
 };
 
 extern "C" void _start() {
-  uintptr_t *frame_ptr =
-      reinterpret_cast<uintptr_t *>(__builtin_frame_address(0));
-
   // This TU is compiled with -fno-omit-frame-pointer. Hence, the previous value
   // of the base pointer is pushed on to the stack. So, we step over it (the
   // "+ 1" below) to get to the args.
-  Args *args = reinterpret_cast<Args *>(frame_ptr + 1);
+  app.args = reinterpret_cast<__llvm_libc::Args *>(
+      reinterpret_cast<uintptr_t *>(__builtin_frame_address(0)) + 1);
+
+  // The x86_64 ABI requires that the stack pointer is aligned to a 16-byte
+  // boundary. We align it here but we cannot use any local variables created
+  // before the following alignment. Best would be to not create any local
+  // variables before the alignment. Also, note that we are aligning the stack
+  // downwards as the x86_64 stack grows downwards. This ensures that we don't
+  // tread on argc, argv etc.
+  // NOTE: Compiler attributes for alignment do not help here as the stack
+  // pointer on entry to this _start function is controlled by the OS. In fact,
+  // compilers can generate code assuming the alignment as required by the ABI.
+  // If the stack pointers as setup by the OS are already aligned, then the
+  // following code is a NOP.
+  __asm__ __volatile__("andq $0xfffffffffffffff0, %%rsp\n\t" ::: "%rsp");
+  __asm__ __volatile__("andq $0xfffffffffffffff0, %%rbp\n\t" ::: "%rbp");
 
   // After the argv array, is a 8-byte long NULL value before the array of env
   // values. The end of the env values is marked by another 8-byte long NULL
   // value. We step over it (the "+ 1" below) to get to the env values.
-  uint64_t *env_ptr = args->argv + args->argc + 1;
+  uint64_t *env_ptr = app.args->argv + app.args->argc + 1;
   uint64_t *env_end_marker = env_ptr;
   app.envPtr = env_ptr;
   while (*env_end_marker)
@@ -145,7 +143,7 @@ extern "C" void _start() {
 
   __llvm_libc::initTLS();
 
-  __llvm_libc::syscall(SYS_exit,
-                       main(args->argc, reinterpret_cast<char **>(args->argv),
-                            reinterpret_cast<char **>(env_ptr)));
+  __llvm_libc::syscall(SYS_exit, main(app.args->argc,
+                                      reinterpret_cast<char **>(app.args->argv),
+                                      reinterpret_cast<char **>(env_ptr)));
 }

libc/test/loader/linux/CMakeLists.txt

@@ -57,19 +57,19 @@ add_loader_test(
 #     GERMANY=Berlin
 # )
 
+if(NOT (${LIBC_TARGET_ARCHITECTURE} STREQUAL "x86_64"))
+  return()
+endif()
 
-# TODO: Disableing this test temporarily to investigate why gold fails to link
-# and produce an executable for this test. Test works all fine with ld.bfd.
-#add_loader_test(
-#  loader_tls_test
-#  SRC
-#    tls_test.cpp
-#  DEPENDS
-#    libc.config.linux.app_h
-#    libc.include.errno
-#    libc.include.sys_mman
-#    libc.loader.linux.crt1
-#    libc.src.assert.__assert_fail
-#    libc.src.errno.errno
-#    libc.src.sys.mman.mmap
-#)
+add_loader_test(
+  loader_tls_test
+  SRC
+    tls_test.cpp
+  DEPENDS
+    .loader_test
+    libc.include.errno
+    libc.include.sys_mman
+    libc.loader.linux.crt1
+    libc.src.errno.errno
+    libc.src.sys.mman.mmap
+)

libc/test/loader/linux/tls_test.cpp

@@ -6,12 +6,11 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "loader_test.h"
+
 #include "include/errno.h"
 #include "include/sys/mman.h"
 
-#undef NDEBUG
-#include "src/assert/assert.h"
-
 #include "src/errno/llvmlibc_errno.h"
 #include "src/sys/mman/mmap.h"
 
@@ -19,22 +18,22 @@ constexpr int threadLocalDataSize = 101;
 _Thread_local int a[threadLocalDataSize] = {123};
 
 int main(int argc, char **argv, char **envp) {
-  assert(a[0] == 123);
+  ASSERT_TRUE(a[0] == 123);
 
   for (int i = 1; i < threadLocalDataSize; ++i)
     a[i] = i;
   for (int i = 1; i < threadLocalDataSize; ++i)
-    assert(a[i] == i);
+    ASSERT_TRUE(a[i] == i);
 
   // Call mmap with bad params so that an error value is
   // set in errno. Since errno is implemented using a thread
   // local var, this helps us test setting of errno and
   // reading it back.
-  assert(llvmlibc_errno == 0);
+  ASSERT_TRUE(llvmlibc_errno == 0);
   void *addr = __llvm_libc::mmap(nullptr, 0, PROT_READ,
                                  MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
-  assert(addr == MAP_FAILED);
-  assert(llvmlibc_errno == EINVAL);
+  ASSERT_TRUE(addr == MAP_FAILED);
+  ASSERT_TRUE(llvmlibc_errno == EINVAL);
 
   return 0;
 }