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

cmov: XOR within the ASM block #925

Merged
merged 13 commits into from
Oct 3, 2023
Merged

Conversation

brxken128
Copy link
Contributor

@brxken128 brxken128 commented Jul 5, 2023

Now that #924 is a thing, we should be able to viably use XOR within the ASM block as opposed to within Rust.

This closes #920.

@tarcieri
Copy link
Member

Looks like a legit test failure?

@tarcieri
Copy link
Member

The test failure seems legitimate?

@brxken128
Copy link
Contributor Author

brxken128 commented Aug 17, 2023

The test failure seems legitimate?

I'm not too sure. It's only happening for u16 and u128, which is really odd. I've tried different register modifiers for all of the different types but will keep testing until I eventually get it working. I do believe u64 needs :r though. I had a thread asking about it in the Rust Discord server and someone extremely helpfully pointed out that xor does actually produce an output, so our second variable needed to be an inout(reg) value => _ to be clobbered.

Maybe my testing methodology is flawed, I'll try changing some things up there too. Everything (in theory) should be working just fine.

@brxken128
Copy link
Contributor Author

brxken128 commented Aug 17, 2023

I tested locally on my R9 7950X, forcing it to use the x86 module (by commenting the others out - apparently miri was enabled?) Anyway, the tests complete just fine.

I even temporarily removed #[no_std] so I could use dbg!() and ensure that the x86 module was being used, and it indeed was. I wonder what's causing CI to continuously fail.

Bit of an odd way to prove it but:
image

@brxken128
Copy link
Contributor Author

Further oddness - cargo test (--release) passes just fine, but cargo nextest run (--release) fails on:

FAIL [   0.003s] cmov::lib u128::cmoveq_works
FAIL [   0.003s] cmov::lib u16::cmoveq_works
FAIL [   0.003s] cmov::lib u16::cmovne_works

This was performed by forcing the x86 backend (by commenting the others out completely). AFAICT the code works just fine? Very odd.

@brxken128
Copy link
Contributor Author

brxken128 commented Aug 18, 2023

Would it be possible for anyone else to give this a shot on x86_64? please? @tarcieri maybe?

Quick command: git clone https://github.com/brxken128/utils utils-testing && cd utils-testing/cmov && git checkout x86-xor && cargo test and then (if installed) try cargo nextest run. Nexttest seems to fail and I'm unsure why.

@brxken128
Copy link
Contributor Author

brxken128 commented Aug 18, 2023

I fixed the issue thanks to a very helpful person in the Rust Discord server. It turns out that :e and 16 bit values don't mix when you're trying to compare the two, as :e is 32-bit and can/will contain garbage data in the upper-half.

This meant that the XOR was always inconsistent as we only wanted to compare the lower half of the register, so by using :x (which is 16 bit) we compare only the actual values, and not the "upper half", potentially garbage data.

We can still use :e in the cmov* calls as they only move what we provide in the first place (I believe?).

I am very grateful for the Rust Discord server, and this has helped me learn a lot - and helped this pull request become merge-ready.

@brxken128
Copy link
Contributor Author

cc @tarcieri

@tarcieri tarcieri merged commit 582883a into RustCrypto:master Oct 3, 2023
15 checks passed
@brxken128 brxken128 deleted the x86-xor branch October 4, 2023 01:40
@tarcieri tarcieri mentioned this pull request Oct 14, 2023
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.

cmov: use ASM XOR for equality comparisons
2 participants