Add AlignType and use it for buffer allocations #141

elichai · 2019-08-08T15:48:49Z

This addresses #138
In rust 1.22, we cannot get any type with alignment bigger than 8.
so we take u64 for any platform that is lower than 64bit and usize for any platform that is u64 and up.

I'm not entirely sure if the assertions are needed, I mostly use them as sanity checks and I hope that they're optimized out by the compiler(they can be easily checked at compile time), but if anyone feel differently I can remove some/all of them.

laanwj · 2019-08-08T15:54:12Z

src/lib.rs

@@ -572,7 +574,15 @@ impl<C: Context> Secp256k1<C> {

    /// Returns the required memory for a preallocated context buffer in a generic manner(sign/verify/all)


please add comment that this returns the size in AlignType units

(also for the specific methods Secp256k1::preallocate_size_XX(), otherwise someone might think they return bytes!)

Good Point!

I think maybe they should return bytes.

src/lib.rs

elichai · 2019-08-08T16:15:29Z

(I might split the commit into code, and tests+doc)

real-or-random

I think 8 may hurt us, libsecp256k1 uses uint128_t on gcc, which has alignment 16 on x86_64 for example:
https://godbolt.org/z/6-TVaY

edit: Oh I should have opened IRC earlier:

<wumpus> secp256k1 never stores 128 bit integers though
<wumpus> (it uses it for multiplication in one place, but only on the stack)

That's true, nevermind.

src/context.rs

real-or-random · 2019-08-08T17:10:35Z

src/context.rs

+    /// ```rust
+    /// # use secp256k1::*;
+    /// let buf_size = Secp256k1::preallocate_size();
+    /// let mut buf = vec![0; buf_size];


0 as AlignType?

rust automatically assign the right type because it's then passed to Secp256k1::preallocated_new which accepts [AlignType]

elichai · 2019-08-08T17:42:25Z

Ha, I don't like the fact that gcc's uint128_t alignment is 16. it means that the second upstream writes it to context then we have a problem.
It's also so weird to me that clang and rust use different alignments for a uint128 clang: https://godbolt.org/z/WyzeRz rust: https://godbolt.org/z/hGjfLQ

real-or-random · 2019-08-08T18:28:05Z

It's also so weird to me that clang and rust use different alignments for a uint128 clang: https://godbolt.org/z/WyzeRz rust: https://godbolt.org/z/eJqq12

I think that's expected. I guess usize is u64 on that system?

real-or-random · 2019-08-08T18:32:17Z

src/context.rs

-    /// Uses the ffi `secp256k1_context_preallocated_size` to check the memory size needed for the context
+    /// Returns the required memory for a preallocated context buffer in a generic manner(sign/verify/all)
+    /// 
+    /// Notice that the memory returned is in [AlignedType](type.AlignType.html)


s/AlignedType/AlignType

To be honest, I don't like that name too much anyway. But I don't have better suggestions. PreallocatedMemory? Maybe better but not great either.

elichai · 2019-08-08T19:03:28Z

It's also so weird to me that clang and rust use different alignments for a uint128 clang: godbolt.org/z/WyzeRz rust: godbolt.org/z/eJqq12

I think that's expected. I guess usize is u64 on that system?

Ops, I meant to do https://godbolt.org/z/hGjfLQ

elichai · 2019-08-08T19:07:08Z

#142 Is a blocker to making the tests here pass.
I have no idea why only now this problem had come up, maybe a new rust compiler version changed the behavior of ZST in emscripten (which is allowed).

elichai · 2019-08-22T00:26:29Z

Now that #142 is solved the tests pass here :)

RalfJung · 2019-09-16T20:55:30Z

It's also so weird to me that clang and rust use different alignments for a uint128 clang

Due to an LVLM bug, i128/u128 in Rust are currently not FFI-safe. So, you cannot use these types when doing Rust <-> C FFI. See rust-lang/rust#54341 for further details.

The "FFI safety" lint should thus warn about i128/u128 on FFI boundaries, does it not?

RalfJung · 2019-09-16T20:58:21Z

src/context.rs

+/// Trying to match what `malloc` does in C, this should be aligned enough to contain pointers too.
+/// This type can have different size/alignment depending on the architecture.
+#[cfg(any(target_pointer_width = "32", target_pointer_width = "16", target_pointer_width = "8"))]
+pub type AlignType = u64;


Uh, based on what documentation are you assuming that this is a type with "the biggest alignment we can get"? I don't think such a type can even exist, since we can do things like

#[repr(align(256))] struct A(u8);

This type is 256-aligned.

I am not sure what you are trying to do here, but it seems extremely fishy.

I'll start with the purpose and then with "the biggest".
The purpose is creating a pointer that is valid for a C function contract that requires a pointer from 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 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).

