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

YJIT: Allow testing assembler with disasm #7470

Merged
merged 6 commits into from Mar 14, 2023

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Mar 7, 2023

This PR proposes an alternative way to write assembler tests, rewriting tests of #7453 as a starter.

I don't really memorize how x86_64 instructions are encoded, so it's hard for me to read these test cases. Ideally, I'd like to test the generated code using disasm, assuming libcapstone is not broken. A challenge in it is that addresses in generated code are always different, so those values are replaced with start_addr-origin values.

@matzbot matzbot requested a review from a team March 7, 2023 23:08
@maximecb
Copy link
Contributor

maximecb commented Mar 8, 2023

This is clever, but I want to be strict about minimizing dependencies, even if they are used only during development. I would rather not merge this at this time.

@k0kubun
Copy link
Member Author

k0kubun commented Mar 8, 2023

How about this? 57a26a4 I removed new dependencies.

@maximecb
Copy link
Contributor

maximecb commented Mar 8, 2023

Still not a huge fan of adding a dev dependency or relying on the output of capstone and string processing 😅

Let's ask @XrXr what his opinion is when he is back next week.

@XrXr
Copy link
Member

XrXr commented Mar 13, 2023

I thought about doing something like this before and never got around to it. Thanks for looking into this! Being able to see the disassembly make these much easier to read.

In terms of dev dependency, I think we can make it test the disassembly optionally. So we would have something like:

assert_disasm!(
    cb,
    "488d5b08", /* checked when cargo test is built without features=disasm */,
    "lea rbx, [rbx + 8]..." /* additionally checked in `cargo test --all-features`, which we do on CI */
)

Some string manipulation to deindent is probably necessary, but maybe we can do less of it by not testing the addresses; I don't think the addresses are awfully important. I'm less concerned about Capstone because it seems to be reliable WRT always giving the same output.

@maximecb
Copy link
Contributor

I was on the fence, but since Alan is in favor, let's go forward with this 👍

@k0kubun
Copy link
Member Author

k0kubun commented Mar 13, 2023

Some string manipulation to deindent is probably necessary, but maybe we can do less of it by not testing the addresses; I don't think the addresses are awfully important. I'm less concerned about Capstone because it seems to be reliable WRT always giving the same output.

Ah, I had forgotten that we were passing addresses to disasm_all ourselves. If we pass 0 to it, we wouldn't need address manipulation there. I'll fix it so.

In terms of dev dependency, I think we can make it test the disassembly optionally. So we would have something like
additionally checked in cargo test --all-features, which we do on CI

👍

@k0kubun k0kubun force-pushed the yjit-test-disasm branch 2 times, most recently from cb8b053 to accc9fe Compare March 13, 2023 23:13
@k0kubun
Copy link
Member Author

k0kubun commented Mar 13, 2023

addressed Alan's points in ff6301b and 05c9865.

yjit/src/disasm.rs Outdated Show resolved Hide resolved
@maximecb maximecb merged commit 76f2031 into ruby:master Mar 14, 2023
96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants