From b9974db5eb5d60f3337035f6d92906b63b1a0077 Mon Sep 17 00:00:00 2001 From: Jack Gallagher Date: Fri, 12 Jul 2024 14:47:53 +0100 Subject: [PATCH] i#6451 AArch64: Fix large page support (#6868) PR #5835 inadvertently broke support for large pages on AArch64 by introducing code that assumed a 4K page size. The core options unit_tests were also failing on large page systems because a lot of options need to be overridden and the test was not accounting for extra options being set in the options string. Issues: #1680, #6451 Fixes: #6451 --- core/options.c | 12 +++++++++++- core/unix/os.c | 6 ------ core/unix/signal.c | 39 ++++++++++++++++++++++++++++----------- 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/core/options.c b/core/options.c index 168f39782dd..6bf67bb2ce7 100644 --- a/core/options.c +++ b/core/options.c @@ -2785,7 +2785,17 @@ unit_test_options(void) * We include a smaller option to ensure we avoid printing out "0G". */ get_dynamo_options_string(&dynamo_options, opstring, sizeof(opstring), true); - EXPECT_EQ(0, strcmp(opstring, "-vmheap_size 16G -persist_short_digest 8K ")); +# define EXPECT_OPT(opt, val) \ + do { \ + const char *start = strstr(opstring, opt); \ + EXPECT_NE(start, NULL); \ + const char expected[] = opt " " val; \ + EXPECT_STR(start, expected, sizeof(expected) / sizeof(expected[0]) - 1); \ + } while (0) + EXPECT_OPT("-vmheap_size", "16G"); + EXPECT_OPT("-persist_short_digest", "8K"); +# undef EXPECT_OPT + # endif SELF_PROTECT_OPTIONS(); diff --git a/core/unix/os.c b/core/unix/os.c index 2d178fd50c0..e3b35471e8d 100644 --- a/core/unix/os.c +++ b/core/unix/os.c @@ -4061,12 +4061,6 @@ client_thread_run(void) dcontext_t *dcontext; byte *xsp; GET_STACK_PTR(xsp); -# ifdef AARCH64 - /* AArch64's Scalable Vector Extension (SVE) requires more space on the - * stack. Align to page boundary, similar to that in get_clone_record(). - */ - xsp = (app_pc)ALIGN_BACKWARD(xsp, PAGE_SIZE); -# endif void *crec = get_clone_record((reg_t)xsp); /* i#2335: we support setup separate from start, and we want to allow a client * to create a client thread during init, but we do not support that thread diff --git a/core/unix/signal.c b/core/unix/signal.c index 8ca76a01c54..de3079cfa6e 100644 --- a/core/unix/signal.c +++ b/core/unix/signal.c @@ -928,26 +928,43 @@ get_clone_record(reg_t xsp) /* xsp should be in a dstack, i.e., dynamorio heap. */ ASSERT(is_dynamo_address((app_pc)xsp)); - /* The (size of the clone record + + /* The stack usage when this is called on most platforms is less than one page so we + * can straightforwardly find the clone record by forward aligning xsp. + * + * However when this function is called by new_thread_setup() on AArch64 the stack + * usage is > 4K: + * (size of the clone record + * stack used by new_thread_start (only for setting up priv_mcontext_t) + * stack used by new_thread_setup before calling get_clone_record()) - * is less than a page for X86 and 2 pages for AArch64. This is verified by - * the assert below. If it does exceed 1 page for X86 and 2 for AArch64, it - * won't happen at random during runtime, but in a predictable way during - * development, which will be caught by the assert. + * so if we have 4k pages we need to account for the extra page we are using by + * adding PAGE_SIZE to dstack_base. + * + * When this function is called by client_thread_run() there is no priv_mcontext_t on + * the stack so we don't have to make this adjustment. * - * The current usage is about 800 bytes (X86) or 1920 bytes (AArch64) for + * The current usage is about 800 bytes (X86) or 4400 bytes (AArch64) for * clone_record + sizeof(priv_mcontext_t) + few words in new_thread_setup * before get_clone_record() is called. */ + const size_t page_size = PAGE_SIZE; + dstack_base = (byte *)ALIGN_FORWARD(xsp, page_size); + + record = (clone_record_t *)(dstack_base - sizeof(clone_record_t)); #ifdef AARCH64 - dstack_base = (byte *)ALIGN_FORWARD(xsp, PAGE_SIZE) + PAGE_SIZE; -#else - dstack_base = (byte *)ALIGN_FORWARD(xsp, PAGE_SIZE); + if (sizeof(clone_record_t) + sizeof(priv_mcontext_t) > page_size && + dstack_base != record->dstack) { + /* Looks like there is a priv_mcontext_t on the stack so we need to skip forward + * another page. + */ + dstack_base += page_size; + record = (clone_record_t *)(dstack_base - sizeof(clone_record_t)); + } #endif - record = (clone_record_t *)(dstack_base - sizeof(clone_record_t)); - /* dstack_base and the dstack in the clone record should be the same. */ + /* dstack_base and the dstack in the clone record should be the same. + * This verifies the assumptions we have made above about the stack useage fitting + * into 1 or 2 pages. + */ ASSERT(dstack_base == record->dstack); #ifdef MACOS ASSERT(record->app_thread_xsp != 0); /* else it's not in dstack */