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

[Alternative2] Making sure everything is aligned correctly #235

Closed
wants to merge 4 commits into from

Conversation

elichai
Copy link
Member

@elichai elichai commented Aug 28, 2020

Another alternative for #233
Here the regular API is the same as #233 (uses alloc/dealloc) but the preallocated api is made unsafe and accepts a raw *mut c_void pointer, so that the caller needs to make sure he holds all the necessary invariants.

@real-or-random
Copy link
Collaborator

Do we really need AlignedType in this approach?

I think that's the cleanest. From the perspective of the caller, if you have some block of memory that you really want to use, then anyway not because the Rust allocator gave it to you with nice guarantees. So you're probably on your own anyway, and a safe API on our side will probably just move the problem somewhere else.

@elichai
Copy link
Member Author

elichai commented Aug 30, 2020

Hmm it isn't needed.
we need to expose what alignment this requires, we can do this via a const, or via a type that is aligned that way.
the downside of a const is that if something would want to stack allocate it, he couldn't do this:

#[repr(align(secp256k1::ALIGNMENT))] struct Aligned([u8; secp256k1::ALIGNMENT]);

because repr(align(N)) requires an int literal, so by providing it via a type (similar to max_align_t we also give users a tool to stack allocate the context)

@elichai
Copy link
Member Author

elichai commented Aug 30, 2020

actually, we could also just document we require it will be as aligned as libc::max_align_t, I wonder if we can make upstream commit to this somehow (even though upstream is C89 and max_align_ is C11) (altough I'd argue it already commits to that, because even in libc impls don't know if you use C89 or C11)

@real-or-random
Copy link
Collaborator

(altough I'd argue it already commits to that, because even in libc impls don't know if you use C89 or C11)

Yes, when I wrote the upstream docs I just quoted C89 but the requirements in C11 have changed for a reason. I think in fact we want what C11 specifies (largest alignment among all types with fundamental alignment requirements), it's just that the C89 standard is broken.

@apoelstra
Copy link
Member

Closed in favor of #233

@apoelstra apoelstra closed this Dec 21, 2020
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.

None yet

3 participants