Stream: git-wasmtime

Topic: wasmtime / PR #5780 Use capstone to validate precise-outp...

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

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 23:30):

elliottt edited PR #5780 from trevor/capstone-output-filetests to main:

Use capstone when checking precise-output test expectations. This lets us continue to rely on printing backend-specific pseudoinstructions in the VCode output for debuggign purposes (for instance br_table on x64), while also checking the final output of the machinst buffer in filetests.

<!--

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 15 2023 at 00:17):

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

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:21):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:21):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:21):

cfallin created PR review comment:

Potential future improvement below (I don't want to necessarily block the PR on this!): the below disassembly logic is largely similar to MachBuffer's now, except that it prints bytes too, and doesn't include relocs, and currently has just a slice-of-u8 here; we could make those format differences options, wire through a MachBuffer somehow, and delegate to the common code here.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:34):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:34):

elliottt created PR review comment:

I like that, and would be happy to push that change through before merging this :+1:

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

I don't think it's possible to build an aarch64 TargetIsa with Architecture::Arm, is it? Same for x64 and X86_32. I can see that you're trying to preserve valid code that was in clif-util, but I think we should just delete these unreachable branches. When there's a 32-bit ARM or x86 backend it'll be easy enough to add these cases again in the appropriate TargetIsa.

I'm inclined not to even match on self.triple().architecture except if we need to know a specific architectural detail to configure the disassembler correctly. The arm.is_thumb() check is a good example of where that would be useful, but I'm proposing to delete that whole branch anyway.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

At this point I think cranelift-codegen only needs anyhow for disassemble, and it seems like a shame to pull it in just for that. Maybe disassemble should return capstone::Error as well.

Also perhaps this comment should mention CompiledCode::disassemble in addition to the existing mention of TargetIsa::to_capstone.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

I don't think cranelift-filetests needs to depend directly on capstone, now that you've moved everything important to cranelift-codegen. (Which turned out really well, I think: good call on putting disassemble there too!)

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

I think this tab is what's making the output... jittery?

Can we match the existing format instead? Looks like... no, because the x64 backend padded the mnemonic out to 7 characters, while the other backends I think all use a single space. I'm leaning toward a single space here.

If we print the space only when i.op_str() is Some, that'll reduce the filetests diff for instructions that don't have any operands, like ret.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

If I'm reading the documentation correctly you can just ask the instruction for its length, you don't need to get its bytes first.

            let end = i.address() + i.len() as u64;

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

It looks like the only use of params in this function is to borrow it, so you could make this argument a borrow as well. Then in run I think you can pass &comp_ctx.func.params instead of having to clone the params.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

I'm wondering if you can minimize the filetest diffs by setting the disassembly syntax. I think it's defaulting to Intel syntax and we previously mostly followed AT&T syntax, right? Looks like you can call builder.syntax(arch::x86::ArchSyntax::Att) to request that.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:44):

jameysharp created PR review comment:

The format string is optional in the writeln macro; if you just want a newline you can leave it out.

            writeln!(buf)?;

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:46):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:46):

elliottt created PR review comment:

I tried this and got a compilation error as len is private. Perhaps it's public in a newer version of capstone?

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:47):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:47):

elliottt created PR review comment:

Nice!

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:48):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:48):

jameysharp created PR review comment:

Ah, I was looking at the docs for 0.11 and we're using 0.9, which indeed didn't have it. Nevermind!

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:50):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:50):

elliottt created PR review comment:

Oh, there's a borrow checker error here if I don't clone :(

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:53):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:53):

jameysharp created PR review comment:

Huh, I may have misread something. Did you access the func through comp_ctx, where it's been moved?

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:53):

elliottt created PR review comment:

I like the switch back to AT&T, as it mirrors the MInst pretty-printing for x64.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:55):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:55):

elliottt created PR review comment:

Can capstone::Error wrap the error potentially raised by write!? That's currently the only thing holding back removing anyhow here.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:59):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 00:59):

elliottt created PR review comment:

