i#3544 RV64: Implement return address handling for post wrappers #6736

apach301 · 2024-03-27T12:45:30Z

Substitute value of return address register to enable post wrappers.

Partially enables drwrap-test for RISC-V:

Add drwrap-test-callconv test to pipeline
Partially supported drwrap-drreg-test: check for pre/post wrappers passes,
but checks for clean_call and restore registers functionality are failing.

ksco

I suppose this is covered by some tests, do these tests work now? If yes, maybe state that in the PR description and enable it in the RUN_ON_QEMU label for RISC-V so it can be checked by the CI.

apach301 · 2024-03-28T10:23:54Z

I suppose this is covered by some tests, do these tests work now? If yes, maybe state that in the PR description and enable it in the RUN_ON_QEMU label for RISC-V so it can be checked by the CI.

I tested it only on my DR client. I don't familiar with DynamoRIO test system, and all I found is

dynamorio/suite/tests/CMakeLists.txt

Line 3032 in 8e23247

if (NOT RISCV64)

and

dynamorio/suite/tests/CMakeLists.txt

Line 3061 in 8e23247

if (NOT RISCV64) # TODO i#3544: Port tests to RISC-V 64

that are disabling drwrap tests on RISC-V. Is it sufficient to just remove the guards to enable these tests?

Also, as much as I understand, the one failing test (ci-aarchxx) isn't related to these changes, and is failing on current master.

ksco · 2024-03-28T10:33:25Z

Yes, remove the guards should be enough for CMake to build the test, then add the test name into suite/tests/CMakeLists.txt#L6152 and run ctest -L RUNS_ON_QEMU see if the test passes.

apach301 · 2024-03-29T15:45:44Z

Yes, remove the guards should be enough for CMake to build the test, then add the test name into suite/tests/CMakeLists.txt#L6152 and run ctest -L RUNS_ON_QEMU see if the test passes.

The tests couldn't be compiled currently for RISCV64: it needs a platform-depend fixes in suite/tests/client-interface/drwrap-drreg-test.dll.c, which requires implementing some functions in core/ir/riscv64/instr_create_api.h.in.

ksco · 2024-03-29T15:55:01Z

I suppose you’re referring to XINST_CREATE_cmp() and XINST_CREATE_jmp_cond? There won’t be such thing on RISC-V since we don’t have eflags. You can use INSTR_CREATE_beq() to rewrite this part for RISC-V.

apach301 · 2024-04-02T16:58:21Z

I suppose you’re referring to XINST_CREATE_cmp() and XINST_CREATE_jmp_cond? There won’t be such thing on RISC-V since we don’t have eflags. You can use INSTR_CREATE_beq() to rewrite this part for RISC-V.

Finally being able to compile and run tests. Test drwrap-callconv seems to work, but with drwrap-drreg things more complicated. The part with pre/post wrappers passes, the part with drreg save/restore registers fails.

Moreover, for riscv default callconv is not working, it requires explicitly setup DRWRAP_CALLCONV_RISCV_LP64 | DRWRAP_REPLACE_RETADDR flags

core/ir/riscv64/instr_create_api.h.in

core/arch/asm_defines.asm

ksco · 2024-04-02T17:18:04Z

suite/tests/client-interface/drwrap-drreg-test.dll.c

@@ -331,7 +351,11 @@ event_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t *inst
    /* Look for nop;nop;nop in reg_val_test() and 4 nops in multipath_test(). */


reg_val_test() and multipath_test() are assembly functions and they are currently not ported to RISC-V, maybe add a TODO here if this is a future work.

I wrote assemblies, but it fails on assertion that check reg value restore functionality. I don't know whether it implemented, in this PR I supported only post-wrappers.

ksco · 2024-04-02T17:22:37Z

