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.

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 riscv64: Enable dynamorio unit test compile #5929

Merged
merged 15 commits into from
Mar 31, 2023
Merged

i#3544 riscv64: Enable dynamorio unit test compile #5929

merged 15 commits into from
Mar 31, 2023

Conversation

shiptux
Copy link
Member

@shiptux shiptux commented Mar 26, 2023

Make unit tests compile by disabling them all.
Next, enable one test at a time when it passes.

Add RISCV-V 64 support to suite/tests/linux/infinite.c, and clients/drcachesim/tests/raw2trace_unit_tests.cpp
Notice: There are still a large number of unimplemented in Dynamorio RISC-V 64, and it cannot be tested via QEMU

Issue #3544

Make unit tests compile by disabling them all.
Next enable one test at a time when it pass.

Notice: There are still a large number of unimplemented
in Dynamorio RISC-V 64, and it cannot be tested via QEMU

Issue #3544
@shiptux shiptux marked this pull request as draft March 26, 2023 16:07
@shiptux shiptux marked this pull request as ready for review March 26, 2023 16:07
@shiptux
Copy link
Member Author

shiptux commented Mar 27, 2023

@derekbruening
I am sorry, but it seems that I don't have permission to invite someone review this PR.
I cannot click the reviewers.

@bete0 bete0 self-requested a review March 28, 2023 21:26
@derekbruening derekbruening requested review from derekbruening and removed request for bete0 March 29, 2023 04:12
api/samples/CMakeLists.txt Outdated Show resolved Hide resolved
api/samples/CMakeLists.txt Outdated Show resolved Hide resolved
api/samples/CMakeLists.txt Outdated Show resolved Hide resolved
api/samples/CMakeLists.txt Outdated Show resolved Hide resolved
core/ir/aarch64/codec.c Outdated Show resolved Hide resolved
core/ir/aarch64/codec.c Outdated Show resolved Hide resolved
core/ir/aarch64/instr_create_api.h Outdated Show resolved Hide resolved
suite/tests/common/getretaddr.c Outdated Show resolved Hide resolved
suite/tests/tools.c Outdated Show resolved Hide resolved
suite/tests/tools.c Show resolved Hide resolved
@shiptux
Copy link
Member Author

shiptux commented Mar 29, 2023

All done.

@shiptux
Copy link
Member Author

shiptux commented Mar 29, 2023

I updated this PR to the latest master branch, and then I found that the CI could not pass. So I revert the updated commit, this is the reason why my PR has some modifications of the aarch64 decoder. @derekbruening

suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
suite/tests/CMakeLists.txt Outdated Show resolved Hide resolved
@derekbruening
Copy link
Contributor

I updated this PR to the latest master branch, and then I found that the CI could not pass. So I revert the updated commit, this is the reason why my PR has some modifications of the aarch64 decoder

Please add comments/labels explaining that in the future as otherwise it looks like an unintentional mistake when looking just at the diff.

@shiptux
Copy link
Member Author

shiptux commented Mar 30, 2023

I updated this PR to the latest master branch, and then I found that the CI could not pass. So I revert the updated commit, this is the reason why my PR has some modifications of the aarch64 decoder

Please add comments/labels explaining that in the future as otherwise it looks like an unintentional mistake when looking just at the diff.

Thank you for your advice and help. I will pay attention next time.

@derekbruening
Copy link
Contributor

Looks good, except it looks like you've missed one comment about the too-long line: once that's fixed we can merge it in.

@shiptux
Copy link
Member Author

shiptux commented Mar 30, 2023

Looks good, except it looks like you've missed one comment about the too-long line: once that's fixed we can merge it in.

I don't know if the too-long line you said that is:
if (NOT RISCV64) # TODO i#3544: Port tests to RISC-V 64
Or
get_target_path_for_execution(drwrap_libpath client.drwrap-test.appdll "${location_suffix}")
I have already moved # TODO i#3544: Port tests to RISC-V 64 to the second line.

-if (NOT RISCV64) # TODO i#3544: Port tests to RISC-V 64
+if (NOT RISCV64)
+ # TODO i#3544: Port tests to RISC-V 64
  tobuild_appdll(client.drwrap-test client-interface/drwrap-test.c)
  get_target_path_for_execution(drwrap_libpath client.drwrap-test.appdll "${location_suffix}")

@shiptux
Copy link
Member Author

shiptux commented Mar 31, 2023

Sorry for replying too late. These changes are all done. @derekbruening

@derekbruening derekbruening merged commit 3440983 into DynamoRIO:master Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants