More ffi view #1688
More ffi view #1688
Conversation
| * This code assumes that it was compiled with the system allocator: | ||
| * https://doc.rust-lang.org/beta/std/alloc/struct.System.html | ||
| * Which means that e.g. returning a Box<str> from Rust can be safely | ||
| * freed on the C side with the C library's `free()`. |
There was a problem hiding this comment.
I don't think we can just assume this. We have to explicitly use #[global_allocator], right?
Also note that was made stable in 1.28, but we're still on 1.26.2. So, maybe for now let's drop this paragraph and once we bump to 1.28, we can add #[global_allocator] and re-add it?
There was a problem hiding this comment.
- Which means that e.g. returning a Box from Rust can be safely
- freed on the C side with the C library's
free().
I was looking at the original RFCs again for global_allocator, and it's not clear if this is a direct consequence of this new feature. For one thing, it depends on how the Drop trait is implemented for specific types, right? So it could be safe for some (most) types, but not others. And worse, that behaviour itself could change from one version to the next.
There was a problem hiding this comment.
We get the global allocator by default today since we're building a static library, not an executable. Also, jemalloc is gone now anyways.
When we hard require 1.28 we can add the #[global_allocator] bit but it'll be a no-op.
There was a problem hiding this comment.
For one thing, it depends on how the Drop trait is implemented for specific types, right?
We clearly can't assume that free() is the same as Drop. But we can assume that Box<> is allocated via malloc() - that's basically what system allocator means. So this assumption is for types that don't implement Drop - the most important of which is str.
There was a problem hiding this comment.
Really this is mostly about strings, and theoretically byte buffers though in practice most of those would probably be fds. I want it to be ergonomic to pass strings back and forth between C/Rust.
There was a problem hiding this comment.
We get the global allocator by default today since we're building a static library, not an executable.
Ahh right, good point. Wait, so... that means we could have exploited this all along, heh.
So this assumption is for types that don't implement
Drop- the most important of which isstr.
Right, I understand the intended goal is mostly about strings. Though I just want to make sure this makes sense at a higher level. I guess my question is: is "not implementing Drop" part of the stable API? I feel like the answer is "yes", but I'd like confirmation.
Re. str specifically, did you mean String? For it to be eligible for free() on the C side, it'd have to be an owned type we then mem::forget(), right?
There was a problem hiding this comment.
Not just "could have" but intentionally have been, see ror_treefile_get_rojig_name(). Now, the API docs say not to call free() but...it really just is inner: Box<[u8]>,.
There was a problem hiding this comment.
see
ror_treefile_get_rojig_name().
Ahh, missed that. Should make things easier going forward.
More obvious one shouldn't leak the pointer.
Change our existing "view as [u8] API", and also add one that does a view as `OsStr`. The motivation for the latter is I noticed ithat `OsStr::from_bytes()` *doesn't* copy, or rather it just copies the pointer value. Rust's lifetime inference ensures that the returned lifetime matches the input array. I think the previous code in `treefile.rs` was confused about this.
Create a new `_new_` naming convention, and extend the FFI documentation to describe the new state as well as background assumptions.
cgwalters
force-pushed
the
more-ffi-view
branch
from
2124cf0 to
cce802f
Compare
|
Rebased 🏄♂️ with fixups. |
|
Hmm, OK gonna have to look into the PAPR issues after lunch. |
|
OK, should hopefully work now. |
Change our existing "view as [u8] API", and also add one that does a view as `OsStr`. The motivation for the latter is I noticed ithat `OsStr::from_bytes()` *doesn't* copy, or rather it just copies the pointer value. Rust's lifetime inference ensures that the returned lifetime matches the input array. I think the previous code in `treefile.rs` was confused about this. Closes: #1688 Approved by: jlebon
Create a new `_new_` naming convention, and extend the FFI documentation to describe the new state as well as background assumptions. Closes: #1688 Approved by: jlebon
|
☀️ Test successful - status-atomicjenkins |
Follow-up to #1685