Skip to content

Commit

Permalink
i#3699 ARM: Add padding to priv_mcontext_t for 8-byte alignment.
Browse files Browse the repository at this point in the history 
ef4482e caused a few additional failures. In particular, the
child of a clone3 system call started with the register values
shifted: R2 got the value that should be in R1 and so on.
That caused the test linux.clone to fail.

It turns out that some callers of insert_{push,pop}_all_registers
expect that an unpadded priv_mcontext_t will be pushed/popped.
Moving the 4-byte padding to above the struct instead of below it
makes some tests pass but breaks others. So we add some padding
to priv_mcontext_t itself, changing its size from 324 to 328. It
will probably run slightly faster with the struct 8-byte aligned.

Update insert_{push,pop}_all_registers and arm.asm for the new
struct layout, with some additional changes to arm.asm:

+ Delete irrelevant X64 macros.

+ Document the macros more clearly (they can be found with grep).

+ Change SP only at start and end of function (standard practice,
makes debugging easier).

+ Avoid writing below SP (compatible with signal handlers).

Issue: #3699

Change-Id: I4977c5bf42e349d1085dbdecf3428e83a4a2399c
  • Loading branch information
@egrimley-arm
egrimley-arm committed Jan 16, 2025
1 parent 359868c commit df170af
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 63 deletions.
20 changes: 10 additions & 10 deletions core/arch/aarchxx/mangle.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,11 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci,
ASSERT(proc_num_simd_registers() == MCXT_NUM_SIMD_SLOTS);
ASSERT(ALIGNED(dstack_offs, get_ABI_stack_alignment()));

/* padding */
PRE(ilist, instr,
XINST_CREATE_sub(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ)));
dstack_offs += XSP_SZ;

/* pc and aflags */
if (cci->skip_save_flags) {
/* even if we skip flag saves we want to keep mcontext shape */
Expand Down Expand Up @@ -642,7 +647,6 @@ insert_push_all_registers(dcontext_t *dcontext, clean_call_info_t *cci,
if (spill)
PRE(ilist, instr, instr_create_restore_from_tls(dcontext, scratch, slot));
}
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.
Expand All @@ -668,12 +672,6 @@ 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 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
Expand Down Expand Up @@ -779,9 +777,6 @@ 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,
* and the stack swap to set the sp value: we assume the stolen reg on
* the stack still has our TLS base in it.
Expand Down Expand Up @@ -816,6 +811,11 @@ insert_pop_all_registers(dcontext_t *dcontext, clean_call_info_t *cci, instrlist
OPND_CREATE_INT_MSR_NZCVQG(), opnd_create_reg(scratch)));
PRE(ilist, instr, instr_create_restore_from_tls(dcontext, scratch, slot));
}

/* padding */
PRE(ilist, instr,
XINST_CREATE_add(dcontext, opnd_create_reg(DR_REG_SP), OPND_CREATE_INT(XSP_SZ)));