suite/tests/client-interface/drwrap-drreg-test.dll.c

        if (instr_get_opcode(inst) == OP_nop IF_ARM(|| instr_is_mov_nop(inst))) {
+#endif


@derekbruening Why don't we use if (instr_is_nop(inst)) { here for all architectures?

Probably originally because this is recognizing a specific opcode we control in the app and we don't want a broad check that recognizes other nop variants (on x86 there are several encoding variants of different sizes all considered nops). The tradeoff of a possible false positive pattern recognition might be outweighed by simpler cross-arch code so probably ok to change it.

suite/tests/CMakeLists.txt

derekbruening · 2024-04-03T03:25:03Z

suite/tests/client-interface/drwrap-drreg-test.dll.c

@@ -80,7 +80,11 @@ module_load_event(void *drcontext, const module_data_t *mod, bool loaded)

        addr_two_args = (app_pc)dr_get_proc_address(mod->handle, "two_args");
        CHECK(addr_two_args != NULL, "cannot find lib export");
+#if defined(RISCV64)
+        bool ok = drwrap_wrap_ex(addr_two_args, wrap_pre, wrap_post, 0, DRWRAP_CALLCONV_RISCV_LP64 | DRWRAP_REPLACE_RETADDR);


style: Line too long. This should fail the clang-format check.

derekbruening · 2024-04-03T03:25:30Z

suite/tests/client-interface/drwrap-drreg-test.dll.c

@@ -104,8 +108,8 @@ clean_call_rw(void)
    mc.flags = DR_MC_CONTROL | DR_MC_INTEGER;
    bool ok = dr_get_mcontext(drcontext, &mc);
    CHECK(ok, "dr_get_mcontext failed");
-    CHECK(IF_X86_ELSE(mc.xdx, mc.r1) == 4, "app reg val not restored for clean call");
-    IF_X86_ELSE(mc.xcx, mc.r2) = 3;
+    CHECK(IF_X86_ELSE(mc.xdx, IF_AARCHXX_ELSE(mc.r1, mc.a1)) == 4, "app reg val not restored for clean call");


style: Line too long. This should fail the clang-format check.

ksco · 2024-04-03T10:08:27Z

Please don't do force push, see: https://dynamorio.org/page_code_reviews.html.

ksco reviewed Mar 27, 2024

View reviewed changes

ksco reviewed Apr 2, 2024

View reviewed changes

core/ir/riscv64/instr_create_api.h.in Outdated Show resolved Hide resolved

core/arch/asm_defines.asm Show resolved Hide resolved

core/arch/asm_defines.asm Show resolved Hide resolved

ksco requested a review from derekbruening April 2, 2024 17:13

ksco reviewed Apr 2, 2024

View reviewed changes

suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved

suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved

derekbruening reviewed Apr 3, 2024

View reviewed changes

apach301 added 7 commits April 3, 2024 12:58

i#3544 RV64: Implement return address handling for postcall wrappers

350f0f4

Fix style

2f66583

Try to compile drwrap tests for RISCV64

2048ced

Port drwrap-callconv and drwrap-drreg tests to RISC-V

f18b224

Try to port drwrap-drreg to RISC-V

3dbfc4c

fix

c0242bd

Fix style

59add87

apach301 force-pushed the i3544-fix-post-wrapper branch from 60340f8 to 59add87 Compare April 3, 2024 10:01

Fix style

33c117b

Fix bug

613785a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i#3544 RV64: Implement return address handling for post wrappers #6736

i#3544 RV64: Implement return address handling for post wrappers #6736

apach301 commented Mar 27, 2024 •

edited

ksco left a comment

apach301 commented Mar 28, 2024 •

edited

ksco commented Mar 28, 2024

apach301 commented Mar 29, 2024

ksco commented Mar 29, 2024

apach301 commented Apr 2, 2024 •

edited

ksco Apr 2, 2024 •

edited

apach301 Apr 2, 2024

ksco Apr 2, 2024

derekbruening Apr 2, 2024

apach301 Apr 3, 2024

derekbruening Apr 3, 2024

apach301 Apr 3, 2024

derekbruening Apr 3, 2024

apach301 Apr 3, 2024

ksco commented Apr 3, 2024

		@@ -331,7 +351,11 @@ event_app_instruction(void drcontext, void tag, instrlist_t bb, instr_t inst
		/* Look for nop;nop;nop in reg_val_test() and 4 nops in multipath_test(). */

		if (instr_get_opcode(inst) == OP_nop IF_ARM(\|\| instr_is_mov_nop(inst))) {
		#endif

i#3544 RV64: Implement return address handling for post wrappers #6736

Are you sure you want to change the base?

i#3544 RV64: Implement return address handling for post wrappers #6736

Conversation

apach301 commented Mar 27, 2024 • edited

ksco left a comment

Choose a reason for hiding this comment

apach301 commented Mar 28, 2024 • edited

ksco commented Mar 28, 2024

apach301 commented Mar 29, 2024

ksco commented Mar 29, 2024

apach301 commented Apr 2, 2024 • edited

ksco Apr 2, 2024 • edited

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

ksco commented Apr 3, 2024

apach301 commented Mar 27, 2024 •

edited

apach301 commented Mar 28, 2024 •

edited

apach301 commented Apr 2, 2024 •

edited

ksco Apr 2, 2024 •

edited