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

Numerous unused variable cause Rust difficulties #1034

Closed
coder0xff opened this issue Mar 12, 2024 · 3 comments
Closed

Numerous unused variable cause Rust difficulties #1034

coder0xff opened this issue Mar 12, 2024 · 3 comments

Comments

@coder0xff
Copy link
Contributor

This issue is related to rust-lang/rust#122357. Large numbers of unused variables in fiat_p384_scalar_mul and possibly others are causing stack overflow in the Rust compiler (LLVM) on some targets. While the code in fiat_p384_scalar_mul is certainly valid Rust code, and the fault lies with the compiler, trimming dead code should alleviate this issue while a proper fix is pending. I will open a PR shortly.

@tarcieri
Copy link
Member

Note that the code in those backends is autogenerated by fiat-crypto. An ideal fix would go into their Rust code generation backend: https://github.com/mit-plv/fiat-crypto

We do also have a tool for applying automated transformations to their output where you could also potentially apply a fix: https://github.com/RustCrypto/utils/tree/master/fiat-constify

I would definitely suggest against any hand edits to these files, as a big part of their value is formal verification.

@coder0xff
Copy link
Contributor Author

Looking at https://github.com/mit-plv/fiat-crypto/blob/master/fiat-rust/src/p384_32.rs#L167 , since it's not a const fn and uses the mutable variables it declares, I don't think that warrants change, unless of course they would be changed to const fn. That would be an improvement, but probably harder to justify. I'll take a pass at modifying updating fiat-constify to strip the unused variables.

@tarcieri
Copy link
Member

Sounds good!

Yeah, fiat-crypto itself should probably wait for const_mut_refs to add const fn support, since it does incur a small performance penalty.

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

No branches or pull requests

2 participants