elliottt edited PR #5780 from trevor/capstone-output-filetests
to main
:
- Add disassembly to precise-output tests
- Bless existing tests
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt submitted PR review.
elliottt created PR review comment:
I'm not happy with how much code was inlined from clif-util here, maybe there's a better place for common testing code like this?
elliottt edited PR #5780 from trevor/capstone-output-filetests
to main
:
Check both the printed vcode and capstone output in
precise-output
tests. This lets us continue to rely on printing backend-specific pseudoinstructions in the VCode output (for instancebr_table
on x64), while also checking the final output of the machinst buffer.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt has marked PR #5780 as ready for review.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
There are a few cases, like here, where the disassembly loses some info because it doesn't carry the metadata -- in this case, a relocation (the call target). Not so important for this particular test but it would be good to audit for this somehow -- any thoughts?
cfallin created PR review comment:
We could put a
.to_capstone()
method onTargetIsa
and enable it only under acapstone
Cargo feature, so we don't unconditionally pull it in but rather only forcranelift-tools
.
cfallin created PR review comment:
We probably don't need the machine-code bytes alongside the instructions in the golden-compile tests (we test that separately in multiple ways, via assembler tests and end-to-end execution tests, and the point isn't to test Capstone's decoding of bytes to instructions).
elliottt submitted PR review.
elliottt created PR review comment:
Great point! The output would be a lot less noisy without that.
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
Perhaps we should not display the instruction address here? Inserting or deleting an instruction will renumber all the later instructions, causing more diff churn in filetests later.
Also, it's a small thing but I'd prefer to replace all these calls to
.unwrap()
with?
. Apparently that's straightforward since there's onewrite!
invocation in this function already using?
, so I expect it should be fine for the other writes too.I hope it's also easy for the call to
disasm_all
, where it would be particularly good to handle errors gracefully since we might very well have generated invalid instructions. I haven't checked if the error type returned from that function converts to ananyhow::Error
though.
elliottt submitted PR review.
elliottt created PR review comment:
I like that!
elliottt submitted PR review.
elliottt created PR review comment:
I like the output without the bytes, and thanks for catching the
unwrap()
uses! (I had pulled this directly over from clif-util, and hadn't taken the time to refactor it).
elliottt submitted PR review.
elliottt created PR review comment:
I like the address for making jumps easier to understand, but you're right that it'll likely cause a lot of churn when updating tests. Maybe this is a good case for using
clif-util compile
when debugging, vs making theprecise-output
easier to read?
elliottt updated PR #5780 from trevor/capstone-output-filetests
to main
.
elliottt updated PR #5780 from trevor/capstone-output-filetests
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
I removed the address as well; @jameysharp pointed out that it would make updates quite noisy as unrelated lines would change frequently.
elliottt submitted PR review.
elliottt created PR review comment:
I think it would be pretty easy to dump them out if they're present. I'll try that and we can see if that seems like enough information.
jameysharp submitted PR review.
jameysharp created PR review comment:
I'd propose two changes to this method:
- Return
capstone::Error
instead ofanyhow::Error
, since this is Capstone-specific anyway, and let the caller map the error however they need to.- Don't provide a default implementation (or provide one that returns
Err(capstone::Error::UnsupportedArch)
) and let the target-specific implementations figure out how to construct an appropriateCapstone
object. This is slightly annoying since the implementations all need to be guarded with#[cfg(feature = "disas")]
but I don't think that's a big deal.
elliottt updated PR #5780 from trevor/capstone-output-filetests
to main
.
elliottt updated PR #5780 from trevor/capstone-output-filetests
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
Much nicer, thanks!
elliottt updated PR #5780 from trevor/capstone-output-filetests
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
I've inlined the relocations in the disassembled output. Here's the updated version you originally referenced:
; push rbp ; mov rbp, rsp ; call 9 ; reloc_external CallPCRel4 u0:521 -4 ; mov rsp, rbp ; pop rbp ; ret
elliottt edited PR review comment.
elliottt requested jameysharp for a review on PR #5780.
elliottt requested cfallin for a review on PR #5780.
Last updated: Dec 23 2024 at 12:05 UTC