Now malloc doesn't define what is this. and currently pointers in 64bit architecture are u64 so we know that we need at least this much alignment because the C library does write pointers into that memory.
The only thing currently that can align more than that is __uint128_t which is 16 bytes alignment.

Now to why this value was chosen.
we support rust 1.22.0 here (last point: https://github.com/rust-bitcoin/rust-secp256k1#contributing) so we can't use #[repr(align(N))], and even if we could i'm not sure what would be the right value to put there.
but currently as we don't have support for that, and we know that we need at most 8 bytes alignment we use u64 for alignment.
I agree that's not a good solution, but because rust will never have max_align_t and we don't have access to C's max alignment (as it's not defined AFAIK), so currently the only thing we can do is maintain an alignment as big as the biggest primitive's alignment that we can check.

Okay, so what you really want is C's alignof(max_align_t)? The Rust libstd has a table for this that I copied into Miri at some point. I don't think it agrees with the value you have been using here, though. In particular, on 64bit x86, max_align_t is 16-byte aligned.

However, how far does this "requires a pointer from malloc" go? Does the pointer have to be freeable with free? If yes, you have to call malloc; the pointer you get from Rust's alloactor might or might not come from malloc. Even if no, it might be easiest to just call malloc?

@gnzlbg does libc expose max_align_t, or is there some other way to get that information though libc?

I am still confused why you need this as a type though, as opposed to just having to know the alignment and then using that (together with the size, which I assume is supplied separately) when allocating.

@gnzlbg does libc expose max_align_t, or is there some other way to get that information though libc?

Not yet but this seems a reasonable thing to implement.

I am still confused why you need this as a type though, as opposed to just having to know the alignment and then using that (together with the size, which I assume is supplied separately) when allocating.

I suppose that people wanted to be able to use C's alignof(<type>) to compute the maximing alignment, so instead of creating a MAX_ALIGN constant, they decided to over engineer it and make it a type.

If we expose this from libc, in Rust, you can just do const MAX_ALIGN: usize = align_of::<libc::max_align_t>(); and use that. In jemalloc-sys we use a hardcoded MAX_ALIGN constant for that.

@RalfJung the exact definition from upstream is:

The caller must provide a pointer to a rewritable contiguous block of memory of size at least secp256k1_context_preallocated_size(flags) bytes, suitably aligned to hold an object of any type.

So being able to pass it to free() isn't a requirement, being aligned as max is.
https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1_preallocated.h#L41

is it possible to achieve alignment 16 without repr(align(N))?

is it possible to achieve alignment 16 without repr(align(N))?

core::arch::...::__m128 have alignment of 16.

the exact definition from upstream is

I see. "object of any type" is certainly not true even in C with compiler extensions, as C also has a way to increase alignment similar to #[repr(align)] in Rust. So probably upstream means max_align_t but it might be a good idea to ask them to clarify their docs.

So, why do you need a type that has that alignment, as opposed to just using alloc::alloc? Is Rust 1.28 (more than a year old at this point!) too "new"?

EDIT: Ah yes, you mentioned Rust 1.22 above (released in 2017, wow). Well I'm afraid if you are restricting yourself to support ancient unsupported versions of Rust, things might become messy. I think it's a mistake to not use a better version when it exists, this is holding back the ecosystem... but that's up to you. I just don't have a lot of motivation to review code that is awful for entirely self-inflicted reasons.