/* FIXME i#1551: once we have cci->num_simd_skip, skip this if possible */
PRE(ilist, instr,
INSTR_CREATE_vldm_wb(dcontext, OPND_CREATE_MEMLIST(DR_REG_SP), SIMD_REG_LIST_LEN,
Expand Down
104 changes: 51 additions & 53 deletions core/arch/arm/arm.asm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,31 +54,17 @@ DECL_EXTERN(initstack_mutex)
#define SAVE_TO_DCONTEXT_VIA_REG(reg,offs,src) str src, PTRSZ [reg, POUND (offs)]

/* offsetof(dcontext_t, dstack) */
#define dstack_OFFSET 0x16c
#define dstack_OFFSET 0x170
/* offsetof(dcontext_t, is_exiting) */
#define is_exiting_OFFSET (dstack_OFFSET+1*ARG_SZ)

#ifdef X64
# define MCXT_NUM_SIMD_SLOTS 32
# define SIMD_REG_SIZE 64
# define MCXT_NUM_PRED_SLOTS 17 /* P regs + FFR */
# define PRED_REG_SIZE 8
# define NUM_GPR_SLOTS 33 /* incl flags */
# define GPR_REG_SIZE 8
#else
# define MCXT_NUM_SIMD_SLOTS 16
# define SIMD_REG_SIZE 16
# define MCXT_NUM_PRED_SLOTS 0
# define PRED_REG_SIZE 0
# define NUM_GPR_SLOTS 17 /* incl flags */
# define GPR_REG_SIZE 4
#endif
#define PRE_SIMD_PADDING 0
#define PRIV_MCXT_SIMD_SIZE (PRE_SIMD_PADDING + MCXT_NUM_SIMD_SLOTS*SIMD_REG_SIZE \
+ MCXT_NUM_PRED_SLOTS*PRED_REG_SIZE)
#define PRIV_MCXT_SIZE (NUM_GPR_SLOTS*GPR_REG_SIZE + PRIV_MCXT_SIMD_SIZE)
#define PRIV_MCXT_SP_FROM_SIMD (-(4*GPR_REG_SIZE)) /* flags, pc, lr, then sp */
#define PRIV_MCXT_PC_FROM_SIMD (-(2*GPR_REG_SIZE)) /* flags, then pc */
/* The struct priv_mcontext_t is defined in mcxtx_api.h. */
#define PRIV_MCXT_SIZE 0x148 /* sizeof(priv_mcontext_t) */
#define PRIV_MCXT_SP_OFFSET 0x34 /* offsetof(priv_mcontext_t, sp) */
#define PRIV_MCXT_LR_OFFSET 0x38 /* offsetof(priv_mcontext_t, lr) */
#define PRIV_MCXT_PC_OFFSET 0x3c /* offsetof(priv_mcontext_t, pc) */
#define PRIV_MCXT_CPSR_OFFSET 0x40 /* offsetof(priv_mcontext_t, cpsr) */
#define PRIV_MCXT_SIMD_OFFSET 0x48 /* offsetof(priv_mcontext_t, simd) */

#ifndef UNIX
# error Non-Unix is not supported
Expand Down Expand Up @@ -181,24 +167,30 @@ GLOBAL_LABEL(dr_call_on_clean_stack:)
#ifdef DR_APP_EXPORTS
DECLARE_EXPORTED_FUNC(dr_app_start)
GLOBAL_LABEL(dr_app_start:)
push {lr}
vstmdb sp!, {d16-d31}
vstmdb sp!, {d0-d15}
mrs REG_R0, cpsr /* r0 is scratch */
push {REG_R0}
/* We can't push all regs w/ writeback */
stmdb sp, {REG_R0-r15}
str lr, [sp, #(PRIV_MCXT_PC_FROM_SIMD+4)] /* +4 b/c we pushed cpsr */
/* we need the sp at function entry */
mov REG_R0, sp
add REG_R0, REG_R0, #(PRIV_MCXT_SIMD_SIZE + 8) /* offset simd,cpsr,lr */
str REG_R0, [sp, #(PRIV_MCXT_SP_FROM_SIMD+4)] /* +4 b/c we pushed cpsr */
sub sp, sp, #(PRIV_MCXT_SIZE-PRIV_MCXT_SIMD_SIZE-4) /* simd,cpsr */
mov REG_R0, sp
#if (PRIV_MCXT_SIZE + 8) % 8 != 0
# error Stack should be 8-byte aligned.
#endif
sub sp, sp, #(PRIV_MCXT_SIZE + 8) /* space for mcontext + LR */
str lr, [sp, #(PRIV_MCXT_SIZE + 4)]
#if PRIV_MCXT_R0_OFFSET != 0
# error
#endif
stmia sp, {r0-r12}
add r0, sp, #(PRIV_MCXT_SIZE + 8) /* get SP at function entry */
str r0, [sp, #PRIV_MCXT_SP_OFFSET]
str lr, [sp, #PRIV_MCXT_LR_OFFSET]
str lr, [sp, #PRIV_MCXT_PC_OFFSET] /* save LR as PC */
mrs r0, cpsr
str r0, [sp, #PRIV_MCXT_CPSR_OFFSET]
add r0, sp, #PRIV_MCXT_SIMD_OFFSET
vstmia r0!, {d0-d15}
vstmia r0!, {d16-d31}
mov r0, sp
CALLC1(GLOBAL_REF(dr_app_start_helper), REG_R0)
/* if we get here, DR is not taking over */
ldr lr, [sp, #(PRIV_MCXT_SIZE + 4)]
add sp, sp, #PRIV_MCXT_SIZE
pop {pc}
bx lr
END_FUNC(dr_app_start)

/*
Expand Down Expand Up @@ -230,25 +222,31 @@ GLOBAL_LABEL(dr_app_running_under_dynamorio:)
*/
DECLARE_EXPORTED_FUNC(dynamorio_app_take_over)
GLOBAL_LABEL(dynamorio_app_take_over:)
push {lr}
vstmdb sp!, {d16-d31}
vstmdb sp!, {d0-d15}
mrs REG_R0, cpsr /* r0 is scratch */
push {REG_R0}
/* We can't push all regs w/ writeback */
stmdb sp, {REG_R0-r15}
str lr, [sp, #(PRIV_MCXT_PC_FROM_SIMD+4)] /* +4 b/c we pushed cpsr */
/* we need the sp at function entry */
mov REG_R0, sp
add REG_R0, REG_R0, #(PRIV_MCXT_SIMD_SIZE + 8) /* offset simd,cpsr,lr */
str REG_R0, [sp, #(PRIV_MCXT_SP_FROM_SIMD+4)] /* +4 b/c we pushed cpsr */
sub sp, sp, #(PRIV_MCXT_SIZE-PRIV_MCXT_SIMD_SIZE-4) /* simd,cpsr */
mov REG_R0, sp
#if (PRIV_MCXT_SIZE + 8) % 8 != 0
# error Stack should be 8-byte aligned.
#endif
sub sp, sp, #(PRIV_MCXT_SIZE + 8) /* space for mcontext + LR */
str lr, [sp, #(PRIV_MCXT_SIZE + 4)]
#if PRIV_MCXT_R0_OFFSET != 0
# error
#endif
stmia sp, {r0-r12}
add r0, sp, #(PRIV_MCXT_SIZE + 8) /* get SP at function entry */
str r0, [sp, #PRIV_MCXT_SP_OFFSET]
str lr, [sp, #PRIV_MCXT_LR_OFFSET]
str lr, [sp, #PRIV_MCXT_PC_OFFSET] /* save LR as PC */
mrs r0, cpsr
str r0, [sp, #PRIV_MCXT_CPSR_OFFSET]
add r0, sp, #PRIV_MCXT_SIMD_OFFSET
vstmia r0!, {d0-d15}
vstmia r0!, {d16-d31}
mov r0, sp
CALLC1(GLOBAL_REF(dynamorio_app_take_over_helper), REG_R0)
/* if we get here, DR is not taking over */
ldr lr, [sp, #(PRIV_MCXT_SIZE + 4)]
add sp, sp, #PRIV_MCXT_SIZE
pop {pc}
END_FUNC(dynamorio_app_take_over)
bx lr
END_FUNC(dr_app_start)


/*
Expand Down
1 change: 1 addition & 0 deletions core/lib/mcxtx_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@
uint apsr; /**< The application program status registers in AArch32. */
uint cpsr; /**< The current program status registers in AArch32. */
}; /**< The anonymous union of alternative names for apsr/cpsr register. */
byte padding[4]; /**< The padding to get simd field 8-byte aligned. */
# endif /* 64/32-bit */

# ifdef X64 /* 64-bit */
Expand Down

0 comments on commit df170af

Please sign in to comment.