proposed work-around for https://github.com/rust-lang/rust/issues/86693 #358
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For 32-bit targets on RISC-V, we have encountered a problem in the non_adjacent_form() routine inside Scalar. To the best we can tell, there is a compiler bug which is causing 64-bit types on 32-bit platforms to not be properly aligned (see issue rust-lang/rust#86693). This bug results in the lower 32 bits of a 64-bit number being replicated into the top 32 bits, as a result of pointer arithmetic optimizations that rely on correct alignment of the data types. So far, we have only seen this problem inside one function that affects ed25519 verifications, but it could be in other libraries we haven't used yet. Thanks to @xobs we have a work-around, which is to wrap the `[u64; 5]` type inside an `AlignedU64Slice` `newtype` which is annotated with a `#[repr(align(32))]`. In our tests this causes the ed25519 signatures tests to pass on our platform. Unfortunately, we are unable to get the bug to express on x86 using a u32 backend; the bug seems to be specific to RISC-V. However, I think this patch is light-fingered enough that it might be worth considering absorbing into the crate. Based on the age of similar bugs filed against the Rust project, it may take some months or years even before this gets properly addressed...
|
I'm going to close this PR. Turns out the root cause is that $sp needs to be aligned to 16-byte boundaries for llvm to work correctly. Our runtime wasn't doing this. Now that this is done, we're running the stock code correctly. Sorry for the distraction! |
|
No problem! Glad you got it working and thanks for the thorough writeup on your investigations! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For 32-bit targets on RISC-V, we have encountered a problem in the
non_adjacent_form()routine insideScalar. To the best we can tell,there is a compiler bug which is causing 64-bit types on 32-bit platforms
to not be properly aligned (see issue rust-lang/rust#86693).
This bug results in the lower 32 bits of a 64-bit number being replicated
into the top 32 bits, as a result of pointer arithmetic optimizations that
rely on correct alignment of the data types.
So far, we have only seen this problem inside one function that affects
ed25519 verifications, but it could be in other libraries we haven't
used yet.
Thanks to @xobs we have a work-around, which is to wrap the
[u64; 5]type inside an
AlignedU64Slicenewtypewhich is annotated witha
#[repr(align(32))]. In our tests this causes the ed25519 signaturestests to pass on our platform.
Unfortunately, we are unable to get the bug to express on x86 using a u32
backend; the bug seems to be specific to RISC-V. However, I think this
patch is light-fingered enough that it might be worth considering
absorbing into the upstream crate. Based on the age of similar bugs filed against
the Rust project, it may take some months or years even before this issue
gets properly addressed...