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

Making sure everything is aligned correctly. Succeeder of #141 #233

Merged
merged 4 commits into from Dec 21, 2020

Conversation

elichai
Copy link
Member

@elichai elichai commented Aug 28, 2020

Introduction

libsecp256k1 expects thing to be "suitably aligned to hold an object of any type"[0] which is the same jargon C89 uses for malloc:

The pointer returned if the allocation succeeds is suitably aligned so that it may be assigned to a pointer to any type of object and then used to access such an object or an array of such objects in the space allocated(until the space is explicitly deallocated). [1]

which translated to C11 jargon means "suitably aligned for any fundamental type" :

The pointer returned if the allocation succeeds is suitably aligned so that it may be assigned to a pointer to any type of object with a fundamental alignment requirement and then used to access such an object or an array of such objects in the space allocated (until the space is explicitly deallocated). [2]

Which is:

A fundamental alignment is represented by an alignment less than or equal to the greatest alignment supported by the implementation in all contexts, which is equal to alignof(max_align_t). [3]

Rust holds a table of all these alignments for the supported platforms: https://github.com/rust-lang/rust/blob/2c31b45ae878b821975c4ebd94cc1e49f6073fd0/library/std/src/sys_common/alloc.rs
And rust-lang/libc implements max_align_t directly: https://docs.rs/libc/0.2.76/libc/struct.max_align_t.html

So I took the bigger alignment any rust architecture supports and used that as our default alignment, and also added a test that it is bigger than max_align_t (we could also test that in the build.rs if we really want).

Implementation

Use alloc() and dealloc() whenever we allocate, with 16 as alignment.
And use #[repr(align(16))] struct AlignType([u8; 16]) for the preallocated buffer API.

Alternatives

Closes #138
[0] https://github.com/bitcoin-core/secp256k1/blob/670cdd3f8be25f81472b2d16dcd228b0d24a5c45/include/secp256k1_preallocated.h#L42
[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 7.20.3
[2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf 7.22.3
[3] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf 6.2.8p2

@elichai
Copy link
Member Author

elichai commented Dec 18, 2020

Rebased

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack 0638107

@apoelstra apoelstra merged commit 3151352 into rust-bitcoin:master Dec 21, 2020
@apoelstra apoelstra mentioned this pull request Dec 23, 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.

Pre-allocation and alignment
2 participants