Pre-allocation and alignment

[laanwj]

Secp256k1::preallocated_*(buf) takes an arbitrary mutable slice of u8 as buffer; however it seems that, as it will be used to store pointers, it needs to be aligned to at least the pointer size of the platform.

I'm using a static buffer in my embedded project, and I've had it crash on ret->illegal_callback = default_illegal_callback in secp256k1_context_preallocated_create for no apparent reason until I discovered this requirement.

• A possible solution would be to take a *mut [u64] or *mut [usize] instead. This at least makes sure it's aligned enough to accept pointers
• Another one would be to accept buffers with arbitrary alignments, but make manual_alloc align the pointer (currently, it only aligns the size)
• Alternatively, let's add an assert! and at least document this. I now see it's documented at the C side:

/** Create a secp256k1 context object in caller-provided memory.
 *
 *  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.

[elichai]

1. Could you point me to the source of however it seems that, as it will be used to store pointers, it needs to be aligned to at least the pointer size of the platform.?

2. If we manually align the pointer we need to take it into account in preallocate_*_size().

3. I see that the C docs say suitably aligned to hold an object of any type, what does "any type" mean? @apoelstra

[laanwj]

@elichai it happens here https://github.com/bitcoin-core/secp256k1/blob/master/src/secp256k1.c#L117 (note that manual_alloc rounds up the size to the required alignment, but not the pointer itself)

[apoelstra]

I think *mut [usize] may do the right thing for C, though e.g. https://rust-lang.github.io/unsafe-code-guidelines/layout/pointers.html hints that things may have a larger alignment than usize.

Note that upstream libsecp uses the constant 16.

[elichai]

@apoelstra Yeah I'm not sure about Any type of object being the alignment of usize, could we assume that this is what is meant by the function contract and wouldn't break in the future?

[apoelstra]

No, we cannot assume that :)

[laanwj]

Right, it's impossible to guarantee it for all architectures and instruction sets. I remember some SIMD primitives on x86 at least require 16-byte alignment. Maybe the compiler might start generating them in the future. I don't know!
In my specific case (rv64) 1-byte alignment was too little, but 8-byte alignment was enough. Requiring 16-byte wouldn't be significantly wasteful.

[elichai]

@laanwj we can definitely enforce 16 byte alignment (https://play.rust-lang.org/?gist=0f30929ccbc1acd321776418ed8861b2) the question is, is this the correct alignment to do and if not then which is.

[apoelstra]

Yeah, let's just copy 16 from upstream.

[elichai]

Ok, I'll do it later today, (with a repr(16) type of our own).

[real-or-random]

repr(align(16)) needs Rust 1.25 and we currently aim to support Rust 1.22. But our reason for Rust 1.22 is Debian stable, which has moved to 1.34 (as far as I can tell from https://packages.debian.org/buster/rustc). So I suggest raise the minimum to 1.34.

Just a note, doesn't matter here: Upstream first uses __BIGGEST_ALIGNMENT__ if that is defined and 16 only as a fallback.

[real-or-random]

repr(align(16)) needs Rust 1.25 and we currently aim to support Rust 1.22. But our reason for Rust 1.22 is Debian stable, which has moved to 1.34 (as far as I can tell from https://packages.debian.org/buster/rustc). So I suggest raise the minimum to 1.34.

If we don't want to do this, here's an ugly hack:
https://stackoverflow.com/a/32429078

But I'd prefer to raise the minimum Rust version if this doesn't imply other issues.

[elichai]

Yeah, I just tested a bunch of reprs and the only ones that work in 1.22 are C,Packed,u*

[elichai]

Personally I would love to use 1.34, we could do a bunch of nice things with it.
(like some const fn stuff)

Otherwise maybe 8 alignment is enough? (align_of) could we somehow add a check in upstream for this that will at least call the callbacks if the alignment is bad? (so we won't trigger any UB)

[real-or-random]

Otherwise maybe 8 alignment is enough? (align_of) could we somehow add a check in upstream for this that will at least call the callbacks if the alignment is bad? (so we won't trigger any UB)

I didn't include this check upstream because there is no way to check alignment in C. The only thing you can do is to 1) cast the pointer to an integer and 2) check that resulting is divisible by the desired alignment but the result of 1) is implementation-defined. I except it to be reasonable on all reasonable platforms though...

[apoelstra]

@real-or-random our reason for using 1.22 is to support building with mrustc (which currently supports only 1.19, so there are a couple layers of bootstrapping required). Every version after 1.19 adds work to the bootstrap; I forget what feature 1.22 had that made us consider it worth the tradeoff, but 1.34 is definitely a no.

Also, in general, we can't be increasing the version all the time. 1.34 is not even 4 months old. It is unreasonable to demand our users update their toolchain every 4 months.

[elichai]

@apoelstra Good point about the layers. https://guix.gnu.org/blog/2018/bootstrapping-rust/

Not fun to need to compile 17 times to get to 1.34

[real-or-random]

@apoelstra lol sorry indeed, I had already forgotten about that. Maybe we should add this rationale to the README...

Also, in general, we can't be increasing the version all the time. 1.34 is not even 4 months old. It is unreasonable to demand our users update their toolchain every 4 months.

Yeah I don't know to be honest. We should have proper guidelines to avoid having this discussing every few month. Debian stable was a rationale once. The best I can come up with after this discussion is "not too far from mrustc's target, supported on Debian stable, and definitively older than 4 months". This is a pretty imprecise and unsatisfactory answer, in particular since we don't know what will happen happen to mrustc, which is already considering to patch rust (thepowersgang/mrustc#95 (comment)).

[apoelstra]

We can have a discussion about 1.29 when it's mrustc-compatible, which probably center around how old it is (I agree, we ought to have some concrete criteria for that). But after that we have to have a discussion about Rust 2018, and I don't see the value in trying to nail down a specific policy until we've sorted that out.

[elichai]

Yes. Rust 2018 is an interesting one, it might come with a big refactoring of the code (hopefully we could use the opportunity to discard a bunch of the old stuff and add some clippy/rustfmt thingy)
But I think it's still in the far future, as the official release version of rust 2018 is 1.31.0 (https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1310-2018-12-06)

[elichai]

If we do want to realign ourselves:
https://doc.rust-lang.org/std/primitive.slice.html#method.align_to
https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset