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#2297: AARCH64: Implement cbr instrumentation #7005

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

i#2297: AARCH64: Implement cbr instrumentation #7005

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a08aec1
i#2297: AARCH64: Implement cbr instrumentation
jiegec Sep 25, 2024
01872ba
i#2297: Document and refactor new instr_create_api for AARCH64
jiegec Sep 26, 2024
79e3375
i#2297: Enable cbr tests on AARCH64
jiegec Sep 27, 2024
b5155c3
i#2297: Refactor code and conform to code style
jiegec Sep 27, 2024
78f6037
i#2297: Add AARCH64 support to count-ctis test
jiegec Sep 27, 2024
22e365d
i#2919: Implement mbr instrumentation on AARCH64
jiegec Sep 27, 2024
f02728e
i#2297: Add XXX comments to note different impls
jiegec Sep 27, 2024
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
12 changes: 9 additions & 3 deletions core/ir/aarch64/instr_create_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,13 +658,19 @@
instr_create_0dst_3src((dc), OP_tbnz, (pc), (reg), (imm))
#define INSTR_CREATE_cmp(dc, rn, rm_or_imm) \
INSTR_CREATE_subs(dc, OPND_CREATE_ZR(rn), rn, rm_or_imm)
#define INSTR_CREATE_eor(dc, d, s) \
INSTR_CREATE_eor_shift(dc, d, d, s, OPND_CREATE_INT8(DR_SHIFT_LSL), \
OPND_CREATE_INT8(0))
#define INSTR_CREATE_eor(dc, d, s_or_imm) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for contributing.

Ideally, every new addition of an INSTR_CREATE_ or XINSTR_CREATE_ should include the Doxygen comment block describing the macro, e.g.

/**                                                                       
 * Creates an EOR instruction with one output and two inputs.             
 * \param dc   The void * dcontext used to allocate memory for the instr_t.
 * \param rd   The output register.                                       
 * \param rn   The first input register.                                  
 * \param rm_or_imm   The second input register or immediate.             
 */

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, added

opnd_is_immed(s_or_imm) \
? instr_create_1dst_2src(dc, OP_eor, d, d, s_or_imm) \
: INSTR_CREATE_eor_shift(dc, d, d, s_or_imm, OPND_CREATE_INT8(DR_SHIFT_LSL), \
OPND_CREATE_INT8(0))
#define INSTR_CREATE_eor_shift(dc, rd, rn, rm, sht, sha) \
instr_create_1dst_4src(dc, OP_eor, rd, rn, \
opnd_create_reg_ex(opnd_get_reg(rm), 0, DR_OPND_SHIFTED), \
opnd_add_flags(sht, DR_OPND_IS_SHIFT), sha)
#define INSTR_CREATE_csinc(dc, rd, rn, rm, cond) \
instr_create_1dst_3src(dc, OP_csinc, rd, rn, rm, cond)
#define INSTR_CREATE_ubfm(dc, rd, rn, lsb, width) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace the instr_create_1dst_3src(dc, OP_ubfm... in XINST_CREATE_slr_s() with this INSTR_CREATE_ubfm().

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, done

instr_create_1dst_3src(dc, OP_ubfm, rd, rn, lsb, width)

