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

ty: Simplify Box<T> to *T #451

Closed
wants to merge 1 commit into from
Closed

Conversation

edsrzf
Copy link
Contributor

@edsrzf edsrzf commented Jan 12, 2020

This simplifies Box<T> to *T, which enables use cases not previously supported.

Box<T> is guaranteed to have the same layout as *T as long as T is sized. This is documented in the current Rust beta. (See rust-lang/rust#62514 and https://doc.rust-lang.org/beta/std/boxed/index.html#memory-layout.)

This does mean that the declarations we're emitting are incorrect for unsized types (eg Box<[u8]>), but it still seems like a strict improvement over old behavior.

This can also be a recipe for memory leaks if not handled carefully, but again, still seems better than previously.

Test cases are shamelessly copied and modified from the NonNull simplification.

This simplifies Box<T> to *T, which enables use cases not previously
supported.

Box<T> is guaranteed to have the same layout as *T as long as T is
sized. This is documented in the current Rust beta.

This does mean that the headers we're emitting are incorrect for unsized
types, but it still seems like a strict improvement over old behavior.

This can also be a recipe for memory leaks if not handled carefully, but
again, still seems better than previously.

Test cases are shamelessly copied and modified from the NonNull simplification.
@@ -389,6 +389,7 @@ impl Type {
generic.simplify_standard_types();

match path.name() {
"Box" => Some(Type::Ptr(Box::new(generic))),
Copy link
Contributor Author

@edsrzf edsrzf Jan 12, 2020

Choose a reason for hiding this comment

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

I'm happy to add a FIXME comment and file an issue for unsized types, similar to the comment below, if this is otherwise acceptable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, sounds sensible, but please add a comment pointing to the documentation.

Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

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

So this is ok for arguments, but not for members, right? For members we still need something that allows to get the right constructor / destructor behavior. Otherwise this needs t be opt-in in some way.

@@ -389,6 +389,7 @@ impl Type {
generic.simplify_standard_types();

match path.name() {
"Box" => Some(Type::Ptr(Box::new(generic))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, sounds sensible, but please add a comment pointing to the documentation.

@emilio
Copy link
Collaborator

emilio commented Jan 12, 2020

In particular, I use it like this to be able to translate boxes inside structs. Obviously this changes the ABI when passing them by value, but that's a trade-off we currently live with (we only pass these types by reference/pointer).

This is only interesting for arguments, as far as I can tell, right?

@edsrzf
Copy link
Contributor Author

edsrzf commented Jan 12, 2020

Thanks for the response.

I'm not sure I follow the example you've given. It looks like the code you've linked to is entirely hand-written (rather than generated by cbindgen), and then cbindgen generates types that use StyleBox? And the memory is managed by C++ rather than Rust, which doesn't quite make sense to me.

Maybe it would help for me to describe my motivation in greater detail.

I'm gradually converting a C++ code base to Rust. The basic idea is to convert something to Rust and then use cbindgen to spit out a header that exposes the converted thing in a way that's as similar as possible to the original C++. Of course I usually have to make some change to the original C++ code base as I go, but I want to keep these changes to an absolute minimum. When I do have to make a change, it should be very simple so that I can use refactoring tools or global find/replace rather than do a bunch of manual work.

When I want to convert a C++ struct or class that owns some data, like, say:

struct Foo {
  std::unique_pointer<Bar> ptr;
};

It makes sense to me that I'd want to convert this to Rust like this:

struct Foo {
  ptr: Box<Bar>,
}

When cbindgen generates a C++ declaration for this Rust struct, it looks like this:

template<typename T>
struct Box<T>;

struct Foo {
  Box<Bar> ptr;
}

Does this mean that it's up to me to define an implementation of Box, similar to what you've done with StyleBox in your example? The difference is that, for me, I want to be able to allocate from within Rust and I want Rust to manage the memory. I'm not sure it's possible for me to define Box such that the memory can be managed by Rust.

The way I intend to work around this is by manually defining a destructor for Foo like:

  // Call into Rust to free the memory.
  ~Foo() { drop_foo(this); }

@emilio
Copy link
Collaborator

emilio commented Jan 12, 2020

I'm not sure I follow the example you've given. It looks like the code you've linked to is entirely hand-written (rather than generated by cbindgen), and then cbindgen generates types that use StyleBox? And the memory is managed by C++ rather than Rust, which doesn't quite make sense to me.

That is ok, beause we use cbindgen to also generate destructors (see the other options related to this), and we share allocators between rust and C++, so delete foo; ends up being the same as drop(Box::from_raw(foo));.

Does this mean that it's up to me to define an implementation of Box, similar to what you've done with StyleBox in your example?

Yeah, that's what I'm doing and it sounds about right.

The difference is that, for me, I want to be able to allocate from within Rust and I want Rust to manage the memory. I'm not sure it's possible for me to define Box such that the memory can be managed by Rust.

That seems also fine. If you want your behavior you basically could do:

template<typename T>
using Box = T*;

Right? In which case we'd have to make cbindgen not generate the forward declaration for box as a struct, or something.

But even without that, you can manage the memory via rust perfectly fine.

C++ is great / horrible for that. You could do that via template specialization, with something like:

#include <cstdint>
#include <cassert>

template <typename T>
struct Deleter {
    static void Delete(T* ptr) {
        delete ptr;
    }
};

struct MyRustType;

extern "C" void my_rust_api_delete(MyRustType*);
extern "C" MyRustType* my_rust_api_new();

template <>
struct Deleter<MyRustType> {
    static void Delete(MyRustType* ptr) {
        my_rust_api_delete(ptr);
    }
};

template<typename T>
struct Box {
    // ...

    T* raw;

    ~Box() {
        Deleter<T>::Delete(raw);
    }
};

int main() {
    Box<MyRustType> t { my_rust_api_new() }; // Dropping this will call my_rust_api_delete.
}

We use some similar patterns to make some less-complex rust bindings work with mozilla::UniquePtr and such (see this for example.

@edsrzf
Copy link
Contributor Author

edsrzf commented Jan 13, 2020

If I explicitly exclude Box, then cbindgen won't emit the forward declaration, and I can do as you suggest with:

template <typename T>
using Box = T*;

This works for me. I don't think this PR is necessarily the right approach anymore, but I do think there could be better docs or examples around some of the patterns available with cbindgen. And it feels like there could be a way to improve ergonomics, but I'm not sure exactly what it is.

@edsrzf edsrzf closed this Jan 13, 2020
@emilio
Copy link
Collaborator

emilio commented Jan 13, 2020

A PR with some docs like that would be extremely useful!

@emilio
Copy link
Collaborator

emilio commented Jan 13, 2020

(And agreed about ergonomics, ideas there welcome :))

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.

2 participants