From 575e7a58a89a4359d8e8b8e78d16b84595852e76 Mon Sep 17 00:00:00 2001 From: Edmund Grimley Evans Date: Mon, 8 Aug 2016 10:00:54 +0100 Subject: [PATCH] i#1569 AArch64: Make base_disp bitfields in opnd_t architecture-dependent. We will add several bits for AArch64 but we do not want to increase the size of opnd_t. Use ifdef rather than a union. Also make opnd_create_base_disp_arm and opnd_{get,set}_index_shift ARM-only, and opnd_set_disp_ex X86-only. Review-URL: https://codereview.appspot.com/309870043 --- core/arch/opnd.h | 39 +++++++++-------- core/arch/opnd_shared.c | 93 ++++++++++++++++++++++------------------- 2 files changed, 72 insertions(+), 60 deletions(-) diff --git a/core/arch/opnd.h b/core/arch/opnd.h index 10de3366f80..b1d65d72121 100644 --- a/core/arch/opnd.h +++ b/core/arch/opnd.h @@ -823,20 +823,18 @@ struct _opnd_t { /* to get cl to not align to 4 bytes we can't use uint here * when we have reg_id_t elsewhere: it won't combine them * (gcc will). alternative is all uint and no reg_id_t. - */ - /* We would use a union and struct to separate the scale from the 2 - * shift fields as they are mutually exclusive, but that would - * require packing the struct or living with a larger size and perf - * hit on copying it. We also have to use byte and not dr_shift_type_t + * We also have to use byte and not dr_shift_type_t * to get cl to not align. */ - byte/*dr_shift_type_t*/ shift_type : 3; /* ARM-only */ - byte shift_amount_minus_1 : 5; /* ARM-only, 1..31 so we store (val - 1) */ - byte scale : SCALE_SPECIFIER_BITS; /* x86-only */ - /* These 3 are all x86-only: */ +# if defined(ARM) + byte/*dr_shift_type_t*/ shift_type : 3; + byte shift_amount_minus_1 : 5; /* 1..31 so we store (val - 1) */ +# elif defined(X86) + byte scale : SCALE_SPECIFIER_BITS; byte/*bool*/ encode_zero_disp : 1; byte/*bool*/ force_full_disp : 1; /* don't use 8-bit even w/ 8-bit value */ byte/*bool*/ disp_short_addr : 1; /* 16-bit (32 in x64) addr (disp-only) */ +# endif } base_disp; /* BASE_DISP_kind */ void *addr; /* REL_ADDR_kind and ABS_ADDR_kind */ } value; @@ -1063,7 +1061,7 @@ DR_API * On ARM, either \p index_reg must be #DR_REG_NULL or disp must be 0. * * On x86, three boolean parameters give control over encoding optimizations - * (these are ignored on ARM): + * (these are ignored on other architectures): * - If \p encode_zero_disp, a zero value for disp will not be omitted; * - If \p force_full_disp, a small value for disp will not occupy only one byte. * - If \p disp_short_addr, short (16-bit for 32-bit mode, 32-bit for @@ -1132,6 +1130,7 @@ opnd_create_far_base_disp_ex(reg_id_t seg, reg_id_t base_reg, reg_id_t index_reg bool encode_zero_disp, bool force_full_disp, bool disp_short_addr); +#ifdef ARM DR_API /** * Returns a memory reference operand that refers to either a base @@ -1150,11 +1149,13 @@ DR_API * value with #DR_OPND_NEGATED set in opnd_get_flags(). * Either \p index_reg must be #DR_REG_NULL or disp must be 0. * + * \note ARM-only. */ opnd_t opnd_create_base_disp_arm(reg_id_t base_reg, reg_id_t index_reg, dr_shift_type_t shift_type, uint shift_amount, int disp, dr_opnd_flags_t flags, opnd_size_t size); +#endif DR_API /** @@ -1668,6 +1669,7 @@ DR_API reg_id_t opnd_get_segment(opnd_t opnd); +#ifdef ARM DR_API /** * Assumes \p opnd is a (near or far) base+disp memory reference. @@ -1675,6 +1677,7 @@ DR_API * Returns the shift type and \p amount if the index register is shifted (this * shift will occur prior to being added to or subtracted from the base * register). + * \note ARM-only. */ dr_shift_type_t opnd_get_index_shift(opnd_t opnd, uint *amount OUT); @@ -1685,11 +1688,11 @@ DR_API * Sets the index register to be shifted by \p amount according to \p shift. * Returns whether successful. * If the shift amount is out of allowed ranges, returns false. - * \note On non-ARM platforms where shifted index registers do not exist, this - * routine will always fail. + * \note ARM-only. */ bool opnd_set_index_shift(opnd_t *opnd, dr_shift_type_t shift, uint amount); +#endif /* ARM */ DR_API /** @@ -1989,23 +1992,23 @@ DR_API void opnd_set_disp(opnd_t *opnd, int disp); +#ifdef X86 DR_API /** - * Set the displacement and, on x86, the encoding controls of a memory - * reference operand (the controls are ignored on ARM): + * Set the displacement and the encoding controls of a memory + * reference operand: * - If \p encode_zero_disp, a zero value for \p disp will not be omitted; * - If \p force_full_disp, a small value for \p disp will not occupy only one byte. * - If \p disp_short_addr, short (16-bit for 32-bit mode, 32-bit for * 64-bit mode) addressing will be used (note that this normally only * needs to be specified for an absolute address; otherwise, simply - * use the desired short registers for base and/or index). This is only - * honored on x86. - * On ARM, a negative value for \p disp will be converted into a positive - * value with #DR_OPND_NEGATED set in opnd_get_flags(). + * use the desired short registers for base and/or index). + * \note x86-only. */ void opnd_set_disp_ex(opnd_t *opnd, int disp, bool encode_zero_disp, bool force_full_disp, bool disp_short_addr); +#endif DR_API /** diff --git a/core/arch/opnd_shared.c b/core/arch/opnd_shared.c index 66e566711a5..feef0867778 100644 --- a/core/arch/opnd_shared.c +++ b/core/arch/opnd_shared.c @@ -554,25 +554,25 @@ opnd_create_far_base_disp_ex(reg_id_t seg, reg_id_t base_reg, reg_id_t index_reg CLIENT_ASSERT(disp == 0 || index_reg == REG_NULL, "opnd_create_*base_disp*: cannot have both disp and index"); }); + opnd_set_disp_helper(&opnd, disp); opnd.value.base_disp.base_reg = base_reg; opnd.value.base_disp.index_reg = index_reg; - IF_X86_ELSE({ - opnd.value.base_disp.scale = (byte) scale; - }, { - if (scale > 1) { - opnd.value.base_disp.shift_type = DR_SHIFT_LSL; - opnd.value.base_disp.shift_amount_minus_1 = - /* we store the amount minus one */ - (scale == 2 ? 0 : (scale == 4 ? 1 : 2)); - } else { - opnd.value.base_disp.shift_type = DR_SHIFT_NONE; - opnd.value.base_disp.shift_amount_minus_1 = 0; - } - }); - opnd_set_disp_helper(&opnd, disp); +#if defined(ARM) + if (scale > 1) { + opnd.value.base_disp.shift_type = DR_SHIFT_LSL; + opnd.value.base_disp.shift_amount_minus_1 = + /* we store the amount minus one */ + (scale == 2 ? 0 : (scale == 4 ? 1 : 2)); + } else { + opnd.value.base_disp.shift_type = DR_SHIFT_NONE; + opnd.value.base_disp.shift_amount_minus_1 = 0; + } +#elif defined(X86) + opnd.value.base_disp.scale = (byte) scale; opnd.value.base_disp.encode_zero_disp = (byte) encode_zero_disp; opnd.value.base_disp.force_full_disp = (byte) force_full_disp; opnd.value.base_disp.disp_short_addr = (byte) disp_short_addr; +#endif return opnd; } @@ -584,6 +584,7 @@ opnd_create_far_base_disp(reg_id_t seg, reg_id_t base_reg, reg_id_t index_reg, i false, false, false); } +#ifdef ARM opnd_t opnd_create_base_disp_arm(reg_id_t base_reg, reg_id_t index_reg, dr_shift_type_t shift_type, uint shift_amount, int disp, @@ -609,11 +610,9 @@ opnd_create_base_disp_arm(reg_id_t base_reg, reg_id_t index_reg, opnd.aux.flags = flags; if (!opnd_set_index_shift(&opnd, shift_type, shift_amount)) CLIENT_ASSERT(false, "opnd_create_base_disp_arm: invalid shift type/amount"); - opnd.value.base_disp.encode_zero_disp = 0; - opnd.value.base_disp.force_full_disp = 0; - opnd.value.base_disp.disp_short_addr = 0; return opnd; } +#endif #undef opnd_get_base #undef opnd_get_disp @@ -631,6 +630,7 @@ reg_id_t opnd_get_segment(opnd_t opnd) { return OPND_GET_SEGMENT(opnd); } #define opnd_get_scale OPND_GET_SCALE #define opnd_get_segment OPND_GET_SEGMENT +#ifdef ARM dr_shift_type_t opnd_get_index_shift(opnd_t opnd, uint *amount OUT) { @@ -696,12 +696,13 @@ opnd_set_index_shift(opnd_t *opnd, dr_shift_type_t shift, uint amount) opnd->value.base_disp.shift_type = shift; return true; } +#endif /* ARM */ bool opnd_is_disp_encode_zero(opnd_t opnd) { if (opnd_is_base_disp(opnd)) - return opnd.value.base_disp.encode_zero_disp; + return IF_X86_ELSE(opnd.value.base_disp.encode_zero_disp, false); CLIENT_ASSERT(false, "opnd_is_disp_encode_zero called on invalid opnd type"); return false; } @@ -710,7 +711,7 @@ bool opnd_is_disp_force_full(opnd_t opnd) { if (opnd_is_base_disp(opnd)) - return opnd.value.base_disp.force_full_disp; + return IF_X86_ELSE(opnd.value.base_disp.force_full_disp, false); CLIENT_ASSERT(false, "opnd_is_disp_force_full called on invalid opnd type"); return false; } @@ -719,7 +720,7 @@ bool opnd_is_disp_short_addr(opnd_t opnd) { if (opnd_is_base_disp(opnd)) - return opnd.value.base_disp.disp_short_addr; + return IF_X86_ELSE(opnd.value.base_disp.disp_short_addr, false); CLIENT_ASSERT(false, "opnd_is_disp_short_addr called on invalid opnd type"); return false; } @@ -733,6 +734,7 @@ opnd_set_disp(opnd_t *opnd, int disp) CLIENT_ASSERT(false, "opnd_set_disp called on invalid opnd type"); } +#ifdef X86 void opnd_set_disp_ex(opnd_t *opnd, int disp, bool encode_zero_disp, bool force_full_disp, bool disp_short_addr) @@ -745,6 +747,7 @@ opnd_set_disp_ex(opnd_t *opnd, int disp, bool encode_zero_disp, bool force_full_ } else CLIENT_ASSERT(false, "opnd_set_disp_ex called on invalid opnd type"); } +#endif opnd_t opnd_create_abs_addr(void *addr, opnd_size_t data_size) @@ -1053,7 +1056,10 @@ opnd_replace_reg(opnd_t *opnd, reg_id_t old_reg, reg_id_t new_reg) reg_id_t b = (old_reg == ob) ? new_reg : ob; reg_id_t i = (old_reg == oi) ? new_reg : oi; int d = opnd_get_disp(*opnd); -#ifdef AARCHXX +#if defined(AARCH64) + /* FIXME i#1569: Include extension and shift. */ + *opnd = opnd_create_base_disp(b, i, 0, d, size); +#elif defined(ARM) uint amount; dr_shift_type_t shift = opnd_get_index_shift(*opnd, &amount); dr_opnd_flags_t flags = opnd_get_flags(*opnd); @@ -1185,24 +1191,25 @@ bool opnd_same(opnd_t op1, opnd_t op2) return (IF_X86(op1.aux.segment == op2.aux.segment &&) op1.value.base_disp.base_reg == op2.value.base_disp.base_reg && op1.value.base_disp.index_reg == op2.value.base_disp.index_reg && - IF_X86_ELSE(op1.value.base_disp.scale == - op2.value.base_disp.scale &&, - op1.value.base_disp.shift_type == - op2.value.base_disp.shift_type && - op1.value.base_disp.shift_amount_minus_1 == - op2.value.base_disp.shift_amount_minus_1 &&) + IF_X86(op1.value.base_disp.scale == + op2.value.base_disp.scale &&) + IF_ARM(op1.value.base_disp.shift_type == + op2.value.base_disp.shift_type && + op1.value.base_disp.shift_amount_minus_1 == + op2.value.base_disp.shift_amount_minus_1 &&) op1.value.base_disp.disp == op2.value.base_disp.disp && - op1.value.base_disp.encode_zero_disp == - op2.value.base_disp.encode_zero_disp && - op1.value.base_disp.force_full_disp == - op2.value.base_disp.force_full_disp && - /* disp_short_addr only matters if no registers are set */ - (((op1.value.base_disp.base_reg != REG_NULL || - op1.value.base_disp.index_reg != REG_NULL) && - (op2.value.base_disp.base_reg != REG_NULL || - op2.value.base_disp.index_reg != REG_NULL)) || - op1.value.base_disp.disp_short_addr == - op2.value.base_disp.disp_short_addr)); + IF_X86(op1.value.base_disp.encode_zero_disp == + op2.value.base_disp.encode_zero_disp && + op1.value.base_disp.force_full_disp == + op2.value.base_disp.force_full_disp && + /* disp_short_addr only matters if no registers are set */ + (((op1.value.base_disp.base_reg != REG_NULL || + op1.value.base_disp.index_reg != REG_NULL) && + (op2.value.base_disp.base_reg != REG_NULL || + op2.value.base_disp.index_reg != REG_NULL)) || + op1.value.base_disp.disp_short_addr == + op2.value.base_disp.disp_short_addr) &&) + true); #if defined(X64) || defined(ARM) case REL_ADDR_kind: #endif @@ -1786,10 +1793,14 @@ opnd_compute_address_priv(opnd_t opnd, priv_mcontext_t *mc) ptr_int_t scaled_index = 0; if (opnd_is_base_disp(opnd)) { reg_id_t index = opnd_get_index(opnd); -#ifdef X86 +#if defined(X86) ptr_int_t scale = opnd_get_scale(opnd); scaled_index = scale * reg_get_value_priv(index, mc); -#elif defined(AARCHXX) +#elif defined(AARCH64) + reg_t index_val = reg_get_value_priv(index, mc); + /* FIXME i#1569: Compute extension and shift. */ + scaled_index = index_val; +#elif defined(ARM) uint amount; dr_shift_type_t type = opnd_get_index_shift(opnd, &amount); reg_t index_val = reg_get_value_priv(index, mc); @@ -1797,7 +1808,6 @@ opnd_compute_address_priv(opnd_t opnd, priv_mcontext_t *mc) case DR_SHIFT_LSL: scaled_index = index_val << amount; break; -# ifndef AARCH64 case DR_SHIFT_LSR: scaled_index = index_val >> amount; break; @@ -1812,7 +1822,6 @@ opnd_compute_address_priv(opnd_t opnd, priv_mcontext_t *mc) scaled_index = (index_val >> 1) || (TEST(EFLAGS_C, mc->cpsr) ? (1 << (sizeof(reg_t)*8-1)) : 0); break; -# endif default: scaled_index = index_val; }