Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#3699 ARM: Add padding to priv_mcontext_t for 8-byte alignment. #7191

Merged
merged 9 commits into from
Jan 17, 2025
Merged

i#3699 ARM: Add padding to priv_mcontext_t for 8-byte alignment. #7191

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
df170af
i#3699 ARM: Add padding to priv_mcontext_t for 8-byte alignment.
egrimley-arm Jan 16, 2025
c5435b5
Merge branch 'master' into i3699-mcontext
egrimley-arm Jan 16, 2025
3f087b2
Improve comments in arm.asm.
egrimley-arm Jan 16, 2025
a53fcb8
arm.asm: Add Arm copyright.
egrimley-arm Jan 16, 2025
fab4f04
Change version from 11.3 to 11.90.
egrimley-arm Jan 16, 2025
f26be17
Merge branch 'master' into i3699-mcontext
egrimley-arm Jan 16, 2025
c7b93b0
Merge branch 'master' into i3699-mcontext
egrimley-arm Jan 17, 2025
cb72c38
Improve change log and add uint64 in priv_mcontext_t for alignment.
egrimley-arm Jan 17, 2025
7f3a6a7
Revert addition of uint64: it causes problems.
egrimley-arm Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci-docs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ jobs:
# We only use a non-zero build # when making multiple manual builds in one day.
run: |
if test -z "${{ github.event.inputs.version }}"; then
export VERSION_NUMBER=11.3.$((`git log -n 1 --format=%ct` / (60*60*24)))
export VERSION_NUMBER=11.90.$((`git log -n 1 --format=%ct` / (60*60*24)))
else
export VERSION_NUMBER=${{ github.event.inputs.version }}
fi
Expand Down
12 changes: 6 additions & 6 deletions .github/workflows/ci-package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ jobs:
# We only use a non-zero build # when making multiple manual builds in one day.
run: |
if test -z "${{ github.event.inputs.version }}"; then
export VERSION_NUMBER=11.3.$((`git log -n 1 --format=%ct` / (60*60*24)))
export VERSION_NUMBER=11.90.$((`git log -n 1 --format=%ct` / (60*60*24)))
else
export VERSION_NUMBER=${{ github.event.inputs.version }}
fi
Expand Down Expand Up @@ -196,7 +196,7 @@ jobs:
# XXX: See x86 job comments on sharing the default ver# with CMakeLists.txt.
run: |
if test -z "${{ github.event.inputs.version }}"; then
export VERSION_NUMBER=11.3.$((`git log -n 1 --format=%ct` / (60*60*24)))
export VERSION_NUMBER=11.90.$((`git log -n 1 --format=%ct` / (60*60*24)))
else
export VERSION_NUMBER=${{ github.event.inputs.version }}
fi
Expand Down Expand Up @@ -285,7 +285,7 @@ jobs:
# XXX: See x86 job comments on sharing the default ver# with CMakeLists.txt.
run: |
if test -z "${{ github.event.inputs.version }}"; then
export VERSION_NUMBER=11.3.$((`git log -n 1 --format=%ct` / (60*60*24)))
export VERSION_NUMBER=11.90.$((`git log -n 1 --format=%ct` / (60*60*24)))
else
export VERSION_NUMBER=${{ github.event.inputs.version }}
fi
Expand Down Expand Up @@ -374,7 +374,7 @@ jobs:
# XXX: See x86 job comments on sharing the default ver# with CMakeLists.txt.
run: |
if test -z "${{ github.event.inputs.version }}"; then
export VERSION_NUMBER=11.3.$((`git log -n 1 --format=%ct` / (60*60*24)))
export VERSION_NUMBER=11.90.$((`git log -n 1 --format=%ct` / (60*60*24)))
else
export VERSION_NUMBER=${{ github.event.inputs.version }}
fi
Expand Down Expand Up @@ -457,7 +457,7 @@ jobs:
# XXX: See x86 job comments on sharing the default ver# with CMakeLists.txt.
run: |
if test -z "${{ github.event.inputs.version }}"; then
export VERSION_NUMBER=11.3.$((`git log -n 1 --format=%ct` / (60*60*24)))
export VERSION_NUMBER=11.90.$((`git log -n 1 --format=%ct` / (60*60*24)))
else
export VERSION_NUMBER=${{ github.event.inputs.version }}
fi
Expand Down Expand Up @@ -541,7 +541,7 @@ jobs:
# XXX: See x86 job comments on sharing the default ver# with CMakeLists.txt.
run: |
if test -z "${{ github.event.inputs.version }}"; then
export VERSION_NUMBER="11.3.$((`git log -n 1 --format=%ct` / (60*60*24)))"
export VERSION_NUMBER="11.90.$((`git log -n 1 --format=%ct` / (60*60*24)))"
export PREFIX="cronbuild-"
else
export VERSION_NUMBER=${{ github.event.inputs.version }}
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ endif (EXISTS "${PROJECT_SOURCE_DIR}/.svn")

