i#2297: AARCH64: Implement cbr instrumentation #7005

jiegec · 2024-09-25T17:10:16Z

Implement cbr instrumentation for AARCH64, supporting cbz/cbnz/tbz/tbnz/bcond opcodes.

Implement cbr instrumentation for AARCH64, supporting cbz/cbnz/tbz/tbnz/bcond opcodes. Issue: DynamoRIO#2297

AssadHashmi · 2024-09-25T21:32:45Z

core/ir/aarch64/instr_create_api.h

-#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)                                            \


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. */

Thanks, added

AssadHashmi · 2024-09-25T21:32:52Z

core/ir/aarch64/instr_create_api.h

 #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) \


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

Thanks, done

Document newly added instruction creation macros for EOR, CSINC and UBFM. Refactor XINST_CREATE_slr_s to use the new macro to create ubfm instruction. Fixes DynamoRIO#2297

derekbruening

Thank you for the contribution. The main missing piece is testing: see the comments.

derekbruening · 2024-09-26T16:02:42Z

core/ir/aarch64/instr_create_api.h

@@ -463,12 +463,12 @@
 * they just need to know whether they need to preserve the app's flags, so maybe


Please enable the samples that use this which are currently disabled:

# dr_insert_cbr_instrument_ex is NYI add_sample_client(cbrtrace "cbrtrace.c;utils.c" "drmgr;drx") add_sample_client(hot_bbcount "hot_bbcount.c" "drmgr;drreg;drbbdup;drx")

Those are run as tests, though w/o targeted correctness checks: just making sure they don't crash.

derekbruening · 2024-09-26T16:03:15Z

core/ir/aarch64/instr_create_api.h

@@ -463,12 +463,12 @@
 * they just need to know whether they need to preserve the app's flags, so maybe


Please enable the count-ctis tests which use this, which will add regression tests.

Since count-ctis test requires mbr instrumentation as well, I have added initial implementation for mbr instrumentation. But it sometimes return a very small number, possibly some index into the indirect branch cache? How to convert it back to actual address?

Oh, didn't realize it needed more: was just grepping for tests that use cbr. Makes sense to separate out the mbr. Is it easy to separate in the test? Or just separately locally in the test and confirm cbr works and state that in the PR description and say that the test will be enabled soon when mbr is added and then enable the test in a separate PR for mbr, so long as that comes in relatively soon (i.e., not months later with no cbr test in the meantime).

mbr is supposed to obtain a real address so that sounds like something is wrong there.

derekbruening · 2024-09-26T16:04:48Z

core/ir/aarch64/instr_create_api.h

@@ -463,12 +463,12 @@
 * they just need to know whether they need to preserve the app's flags, so maybe


Please update the PR description to describe how you tested this, since there are no tests added/enabled by this PR (see prior comment which will add some).

derekbruening · 2024-09-26T16:09:14Z

core/ir/aarch64/instr_create_api.h

+    instr_create_1dst_3src(dc, OP_csinc, rd, rn, rm, cond)
+
+/**
+ * Creates an UBFM instruction with one output and three inputs.


(nit: Do you pronounce this "uhb-fmmm"? My brain doesn't like the "an" because I say it "you-bee-eff-emm" and so I want "a" not "an" but if you start it with "uh" feel free to leave the "an".)

derekbruening · 2024-09-26T16:09:41Z

core/lib/instrument.c

+    reg_id_t dir = DR_REG_NULL;
+    reg_id_t flags = DR_REG_NULL;
+    int opc;
+    ;


Please remove.

derekbruening · 2024-09-26T16:11:03Z

core/lib/instrument.c

+                  "dr_insert_cbr_instrumentation must be applied to a cbr");
+    target = (ptr_uint_t)opnd_get_pc(instr_get_target(instr));
+
+    /* Compute branch direction */


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.

derekbruening · 2024-09-26T16:14:33Z

core/lib/instrument.c

+        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;


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.

derekbruening · 2024-09-26T16:35:09Z

core/lib/instrument.c

+        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 */


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.

derekbruening · 2024-09-26T16:36:11Z

core/lib/instrument.c

+        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) {


Use reg_is_64bit()

derekbruening · 2024-09-26T16:38:58Z

core/lib/instrument.c

+        } 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;


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));

Issue: DynamoRIO#2919

jiegec · 2024-10-28T05:49:09Z

Help is wanted to fix mbr instrumentation: it sometimes receives small integers instead of actual target address, possibly some indices into the basic box cache?

derekbruening · 2024-10-28T15:41:40Z

Help is wanted to fix mbr instrumentation: it sometimes receives small integers instead of actual target address, possibly some indices into the basic box cache?

I assume it has the wrong register or something. There are no such "indices" in in-fragment code. Use the logs https://dynamorio.org/page_logging.html to see where the app's register values goes and which one is passed to the clean call.

i#2297: AARCH64: Implement cbr instrumentation

a08aec1

Implement cbr instrumentation for AARCH64, supporting cbz/cbnz/tbz/tbnz/bcond opcodes. Issue: DynamoRIO#2297

AssadHashmi reviewed Sep 25, 2024

View reviewed changes

AssadHashmi requested a review from derekbruening September 25, 2024 21:35

i#2297: Document and refactor new instr_create_api for AARCH64

01872ba

Document newly added instruction creation macros for EOR, CSINC and UBFM. Refactor XINST_CREATE_slr_s to use the new macro to create ubfm instruction. Fixes DynamoRIO#2297

derekbruening reviewed Sep 26, 2024

View reviewed changes

jiegec added 5 commits September 27, 2024 10:07

i#2297: Enable cbr tests on AARCH64

79e3375

i#2297: Refactor code and conform to code style

b5155c3

i#2297: Add AARCH64 support to count-ctis test

78f6037

i#2919: Implement mbr instrumentation on AARCH64

22e365d

Issue: DynamoRIO#2919

i#2297: Add XXX comments to note different impls

f02728e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i#2297: AARCH64: Implement cbr instrumentation #7005

i#2297: AARCH64: Implement cbr instrumentation #7005

jiegec commented Sep 25, 2024

AssadHashmi Sep 25, 2024

jiegec Sep 26, 2024

AssadHashmi Sep 25, 2024

jiegec Sep 26, 2024

derekbruening left a comment

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

jiegec Sep 27, 2024

derekbruening Sep 27, 2024

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

derekbruening Sep 26, 2024

jiegec commented Oct 28, 2024

derekbruening commented Oct 28, 2024

		@@ -463,12 +463,12 @@
		* they just need to know whether they need to preserve the app's flags, so maybe

i#2297: AARCH64: Implement cbr instrumentation #7005

Are you sure you want to change the base?

i#2297: AARCH64: Implement cbr instrumentation #7005

Conversation

jiegec commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiegec commented Oct 28, 2024

derekbruening commented Oct 28, 2024