EDIT2: Sorry for venting, you probably didn't even come up with this policy. It's just frustrating to see how policies I consider misguided hold back language development.

"object of any type" is certainly not true even in C with compiler extensions,

That's because that's incorrect. The requirement is that the allocation must be suited for any type with fundamental alignment, where "fundamental" means that you did not use anything like repr(align()) anywhere on it, among other things.

If you use repr(align()) on it in such a way that the alignment of the type is larger than the largest alignment of such a "fundamental" type, then you can't use malloc to allocate its memory, and need to use an API that accepts an alignment instead.

You're right when it comes to modern C.
But upstream uses C89, so there's no max_align_t and no _Alignas, and AFAIK there's no sense of "fundamental type".
But it still may be confusing so i'll ask upstream what people think. Thanks.

elichai · 2020-08-24T10:52:56Z

I think we should postpone this until we bump MSRV and then replace the AlignType typedef with a struct of size+align of 16, which aligns with https://github.com/rust-lang/rust/blob/2c31b45ae878b821975c4ebd94cc1e49f6073fd0/library/std/src/sys_common/alloc.rs
and we can test it against the alignment of https://docs.rs/libc/0.2.76/libc/struct.max_align_t.html

apoelstra · 2020-08-27T16:36:41Z

Now that our MSRV is 1.29 I think we can do this.

real-or-random · 2020-08-28T09:36:21Z

With 1.29, I think all of this can be done much simpler using https://doc.rust-lang.org/std/alloc/fn.alloc.html and https://doc.rust-lang.org/std/alloc/struct.Layout.html#method.from_size_align. Then we're responsible for deallocation but we don't need to create artifical types to abuse Vec to do the right thing.

elichai · 2020-08-28T09:39:49Z

With 1.29, I think all of this can be done much simpler using https://doc.rust-lang.org/std/alloc/fn.alloc.html and https://doc.rust-lang.org/std/alloc/struct.Layout.html#method.from_size_align. Then we're responsible for deallocation but we don't need to create artifical types to abuse Vec to do the right thing.