# N.B.: When updating this, update all the default versions in ci-package.yml
# and ci-docs.yml. We should find a way to share (xref i#1565).
set(VERSION_NUMBER_DEFAULT "11.3.${VERSION_NUMBER_PATCHLEVEL}")
set(VERSION_NUMBER_DEFAULT "11.90.${VERSION_NUMBER_PATCHLEVEL}")
# do not store the default VERSION_NUMBER in the cache to prevent a stale one
# from preventing future version updates in a pre-existing build dir
set(VERSION_NUMBER "" CACHE STRING "Version number: leave empty for default")
Expand Down
3 changes: 2 additions & 1 deletion api/docs/release.dox
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ clients.

The changes between version \DR_VERSION and 11.3.0 include the following compatibility
changes:
- No compatibility changes yet.
- The size of #dr_mcontext_t on 32-bit Arm has been increased by 4 to
make it 8-byte aligned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it sound like we appended padding to the end where a recompile is enough; but really we inserted padding in between fields, so any code such as generated machine instructions for instrumentation has to be updated as the field offsets have changed. I think this needs to explicitly say that some field offsets have changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've improved the change log ... and I've just realised that I hadn't actually made priv_mcontext_t 8-byte aligned! I'd changed its size to a multiple of 8, and in the places where it is allocated in assembly on the stack it is 8-byte aligned because SP is 8-byte aligned, and it would be automatically 8-byte aligned if someone were to use malloc, but a priv_mcontext_t local variable might not be 8-byte aligned as it was. So I've added a uint64 _unused in an anonymous union in the struct definition. Do you think that's the thing to do and the right way to do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've added a uint64 _unused in an anonymous union in the struct definition. Do you think that's the thing to do and the right way to do it?

Use ALIGN_VAR(8).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you marked priv_mcontext_t as ALIGN_VAR, so it won't be aligned, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I may have been a bit hasty in merging that, and the change log entry is unclear: I think I honestly meant "it" to refer to the "size" when I wrote that but readers will, of course, understand "it" to refer to the struct!

Enforcing alignment seems to cause a problem with cross-compilation that I'm not yet set up to reproduce on a local machine: #7196.

We must either debug that cross-compilation problem and enforce alignment, or correct that log message to make it clear that all we have done is make the size a multiple of 8 in order to facilitate alignment.

(The cross-compilation problem seems rather interesting. If there are lingering alignment issues in the code I wonder if #7161 will turn out to be alignment-related.)


Further non-compatibility-affecting changes include:
- Added support for reading a single drmemtrace trace file from stdin
Expand Down
20 changes: 10 additions & 10 deletions core/arch/aarchxx/mangle.c
View file
Open in desktop
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
107 changes: 54 additions & 53 deletions core/arch/arm/arm.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* **********************************************************
* Copyright (c) 2014-2022 Google, Inc. All rights reserved.
* Copyright (c) 2025 Arm Limited. All rights reserved.
* ********************************************************** */

/*
Expand Down Expand Up @@ -54,31 +55,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 +168,31 @@ 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 != 0
# error Size of priv_mcontext_t should be 8-byte aligned.
#endif
/* space for mcontext + padding + LR (stack must be 8-byte aligned */
sub sp, sp, #(PRIV_MCXT_SIZE + 8)
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 +224,32 @@ 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 != 0
# error Size of priv_mcontext_t should be 8-byte aligned.
#endif
/* space for mcontext + padding + LR (stack must be 8-byte aligned */
sub sp, sp, #(PRIV_MCXT_SIZE + 8)
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
Open in desktop
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
Loading