From 320885b4b609947f3ffc808cc381e54a047502ef Mon Sep 17 00:00:00 2001 From: Edmund Grimley Evans Date: Tue, 17 Dec 2024 12:02:37 +0000 Subject: [PATCH 1/4] i#3699 ARM: Keep stack aligned in insert_{push,pop}_all_registers. dd1589c0c modified dstack_offs in insert_push_all_registers without generating an instruction that modifies SP. So add a SUB SP, SP, #4 there, and a corresponding ADD SP, SP, #4 in insert_pop_all_registers. This made the test client.cleancallparams pass. Issue: #3699 Change-Id: I25d2a22e7feb7bc1227aeec74847c80754eae254 --- core/arch/aarchxx/mangle.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/arch/aarchxx/mangle.c b/core/arch/aarchxx/mangle.c index 49d9c05d0de..b92cf677ebd 100644 --- a/core/arch/aarchxx/mangle.c +++ b/core/arch/aarchxx/mangle.c @@ -665,6 +665,9 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, dstack_offs += 15 * XSP_SZ; /* Make dstack_offs 8-byte algined, as we only accounted for 17 4-byte slots. */ + PRE(ilist, instr, + XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), + OPND_CREATE_INT(XSP_SZ))); dstack_offs += XSP_SZ; #endif ASSERT(cci->skip_save_flags || cci->num_simd_skip != 0 || cci->num_regs_skip != 0 || @@ -769,6 +772,9 @@ insert_pop_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, instrlist } #else + PRE(ilist, instr, + XINST_CREATE_add(dcontext, opnd_create_reg(DR_REG_SP), + OPND_CREATE_INT(XSP_SZ))); /* We rely on dr_set_mcontext_priv() to set the app's stolen reg value, * and the stack swap to set the sp value: we assume the stolen reg on * the stack still has our TLS base in it. From 6e851f36a2eeedd4f4a9efa6d50282786f45d1c2 Mon Sep 17 00:00:00 2001 From: Edmund Grimley Evans Date: Tue, 17 Dec 2024 12:09:41 +0000 Subject: [PATCH 2/4] fixup: clang-format Change-Id: Ieeea1c87e51b07021b7e1dd7cc306139ffacbbe7 --- core/arch/aarchxx/mangle.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/arch/aarchxx/mangle.c b/core/arch/aarchxx/mangle.c index b92cf677ebd..f3a8b27b121 100644 --- a/core/arch/aarchxx/mangle.c +++ b/core/arch/aarchxx/mangle.c @@ -666,8 +666,7 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, /* Make dstack_offs 8-byte algined, as we only accounted for 17 4-byte slots. */ PRE(ilist, instr, - XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), - OPND_CREATE_INT(XSP_SZ))); + XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ))); dstack_offs += XSP_SZ; #endif ASSERT(cci->skip_save_flags || cci->num_simd_skip != 0 || cci->num_regs_skip != 0 || @@ -773,8 +772,7 @@ insert_pop_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, instrlist #else PRE(ilist, instr, - XINST_CREATE_add(dcontext, opnd_create_reg(DR_REG_SP), - OPND_CREATE_INT(XSP_SZ))); + XINST_CREATE_add(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ))); /* We rely on dr_set_mcontext_priv() to set the app's stolen reg value, * and the stack swap to set the sp value: we assume the stolen reg on * the stack still has our TLS base in it. From 373761c0a4434bd60d800af0a37f9bdd39f72ec3 Mon Sep 17 00:00:00 2001 From: Edmund Grimley Evans Date: Wed, 18 Dec 2024 20:27:24 +0000 Subject: [PATCH 3/4] Add comments and ASSERTs. Change-Id: Ia5c9b6731374f1e9d655af769c2fb62a56a3f8e2 --- core/arch/aarchxx/mangle.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/core/arch/aarchxx/mangle.c b/core/arch/aarchxx/mangle.c index f3a8b27b121..68d845a3905 100644 --- a/core/arch/aarchxx/mangle.c +++ b/core/arch/aarchxx/mangle.c @@ -596,18 +596,18 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, PRE(ilist, instr, INSTR_CREATE_vstmdb(dcontext, OPND_CREATE_MEMLIST(DR_REG_SP), SIMD_REG_LIST_LEN, SIMD_REG_LIST_0_15)); - dstack_offs += proc_num_simd_registers() * sizeof(dr_simd_t); ASSERT(proc_num_simd_registers() == MCXT_NUM_SIMD_SLOTS); + ASSERT(ALIGNED(dstack_offs, get_ABI_stack_alignment())); /* pc and aflags */ if (cci->skip_save_flags) { /* even if we skip flag saves we want to keep mcontext shape */ int offs_beyond_xmm = 2 * XSP_SZ; - dstack_offs += offs_beyond_xmm; PRE(ilist, instr, XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(offs_beyond_xmm))); + dstack_offs += offs_beyond_xmm; } else { uint slot = TLS_REG0_SLOT; bool spill = scratch == REG_NULL; @@ -633,14 +633,16 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, PRE(ilist, instr, XINST_CREATE_load_int(dcontext, opnd_create_reg(scratch), push_pc)); PRE(ilist, instr, INSTR_CREATE_push(dcontext, opnd_create_reg(scratch))); + dstack_offs += XSP_SZ; } else { ASSERT(opnd_is_reg(push_pc)); PRE(ilist, instr, INSTR_CREATE_push(dcontext, push_pc)); + dstack_offs += XSP_SZ; } if (spill) PRE(ilist, instr, instr_create_restore_from_tls(dcontext, scratch, slot)); - dstack_offs += XSP_SZ; } + ASSERT(ALIGNED(dstack_offs, get_ABI_stack_alignment())); /* We rely on dr_get_mcontext_priv() to fill in the app's stolen reg value * and sp value. @@ -648,26 +650,30 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, if (dr_get_isa_mode(dcontext) == DR_ISA_ARM_THUMB) { /* We can't use sp with stm */ PRE(ilist, instr, INSTR_CREATE_push(dcontext, opnd_create_reg(DR_REG_LR))); + dstack_offs += XSP_SZ; /* We can't push sp w/ writeback, and in fact dr_get_mcontext() gets * sp from the stack swap so we can leave this empty. */ PRE(ilist, instr, XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ))); + dstack_offs += XSP_SZ; PRE(ilist, instr, INSTR_CREATE_stmdb_wb(dcontext, OPND_CREATE_MEMLIST(DR_REG_SP), DR_REG_LIST_LENGTH_T32, DR_REG_LIST_T32)); + dstack_offs += DR_REG_LIST_LENGTH_T32 * XSP_SZ; } else { PRE(ilist, instr, INSTR_CREATE_stmdb_wb(dcontext, OPND_CREATE_MEMLIST(DR_REG_SP), DR_REG_LIST_LENGTH_ARM, DR_REG_LIST_ARM)); + dstack_offs += DR_REG_LIST_LENGTH_ARM * XSP_SZ; } - dstack_offs += 15 * XSP_SZ; - /* Make dstack_offs 8-byte algined, as we only accounted for 17 4-byte slots. */ + /* Make dstack_offs 8-byte aligned, as we only accounted for 17 4-byte slots. */ PRE(ilist, instr, XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ))); dstack_offs += XSP_SZ; + ASSERT(ALIGNED(dstack_offs, get_ABI_stack_alignment())); #endif ASSERT(cci->skip_save_flags || cci->num_simd_skip != 0 || cci->num_regs_skip != 0 || dstack_offs == (uint)get_clean_call_switch_stack_size()); @@ -771,6 +777,7 @@ insert_pop_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, instrlist } #else + /* This undoes the XINST_CREATE_sub done for alignment in XINST_CREATE_sub. */ PRE(ilist, instr, XINST_CREATE_add(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ))); /* We rely on dr_set_mcontext_priv() to set the app's stolen reg value, From dab1876fd25e5e262b164dd6a3c2cf59bf536214 Mon Sep 17 00:00:00 2001 From: Edmund Grimley Evans Date: Wed, 18 Dec 2024 21:18:14 +0000 Subject: [PATCH 4/4] Improve comment. Change-Id: I4dd426098047a851bb9e81c7dedb62afc231e956 --- core/arch/aarchxx/mangle.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/arch/aarchxx/mangle.c b/core/arch/aarchxx/mangle.c index 68d845a3905..0bd8b70a0ee 100644 --- a/core/arch/aarchxx/mangle.c +++ b/core/arch/aarchxx/mangle.c @@ -668,12 +668,14 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, DR_REG_LIST_LENGTH_ARM, DR_REG_LIST_ARM)); dstack_offs += DR_REG_LIST_LENGTH_ARM * XSP_SZ; } - - /* Make dstack_offs 8-byte aligned, as we only accounted for 17 4-byte slots. */ + /* Make dstack_offs 8-byte aligned as we have just pushed an odd + * number of 4-byte registers. + */ PRE(ilist, instr, XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ))); dstack_offs += XSP_SZ; ASSERT(ALIGNED(dstack_offs, get_ABI_stack_alignment())); + #endif ASSERT(cci->skip_save_flags || cci->num_simd_skip != 0 || cci->num_regs_skip != 0 || dstack_offs == (uint)get_clean_call_switch_stack_size());