Stream: git-wasmtime

Topic: wasmtime / PR #5780 Add disassembly to existing precise-o...

Wasmtime GitHub notifications bot (Feb 14 2023 at 20:52):

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.
-->

Wasmtime GitHub notifications bot (Feb 14 2023 at 20:54):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 20:54):

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?

Wasmtime GitHub notifications bot (Feb 14 2023 at 20:56):

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 instance br_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.
-->

Wasmtime GitHub notifications bot (Feb 14 2023 at 20:57):

elliottt has marked PR #5780 as ready for review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:01):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:01):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:01):

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?

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:01):

cfallin created PR review comment:

We could put a .to_capstone() method on TargetIsa and enable it only under a capstone Cargo feature, so we don't unconditionally pull it in but rather only for cranelift-tools.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:01):

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).

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:03):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:03):

elliottt created PR review comment:

Great point! The output would be a lot less noisy without that.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:05):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:05):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:05):

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 one write! 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 an anyhow::Error though.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:06):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:06):

elliottt created PR review comment:

I like that!

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:09):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:09):

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).

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:27):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:27):

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 the precise-output easier to read?

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:30):

elliottt updated PR #5780 from trevor/capstone-output-filetests to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:31):

elliottt updated PR #5780 from trevor/capstone-output-filetests to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:35):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:35):

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.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:38):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:38):

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.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:53):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:53):

jameysharp created PR review comment:

I'd propose two changes to this method:

• Return capstone::Error instead of anyhow::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 appropriate Capstone 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.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:12):

elliottt updated PR #5780 from trevor/capstone-output-filetests to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:15):

elliottt updated PR #5780 from trevor/capstone-output-filetests to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:17):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:17):

elliottt created PR review comment:

Much nicer, thanks!

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:21):

elliottt updated PR #5780 from trevor/capstone-output-filetests to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:28):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:28):

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

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:29):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:29):

elliottt requested jameysharp for a review on PR #5780.

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:29):

elliottt requested cfallin for a review on PR #5780.

Last updated: Nov 22 2024 at 16:03 UTC