[libc-commits] [libc] fe2cc14 - [libc] Align the stack pointer in the start function.
Siva Chandra Reddy via libc-commits
libc-commits at lists.llvm.org
Mon Mar 7 11:46:00 PST 2022
Author: Siva Chandra Reddy
Date: 2022-03-07T19:45:53Z
New Revision: fe2cc14ad43807e7f1f55de64c3e61c5bf034bf2
URL: https://github.com/llvm/llvm-project/commit/fe2cc14ad43807e7f1f55de64c3e61c5bf034bf2
DIFF: https://github.com/llvm/llvm-project/commit/fe2cc14ad43807e7f1f55de64c3e61c5bf034bf2.diff
LOG: [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
Added:
Modified:
libc/config/linux/CMakeLists.txt
libc/config/linux/app.h
libc/loader/linux/aarch64/start.cpp
libc/loader/linux/x86_64/start.cpp
libc/test/loader/linux/CMakeLists.txt
libc/test/loader/linux/tls_test.cpp
Removed:
################################################################################
diff --git a/libc/config/linux/CMakeLists.txt b/libc/config/linux/CMakeLists.txt
index bc42557f3db95..82f7a01076e3d 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 35913998d5960..024aed0dcdd71 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 <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;
diff --git a/libc/loader/linux/aarch64/start.cpp b/libc/loader/linux/aarch64/start.cpp
index 5b853a4e197c5..ecff48f3235de 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<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)));
}
diff --git a/libc/loader/linux/x86_64/start.cpp b/libc/loader/linux/x86_64/start.cpp
index 8a47a2713cfca..d97c9bc9ae30a 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<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)));
}
diff --git a/libc/test/loader/linux/CMakeLists.txt b/libc/test/loader/linux/CMakeLists.txt
index 36f4ddb1d9627..e9db6451aa57d 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 144d894a2caf0..7d71db4a5baff 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;
}
More information about the libc-commits
mailing list