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

POC: Retrieve the actual alignment of max_align_t #554

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Dec 4, 2022

The code had a hard-coded maximum alignment of 16 (128 bits) which may have potentially wasted memory and cause memory unsafety if it was ever compiled with larger alignment (unlikely).

This change implements a trick to retrieve the actual alignment from C. To achieve this a newly added C file defines a symbol initialized to _Alignof(max_align_t). It is then compiled (as a separate object), the contents of the symbol is extracted and converted to decimal string which is injected into the AlignedType definition. The definition is generated as a separate .rs file in OUT_DIR and included into types.rs.

This POC currently doesn't support cross-compilation which I intend to fix, I just wanted to show that it's possible to do this. Do we want to?

The code had a hard-coded maximum alignment of 16 (128 bits) which may
have potentially wasted memory and cause memory unsafety if it was ever
compiled with larger alignment (unlikely).

This change implements a trick to retrieve the actual alignment from C.
To achieve this a newly added C file defines a symbol initialized to
`_Alignof(max_align_t)`. It is then compiled (as a separate object), the
contents of the symbol is extracted and converted to decimal string
which is injected into the `AlignedType` definition. The definition is
generated as a separate `.rs` file in `OUT_DIR` and included into
`types.rs`.
@apoelstra
Copy link
Member

Hah, clever!

Do you think we could just make the C code printf the relevant value, then we run the binary and read it off of stdout? That would be trivially cross-platform (assuming people aren't compiling on embedded platforms) and easy to understand.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Dec 4, 2022

We can't because the binary would be produced for target (to have the correct max_align_t) and we need to run it at host which would be impossible without an emulator.

@elichai
Copy link
Member

elichai commented Jul 6, 2024

Side note, I don't remember what was the reason we decided not to use the libc crate, but it does expose the max_align_t that we can take the sizeof for creating the array.
https://docs.rs/libc/latest/libc/struct.max_align_t.html

@apoelstra
Copy link
Member

We stopped using libc because it routinely has 10000-line-diff minor revisions, and given that we only need like, 5 LOC from the entire project, it didn't seem worth the extra trust requirement.

As for this PR, I think it's worth resurrecting. It's pretty simple and I have some experience now doing something similar in rust-simplicity (though what we did there is much less clever; we just export the sizes from C and then use unit tests to check that they're the same as what we hardcoded in Rust -- not sufficient for rust-secp which is a much more mature project and cannot expect anybody to be running the unit tests before using it).

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 6, 2024

We stopped using libc because it routinely has 10000-line-diff minor revisions

That's nothing compared to another reason.

Click here if you want to be pissed off about `libc` even more

They recently silently changed MSRV one-past the upcoming Rust version in Debian stable...

As for this PR, I think it's worth resurrecting.

I'd love to but I don't remember what the issue with cross compilation was. Will have to dig into it.

Also I had an idea that for vendored version we could use the actual alignment of the struct, not the maximum.

@apoelstra
Copy link
Member

I don't think there was an actual issue, just that the hardcoded 16 is (at best) inefficient.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 6, 2024

IIRC for cross compiling we need different binutils and thus convert Rust toolchain name to the GNU one.

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.

3 participants