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

WIP: test: add acceptance test for riscv64 #452

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vobst
Copy link
Collaborator

@vobst vobst commented Apr 2, 2024

Compile all acceptance tests for riscv64 (clang and gcc).

TODO:

  • register_calling_convention is missing in the output of the Ghidra plugin (tested 10.2.3). This causes deserialization to fail on the Rust side.
  • also add 32 bit RISCV; no gcc package available (double check); clang-10 should work (check)

@vobst vobst requested a review from Enkelmann April 2, 2024 14:57
Copy link
Contributor

@Enkelmann Enkelmann left a comment

Choose a reason for hiding this comment

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

Adding RISC-V to our test collection is a very good thing! :) However, I am inclined to suggest to postpone merging of this PR for the time being for two reasons:

First, we should also add 32-bit-RISC-V to the CI, because it might be more common in practice than the 64-bit variant.

Second, I do not think we should merge it as long as we cannot test it in the CI. It feels like adding dead code to the repository.

So my suggestion is to leave the PR open (maybe change it to WIP) for now and revisit it when we can actually add the test cases to the acceptance tests.

@vobst vobst marked this pull request as draft April 3, 2024 07:24
@vobst vobst changed the title test: add acceptance test for riscv64 WIP: test: add acceptance test for riscv64 Apr 3, 2024
@vobst
Copy link
Collaborator Author

vobst commented Apr 3, 2024

Adding RISC-V to our test collection is a very good thing! :) However, I am inclined to suggest to postpone merging of this PR for the time being for two reasons:

First, we should also add 32-bit-RISC-V to the CI, because it might be more common in practice than the 64-bit variant.

As far as I can tell there is no gcc package for that; building a toolchain from source is (probably?) not a viable option; clang should hopefully just work; however, I don't know if the tests can handle a situation where an arch is only available from one compiler.

Second, I do not think we should merge it as long as we cannot test it in the CI. It feels like adding dead code to the repository.

So my suggestion is to leave the PR open (maybe change it to WIP) for now and revisit it when we can actually add the test cases to the acceptance tests.

Yea, I think we either need to hope this is fixed by updating the Ghidra version (which we can't across 11+) or by the new approach to Pcode extraction ... one more reason to prioritize work on that I guess.

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.

None yet

2 participants