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

[sivachandra]

Summary

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

[jeffbailey]

Nice find - A couple comments within.

libc/loader/linux/x86_64/start.cpp

96

Would attribute((aligned)) not work here? If not, it would be nice to say why

libc/test/loader/linux/CMakeLists.txt

60

It might be nice to have this be a constexpr of tls_enabled or similar for arch bringup.

[sivachandra]

Add a comment explaining why a compiler attribute to align
the stack pointer will not help.

[sivachandra]

libc/loader/linux/x86_64/start.cpp

96

Added comment now explaining why an attribute will not help.

libc/test/loader/linux/CMakeLists.txt

60

Can you explain how you envision this constexpr to be used? Is it CMake variable or a variable in the source code?

[jeffbailey]

libc/test/loader/linux/CMakeLists.txt

60

Sorry - I was clearly not awake this morning and through this was in the C++ code.

libc/config/linux/CMakeLists.txt

 add_header(
   app_h
   HDR
     app.h
+  DEPENDS
+    libc.src.__support.common
 )

libc/config/linux/app.h

 //===-- Classes to capture properites of linux applications -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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 {
 
 // Data structure to capture properties of the linux/ELF TLS.
 struct TLS {
   // The load address of the TLS.
   uintptr_t address;
 
   // The bytes size of the TLS.
   uintptr_t size;
 
   // The alignment of the TLS layout. It assumed that the alignment
   // value is a power of 2.
   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;
 
   // Environment data.
   uint64_t *envPtr;
 };
 
 extern AppProperties app;
 
 // Creates and initializes the TLS area for the current thread. Should not
 // be called before app.tls has been initialized.
 void initTLS();
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_CONFIG_LINUX_APP_H

libc/loader/linux/aarch64/start.cpp

 namespace __llvm_libc {
 
 AppProperties app;
 
 } // namespace __llvm_libc
 
 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;
   uint64_t value;
 };
 
 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;
 
   // After the env array, is the aux-vector. The end of the aux-vector is
   // denoted by an AT_NULL entry.
   for (AuxEntry *aux_entry = reinterpret_cast<AuxEntry *>(env_end_marker + 1);
        aux_entry->type != AT_NULL; ++aux_entry) {
     switch (aux_entry->type) {
     case AT_PAGESZ:
       app.pageSize = aux_entry->value;
       break;
     default:
       break; // TODO: Read other useful entries from the aux vector.
     }
   }
 
   // TODO: Init TLS
 
-  __llvm_libc::syscall(SYS_exit,
-                       main(args->argc, reinterpret_cast<char **>(args->argv),
+  __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

void initTLS() {
   if (__llvm_libc::syscall(SYS_arch_prctl, ARCH_SET_FS, endPtr) == -1)
     __llvm_libc::syscall(SYS_exit, 1);
 }
 
 } // namespace __llvm_libc
 
 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;
   uint64_t value;
 };
 
 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)
     ++env_end_marker;
 
   // After the env array, is the aux-vector. The end of the aux-vector is
   // denoted by an AT_NULL entry.
   Elf64_Phdr *programHdrTable = nullptr;
for (uintptr_t i = 0; i < programHdrCount; ++i) {
     // TODO: p_vaddr value has to be adjusted for static-pie executables.
     app.tls.address = phdr->p_vaddr;
     app.tls.size = phdr->p_memsz;
     app.tls.align = phdr->p_align;
   }
 
   __llvm_libc::initTLS();
 
-  __llvm_libc::syscall(SYS_exit,
-                       main(args->argc, reinterpret_cast<char **>(args->argv),
+  __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

 #     .loader_test
 #     libc.loader.linux.crt1
 #     libc.src.stdlib.getenv
 #   ENV
 #     FRANCE=Paris
 #     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

 //===-- Loader test to check if tls size is read correctly ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#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"
 
 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;
 }