Implemented context create/destroy in rust #130
Conversation
|
Nice :) If only they'd define "word size".. |
|
Oh, I see, they define it as the size of |
|
do you mean |
|
Yep. After talking on IRC, I think based on https://doc.rust-lang.org/std/primitive.usize.html this assertion should never fail even on esorteric architectures, but I think it's best to have it anyway. |
|
Added, And tested rust-bitcoinconsensus with this and it no longer complains about the missing symbols. |
|
This works. It makes rust-bitcoinconsensus' tests pass. |
|
@elichai can you add a commit which bumps the minor version number and adds a |
src/ffi.rs
Outdated
| /// The pointer shouldn't be used after passing to this function, consider it as passing it to `free()`. | ||
| /// | ||
| pub unsafe extern "C" fn secp256k1_context_destroy(ctx: *mut Context) { | ||
| assert_eq!(ctx as usize % mem::align_of::<usize>(), 0); |
There was a problem hiding this comment.
I think this assertion should be dropped; it's not obvious to me that alignment should be preserved across casts to usize like this and ultimately if the user gives us a pointer here that we didn't create then it's UB anyway.
There was a problem hiding this comment.
shouldn't it hopefully be the same pointer that was created by secp256k1_context_create?
I think of this assertion as a fail safe to notice us if something went really wrong on some weird system. but I guess it can be removed
There was a problem hiding this comment.
There exist systems where the low-order bits of pointers are used for weird accounting stuff, so the alignment of the pointer will not correspond to the "alignment" of the result of casting to usize.
There was a problem hiding this comment.
Weird. people here suggested this to be the right way to check alignment rust-lang/rust#42561
src/ffi.rs
Outdated
|
|
||
| let size_ptr: *mut usize = ctx.offset(-1); | ||
| let size: usize = ::core::ptr::read(size_ptr); | ||
| let slice: &mut [usize] = slice::from_raw_parts_mut(size_ptr , size+1); |
There was a problem hiding this comment.
Please rename size to n_words and drop the + 1
There was a problem hiding this comment.
The +1 is important because that size doesn't account for itself.
and if we want the memory to be dropped we need the box to be reconstructed.
although I'm wondering now if the ptr::read() that memory be dropped afterwards
|
can you also squash the division commit into the original commit, since both commits are needed for the code to be correct? |
|
Passes valgrind and leak sanitizer |
| let word_size = mem::size_of::<usize>(); | ||
| let n_words = (secp256k1_context_preallocated_size(flags) + word_size - 1) / word_size; | ||
|
|
||
| let buf = vec![0usize; n_words + 1].into_boxed_slice(); |
There was a problem hiding this comment.
I don't think you need this +1.
There was a problem hiding this comment.
Oh, derp, the +1 is the extra word for the length.
| // Returns: a newly created context object. | ||
| // In: flags: which parts of the context to initialize. | ||
| pub unsafe extern "C" fn secp256k1_context_create(flags: c_uint) -> *mut Context { | ||
| assert!(mem::align_of::<usize>() >= mem::align_of::<u8>()); |
There was a problem hiding this comment.
Heh, in light of our recent alignment discussion I don't think this assertion provides any value. It's better to add a comment saying that https://rust-lang.github.io/unsafe-code-guidelines/layout/pointers.html guarantees our pointers will be aligned to size_of::<usize> so we can offset by that amount without fear.
|
utACK. cc @jonasnick can I also have an ack from you? |
|
Published. rust-bitcoin/rust-bitcoinconsensus#9 should be able to move forward now. Thanks @elichai ! |
This is an approach to fixing the problems raised here: rust-bitcoin/rust-bitcoinconsensus#9
Because C has no notion as fat pointers like rust we need to store the allocated size somewhere for deallocation.
what I've done here is writing the size of the allocation, offset the pointer by the size of the size and use that.
then for deallocation we can offset it to read the size for deallocation.
I tried minimizing alignments problems by using the most aligned type we need, if I understand correctly if
assert!(mem::align_of::<usize>() >= mem::align_of::<u8>());is true than everything aligned to usize will be also aligned to u8.Then I just allocate everything using usizes instead of u8.
And I added some assertions that should never fail. (the check for usize->u8 alignments would probably even be evaluated in compile time)
when the day comes and we upgrade to rust 1.28 we could use this instead: https://doc.rust-lang.org/alloc/alloc/struct.Layout.html
I do think that aligning to usize/u8 and not to [u8] and [usize] is correct because arrays and the types in them should be aligned the same way (and you can't check alignments of unsized types) and the construction of the fat pointer is done via slice::from_raw_parts which accepts
*const Tand returns&[T].and in rust references are a pointer + length, and
*const [T]is semantically equivalent to *const T(source: rust-lang/rust-bindgen#1561 (comment)).Would more than love feedback :)
cc @jonasnick