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

Make basepoint table constants &'static references #488

Merged
merged 2 commits into from
Dec 28, 2022

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Dec 27, 2022

All current accesses to the basepoint tables currently use explicit borrowing.

It's good borrowing is used to prevent them from being copied onto the stack, however instead of requiring the caller to borrow the constant, instead the constant can simply be defined as a &'static reference.

Closes #486 (I think? cc @elichai)

@tarcieri tarcieri requested a review from rozbb December 27, 2022 15:14
@tarcieri tarcieri force-pushed the basepoint-tables-static-references branch 4 times, most recently from 618fcc3 to e42155b Compare December 27, 2022 15:23
@elichai
Copy link
Contributor

elichai commented Dec 27, 2022

According to the rust reference: https://doc.rust-lang.org/reference/items/constant-items.html

References to the same constant are not necessarily guaranteed to refer to the same memory address.

So the reference doesn't guarantee that the current implementation won't end up duplicating the table

And I'm not longer sure that even a const reference is enough: rust-lang/rust#72004

src/constants.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 27, 2022

Okay, I figured out how to make all of the basepoint tables static in d69e67c.

They're now all static as well as &'static references. Not sure if that's overkill or not.

@tarcieri tarcieri force-pushed the basepoint-tables-static-references branch from fe1bd6d to d0465a8 Compare December 27, 2022 17:43
src/backend/serial/u32/constants.rs Show resolved Hide resolved
src/backend/serial/u64/constants.rs Show resolved Hide resolved
src/constants.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri force-pushed the basepoint-tables-static-references branch 3 times, most recently from 8edee3c to 8716dcb Compare December 27, 2022 18:21
@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 27, 2022

Went ahead and squashed and split out the rustfmt changes into a separate commit.

There are <50 lines of actual changes in 59b4f6b 00df378 (rebased) but thanks to rustfmt it's over 9000 total.

This ensures they have a fixed address and aren't duplicated across
compilation units.

Since they were already always borrowed, this changes the static values
to be `&'static` addresses to ensure they're always borrowed rather than
potentially copied.
@tarcieri tarcieri force-pushed the basepoint-tables-static-references branch from 8716dcb to 3c6ed86 Compare December 27, 2022 21:13
@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 27, 2022

Phew, green again.

Sidebar: we might consider feature-gating these basepoint tables. Embedded users would probably appreciate that. Edit: opened #489

@rozbb
Copy link
Contributor

rozbb commented Dec 28, 2022

Looks good, thank you! This is how I checked the fmt step:

/t/curve25519-dalek (main)$ git diff 3c6ed86^..3c6ed86 | shasum -a 256
    42953ce3fdbb4f7297d5dfe2b1647977325047b68a8cc72b10e68adf534d4b46  -
/t/curve25519-dalek (main)$ git checkout 3c6ed86^
/t/curve25519-dalek ((00df3786…))$ cargo fmt
/t/curve25519-dalek ((00df3786…))$ git diff | shasum -a 256
    42953ce3fdbb4f7297d5dfe2b1647977325047b68a8cc72b10e68adf534d4b46  -

@rozbb rozbb merged commit 6a51f4f into main Dec 28, 2022
@tarcieri tarcieri deleted the basepoint-tables-static-references branch May 31, 2023 02:11
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.

Should the tables be static instead of const?
3 participants