-
Notifications
You must be signed in to change notification settings - Fork 546
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.
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#3544 RV64: Implement return address handling for post wrappers #6736
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
dynamorio/suite/tests/CMakeLists.txt Line 3061 in 8e23247
Also, as much as I understand, the one failing test (ci-aarchxx) isn't related to these changes, and is failing on current master. |
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 |
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. |
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 |
@@ -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(). */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (instr_get_opcode(inst) == OP_nop IF_ARM(|| instr_is_mov_nop(inst))) { | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekbruening Why don't we use if (instr_is_nop(inst)) {
here for all architectures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Line too long. This should fail the clang-format check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Line too long. This should fail the clang-format check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
60340f8
to
59add87
Compare
Please don't do force push, see: https://dynamorio.org/page_code_reviews.html. |
Substitute value of return address register to enable post wrappers.
Partially enables drwrap-test for RISC-V:
but checks for clean_call and restore registers functionality are failing.
Issue: #3544