I'm fine with a single space, though it's too bad we can't get the consistent alignment :)

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:01):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:01):

elliottt created PR review comment:

compile takes a mut self, so comp_ctx is no longer available at that point.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:02):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:02):

elliottt created PR review comment:

I misread my earlier error, it's not taking a mut self, but a &mut self. Let me investigate a bit.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:06):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:06):

jameysharp created PR review comment:

There's a capstone::Error::CustomError(&'static str) variant. Since there's no data attached to a fmt::Error (the Display impl currently always returns "an error occurred when formatting an argument") that would work. But yeah, I forgot about the write invocations.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:08):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:08):

elliottt created PR review comment:

Okay, it's not consumed by compile, but the mutable borrow lasts as long as compiled_code, which is causing the borrow error.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:10):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:10):

elliottt created PR review comment:

Good catch!

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:13):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:13):

elliottt created PR review comment:

I like that!

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:24):

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

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:32):

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

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:42):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 01:42):

elliottt created PR review comment:

This cleaned up 10k lines of diff! Thanks for the suggestion :D

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp created PR review comment:

Here are more mysterious disappearing blocks.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp created PR review comment:

Huh, why did this output disappear?

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp created PR review comment:

For pextrb/w/d we've been pretty-printing the destination register as the 64-bit %rax but it appears that it should have been the 32-bit %eax. Does that mean we've been leaving the high 32-bits uninitialized, and if so, is that an issue? I don't remember the x86-64 rules for when registers get silently zero-extended for you.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp created PR review comment:

Interesting, Capstone believes we printed the andn operands in reverse order. Could you double-check that and fix the pretty-printer if the disassembler is correct?

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp created PR review comment:

I feel like maybe the disassembler got confused here due to an inline constant. There doesn't appear to be a Capstone flag for skipping unknown instructions on s390, unlike for other targets. Maybe @uweigand can weigh in on whether this disassembly result is nonsense or is actually equivalent to the previous output.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:16):

jameysharp created PR review comment:

Now that params is a borrow, it doesn't need to be borrowed again here. I guess Rust auto-dereferenced it for you?

    let buf = compiled_code.buffer.disassemble(Some(params), &cs)?;

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:19):

cfallin created PR review comment:

The general rule on x86-64 is that instructions that write the low 32 bits of a 64-bit GPR clear the high 32 bits; I suspect this difference here is just one of notational convention (i.e., there's not actually a bit selecting rax vs eax that we're flipping).

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:35):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:35):

elliottt created PR review comment:

Thanks!

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:37):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:37):

elliottt created PR review comment:

I followed the intel manual when I added support for andn; I must have inadvertently implemented the intel formatting for the instruction :)

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

elliottt created PR review comment:

If I switch back to intel syntax and regenerate this test output, I see the following:

;   push rbp
;   mov rbp, rsp
;   andn eax, esi, edi
;   mov rsp, rbp
;   pop rbp
;   ret

I'll update the pretty-printer in a separate PR :+1:

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:50):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:50):

elliottt created PR review comment:

I enabled skipdata on s390x locally, and it fixed a lot of the missing test output. I'll push that change.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:51):

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

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:54):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:54):

elliottt created PR review comment:

This should be fixed after enabling skipdata in to_capstone for s390x.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:55):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:55):

elliottt created PR review comment:

This is fixed after enabling skipdata in to_capstone for s390x.

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:29):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:29):

elliottt created PR review comment:

https://github.com/bytecodealliance/wasmtime/pull/5789

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:34):

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

Wasmtime GitHub notifications bot (Feb 15 2023 at 19:18):

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

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

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

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

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

Wasmtime GitHub notifications bot (Feb 16 2023 at 00:20):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2023 at 00:20):

elliottt created PR review comment:

I looked into this, and it would be a bit more work than I'd like to tackle in this PR. I'll make a follow-up pr :+1:

Wasmtime GitHub notifications bot (Feb 16 2023 at 00:35):

elliottt merged PR #5780.

Last updated: Dec 23 2024 at 13:07 UTC