From fe2cc14ad43807e7f1f55de64c3e61c5bf034bf2 Mon Sep 17 00:00:00 2001 From: Siva Chandra Reddy Date: Mon, 7 Mar 2022 09:00:14 +0000 Subject: [PATCH] [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 ++ libc/config/linux/app.h | 30 +++++++++++++++++++ libc/loader/linux/aarch64/start.cpp | 26 ++++------------- libc/loader/linux/x86_64/start.cpp | 42 +++++++++++++-------------- libc/test/loader/linux/CMakeLists.txt | 30 +++++++++---------- libc/test/loader/linux/tls_test.cpp | 15 +++++----- 6 files changed, 80 insertions(+), 65 deletions(-) diff --git a/libc/config/linux/CMakeLists.txt b/libc/config/linux/CMakeLists.txt index bc42557f3db9..82f7a01076e3 100644 --- a/libc/config/linux/CMakeLists.txt +++ b/libc/config/linux/CMakeLists.txt @@ -2,4 +2,6 @@ add_header( app_h HDR app.h + DEPENDS + libc.src.__support.common ) diff --git a/libc/config/linux/app.h b/libc/config/linux/app.h index 35913998d596..024aed0dcdd7 100644 --- a/libc/config/linux/app.h +++ b/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 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; diff --git a/libc/loader/linux/aarch64/start.cpp b/libc/loader/linux/aarch64/start.cpp index 5b853a4e197c..ecff48f3235d 100644 --- a/libc/loader/linux/aarch64/start.cpp +++ b/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(__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(frame_ptr + 2); + app.args = reinterpret_cast<__llvm_libc::Args *>( + reinterpret_cast(__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(args->argv), - reinterpret_cast(env_ptr))); + __llvm_libc::syscall(SYS_exit, main(app.args->argc, + reinterpret_cast(app.args->argv), + reinterpret_cast(env_ptr))); } diff --git a/libc/loader/linux/x86_64/start.cpp b/libc/loader/linux/x86_64/start.cpp index 8a47a2713cfc..d97c9bc9ae30 100644 --- a/libc/loader/linux/x86_64/start.cpp +++ b/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(__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(frame_ptr + 1); + app.args = reinterpret_cast<__llvm_libc::Args *>( + reinterpret_cast(__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(args->argv), - reinterpret_cast(env_ptr))); + __llvm_libc::syscall(SYS_exit, main(app.args->argc, + reinterpret_cast(app.args->argv), + reinterpret_cast(env_ptr))); } diff --git a/libc/test/loader/linux/CMakeLists.txt b/libc/test/loader/linux/CMakeLists.txt index 36f4ddb1d962..e9db6451aa57 100644 --- a/libc/test/loader/linux/CMakeLists.txt +++ b/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 +) diff --git a/libc/test/loader/linux/tls_test.cpp b/libc/test/loader/linux/tls_test.cpp index 144d894a2caf..7d71db4a5baf 100644 --- a/libc/test/loader/linux/tls_test.cpp +++ b/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; }