Yep. I already started working on a few alternatives, the main "problem" is the preallocated_*(&mut [u8]) functions
where we'll need to switch [u8] with something more aligned and store it.
(if we bumped to 1.30 we could've done something like this https://doc.rust-lang.org/std/primitive.slice.html#method.align_to but it could also cause problems in actually knowing what size you need beforehand)

real-or-random · 2020-08-28T09:53:08Z

the main "problem" is the preallocated_*(&mut [u8]) functions
where we'll need to switch [u8] with something more aligned and store it.

Uff, I wasn't aware that we offer these functions. Aren't these terribly broken even now if you pass an unaliged buffer there?

The docs don't even say anything about alignment.

elichai · 2020-12-09T21:22:56Z

succeeded by #233

Making sure everything is aligned correctly. Succeeder of #141

laanwj reviewed Aug 8, 2019

View reviewed changes

apoelstra reviewed Aug 8, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

elichai force-pushed the 2019-08-16-alignment branch from 93b3a49 to cca6642 Compare August 8, 2019 16:15

real-or-random reviewed Aug 8, 2019

View reviewed changes

src/context.rs Outdated Show resolved Hide resolved

src/context.rs Outdated Show resolved Hide resolved

real-or-random reviewed Aug 8, 2019

View reviewed changes

elichai force-pushed the 2019-08-16-alignment branch 2 times, most recently from 8477d9f to 07460fd Compare August 8, 2019 17:59

real-or-random reviewed Aug 8, 2019

View reviewed changes

elichai force-pushed the 2019-08-16-alignment branch from 07460fd to e8a1513 Compare August 8, 2019 19:04

elichai force-pushed the 2019-08-16-alignment branch from e8a1513 to 07b7e74 Compare August 22, 2019 00:11

elichai mentioned this pull request Sep 16, 2019

Handle ZST rust-random/getrandom#104

Merged

RalfJung reviewed Sep 16, 2019

View reviewed changes

elichai force-pushed the 2019-08-16-alignment branch from 07b7e74 to 6e399ce Compare February 12, 2020 15:11

elichai force-pushed the 2019-08-16-alignment branch from 6e399ce to 40737a6 Compare April 29, 2020 19:53

Add AlignType and use it for buffer allocations

84ebaa0

elichai force-pushed the 2019-08-16-alignment branch from 40737a6 to 43b4906 Compare April 29, 2020 19:55

Add docs for AlignType and preallocation, and update preallocate tests

12be8fa

elichai force-pushed the 2019-08-16-alignment branch from 43b4906 to 12be8fa Compare April 30, 2020 07:44

elichai closed this Dec 9, 2020

apoelstra added a commit that referenced this pull request Dec 21, 2020

Merge pull request #233 from elichai/alloc-AlignedType2

3151352

Making sure everything is aligned correctly. Succeeder of #141

elichai mentioned this pull request Mar 8, 2022

Undocumented alignment of secp256k1_context bitcoin-core/secp256k1#1082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AlignType and use it for buffer allocations #141

Add AlignType and use it for buffer allocations #141

elichai commented Aug 8, 2019

laanwj Aug 8, 2019 •

edited

Loading

elichai Aug 8, 2019

apoelstra Aug 8, 2019

elichai Aug 8, 2019

elichai commented Aug 8, 2019

real-or-random left a comment •

edited

Loading

real-or-random Aug 8, 2019 •

edited

Loading

elichai Aug 8, 2019

elichai commented Aug 8, 2019 •

edited

Loading

real-or-random commented Aug 8, 2019

real-or-random Aug 8, 2019

elichai commented Aug 8, 2019

elichai commented Aug 8, 2019

elichai commented Aug 22, 2019

RalfJung commented Sep 16, 2019

RalfJung Sep 16, 2019

elichai Sep 16, 2019 •

edited

Loading

RalfJung Sep 17, 2019

gnzlbg Sep 17, 2019

gnzlbg Sep 17, 2019

elichai Sep 17, 2019 •

edited

Loading

gnzlbg Sep 17, 2019

RalfJung Sep 17, 2019 •

edited

Loading

gnzlbg Sep 17, 2019

elichai Sep 18, 2019

elichai commented Aug 24, 2020

apoelstra commented Aug 27, 2020

real-or-random commented Aug 28, 2020

elichai commented Aug 28, 2020

real-or-random commented Aug 28, 2020 •

edited

Loading

elichai commented Dec 9, 2020

		@@ -572,7 +574,15 @@ impl<C: Context> Secp256k1<C> {

		/// Returns the required memory for a preallocated context buffer in a generic manner(sign/verify/all)

Add AlignType and use it for buffer allocations #141

Add AlignType and use it for buffer allocations #141

Conversation

elichai commented Aug 8, 2019

laanwj Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Aug 8, 2019

real-or-random left a comment • edited Loading

Choose a reason for hiding this comment

real-or-random Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Aug 8, 2019 • edited Loading

real-or-random commented Aug 8, 2019

Choose a reason for hiding this comment

elichai commented Aug 8, 2019

elichai commented Aug 8, 2019

elichai commented Aug 22, 2019

RalfJung commented Sep 16, 2019

Choose a reason for hiding this comment

elichai Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Aug 24, 2020

apoelstra commented Aug 27, 2020

real-or-random commented Aug 28, 2020

elichai commented Aug 28, 2020

real-or-random commented Aug 28, 2020 • edited Loading

elichai commented Dec 9, 2020

laanwj Aug 8, 2019 •

edited

Loading

real-or-random left a comment •

edited

Loading

real-or-random Aug 8, 2019 •

edited

Loading

elichai commented Aug 8, 2019 •

edited

Loading

elichai Sep 16, 2019 •

edited

Loading

elichai Sep 17, 2019 •

edited

Loading

RalfJung Sep 17, 2019 •

edited

Loading

real-or-random commented Aug 28, 2020 •

edited

Loading