#define INSTR_CREATE_ldp(dc, rt1, rt2, mem) \
instr_create_2dst_1src(dc, OP_ldp, rt1, rt2, mem)
Expand Down
140 changes: 140 additions & 0 deletions core/lib/instrument.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6328,6 +6328,146 @@ dr_insert_cbr_instrumentation_help(void *drcontext, instrlist_t *ilist, instr_t
#elif defined(RISCV64)
/* FIXME i#3544: Not implemented */
ASSERT_NOT_IMPLEMENTED(false);
#elif defined(AARCH64)
dcontext_t *dcontext = (dcontext_t *)drcontext;
ptr_uint_t address, target;
reg_id_t dir = DR_REG_NULL;
reg_id_t flags = DR_REG_NULL;
int opc;
;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove.

CLIENT_ASSERT(drcontext != NULL,
"dr_insert_cbr_instrumentation: drcontext cannot be NULL");
address = (ptr_uint_t)instr_get_translation(instr);
CLIENT_ASSERT(address != 0,
"dr_insert_cbr_instrumentation: can't determine app address");
CLIENT_ASSERT(instr_is_cbr(instr),
"dr_insert_cbr_instrumentation must be applied to a cbr");
target = (ptr_uint_t)opnd_get_pc(instr_get_target(instr));

/* Compute branch direction */
Copy link
Contributor

Choose a reason for hiding this comment

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

style: For all new comments, please capitalize and use end-punctuation: so end period here, and capitalize and add end periods to all the ones below: line 6352, line 6354, etc.

opc = instr_get_opcode(instr);
if (opc == OP_cbnz || opc == OP_cbz) {
opnd_t reg_op = instr_get_src(instr, 1);
reg_id_t reg = opnd_get_reg(reg_op);
/* use dir register to compute direction */
dir = (reg == DR_REG_X0 || reg == DR_REG_W0) ? DR_REG_X1 : DR_REG_X0;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO using reg_to_pointer_sized(reg) == DR_REG_X0 is better than checking both X0 and W0 since it will generalize to other architectures or SIMD registers which have more than 2 overlapping. Ditto below.

/* save old value of dir register to SPILL_SLOT_1 */
dr_save_reg(dcontext, ilist, instr, dir, SPILL_SLOT_1);
/* use flags register to save nzcv */
flags = (reg == DR_REG_X2 || reg == DR_REG_W2) ? DR_REG_X3 : DR_REG_X2;
/* save old value of flags register to SPILL_SLOT_2 */
dr_save_reg(dcontext, ilist, instr, flags, SPILL_SLOT_2);
/* save flags to flags register */
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, instead of using CMP and having to save the flags is it better to branch and use the same CBZ/CBNZ to set dir_op to 0 or 1? Fewer loads+stores, but has a branch...my intuition says avoiding the store is worth having the branch. But probably should be profiled. OK to just put a XXX comment about it.

dr_save_arith_flags_to_reg(dcontext, ilist, instr, flags);

/* compare reg against zero */
instr_t *cmp = INSTR_CREATE_cmp(dcontext, reg_op, OPND_CREATE_INT(0));
MINSERT(ilist, instr, cmp);
/* compute branch direction */
opnd_t dir_op = opnd_create_reg(dir);
instr_t *cset = INSTR_CREATE_csinc(
dcontext, dir_op, OPND_CREATE_ZR(dir_op), OPND_CREATE_ZR(dir_op),
opnd_create_cond(opc == OP_cbnz ? DR_PRED_EQ : DR_PRED_NE));
MINSERT(ilist, instr, cset);
} else if (opc == OP_tbnz || opc == OP_tbz) {
opnd_t reg_op = instr_get_src(instr, 1);
reg_id_t reg = opnd_get_reg(reg_op);
reg_id_t dir_same_width = DR_REG_NULL;

/* use dir register to compute direction */
if (DR_REG_START_64 <= reg && reg <= DR_REG_STOP_64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use reg_is_64bit()

/* 64-bit register */
dir = (reg == DR_REG_X0) ? DR_REG_X1 : DR_REG_X0;
dir_same_width = (reg == DR_REG_X0) ? DR_REG_X1 : DR_REG_X0;
} else {
/* 32-bit register */
dir = (reg == DR_REG_W0) ? DR_REG_X1 : DR_REG_X0;
dir_same_width = (reg == DR_REG_W0) ? DR_REG_W1 : DR_REG_W0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you can use API routines to make this simpler, replacing this if/else lines 6378-6386 with this:

dir = (reg_to_pointer_sized(reg) == DR_REG_X0) ? DR_REG_X1 : DR_REG_X0;
dir_same_width = reg_resize_to_opsz(dir, opnd_get_size(reg));

}
/* save old value of dir register to SPILL_SLOT_1 */
dr_save_reg(dcontext, ilist, instr, dir, SPILL_SLOT_1);

/* extract tst_bit from reg */
int tst_bit = opnd_get_immed_int(instr_get_src(instr, 2));
opnd_t dir_same_width_op = opnd_create_reg(dir_same_width);
instr_t *ubfm =
INSTR_CREATE_ubfm(dcontext, dir_same_width_op, reg_op,
OPND_CREATE_INT(tst_bit), OPND_CREATE_INT(tst_bit));
MINSERT(ilist, instr, ubfm);

/* invert result if tbz */
if (opc == OP_tbz) {
instr_t *eor =
INSTR_CREATE_eor(dcontext, dir_same_width_op, OPND_CREATE_INT(1));
MINSERT(ilist, instr, eor);
}
} else if (opc == OP_bcond) {
/* use dir register to compute direction */
dir = SCRATCH_REG0;
/* save old value of dir register to SPILL_SLOT_1 */
dr_save_reg(dcontext, ilist, instr, dir, SPILL_SLOT_1);
/* compute branch direction */
dr_pred_type_t pred = instr_get_predicate(instr);
opnd_t dir_op = opnd_create_reg(dir);
instr_t *cset = INSTR_CREATE_csinc(
dcontext, dir_op, OPND_CREATE_ZR(dir_op), OPND_CREATE_ZR(dir_op),
opnd_create_cond(instr_invert_predicate(pred)));
MINSERT(ilist, instr, cset);
} else {
CLIENT_ASSERT(false, "unknown conditional branch type");
return;
}

if (has_fallthrough) {
ptr_uint_t fallthrough = address + instr_length(drcontext, instr);
CLIENT_ASSERT(fallthrough > address, "wrong fallthrough address");
dr_insert_clean_call_ex(
drcontext, ilist, instr, callee,
/* Many users will ask for mcontexts; some will set; it doesn't seem worth
* asking the user to pass in a flag: if they're using this they are not
* super concerned about overhead.
*/
DR_CLEANCALL_READS_APP_CONTEXT | DR_CLEANCALL_WRITES_APP_CONTEXT, 5,
/* address of cbr is 1st parameter */
OPND_CREATE_INTPTR(address),
/* target is 2nd parameter */
OPND_CREATE_INTPTR(target),
/* fall-through is 3rd parameter */
OPND_CREATE_INTPTR(fallthrough),
/* branch direction is 4th parameter */
opnd_create_reg(dir),
/* user defined data is 5th parameter */
opnd_is_null(user_data) ? OPND_CREATE_INT32(0) : user_data);
} else {
dr_insert_clean_call_ex(
drcontext, ilist, instr, callee,
/* Many users will ask for mcontexts; some will set; it doesn't seem worth
* asking the user to pass in a flag: if they're using this they are not
* super concerned about overhead.
*/
DR_CLEANCALL_READS_APP_CONTEXT | DR_CLEANCALL_WRITES_APP_CONTEXT, 3,
/* address of cbr is 1st parameter */
OPND_CREATE_INTPTR(address),
/* target is 2nd parameter */
OPND_CREATE_INTPTR(target),
/* branch direction is 3rd parameter */
opnd_create_reg(dir));
}

/* Restore state */
if (opc == OP_cbnz || opc == OP_cbz) {
/* restore arith flags */
dr_restore_arith_flags_from_reg(dcontext, ilist, instr, flags);
/* restore old value of flags register */
dr_restore_reg(dcontext, ilist, instr, flags, SPILL_SLOT_2);
/* restore old value of dir register */
dr_restore_reg(dcontext, ilist, instr, dir, SPILL_SLOT_1);
} else if (opc == OP_bcond || opc == OP_tbnz || opc == OP_tbz) {
/* restore old value of dir register */
dr_restore_reg(dcontext, ilist, instr, dir, SPILL_SLOT_1);
} else {
CLIENT_ASSERT(false, "unknown conditional branch type");
}
#endif /* X86/ARM/RISCV64 */
}

Expand Down
Loading