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

Sugar Needed? #228

Open
abrown opened this issue Jul 9, 2020 · 9 comments
Open

Sugar Needed? #228

abrown opened this issue Jul 9, 2020 · 9 comments
Labels
integration How cxx fits into the big picture of an organization's codebase and builds

Comments

@abrown
Copy link

abrown commented Jul 9, 2020

After the discussion of #221, I tried out using cxx to expose OpenVINO's C++ API to Rust. As @dtolnay suggested, I created wrapper functions for the constructors to wrap the constructed object with std::make_unique. The process to expose each C++ function looks something like:

  1. Create a C++ wrapper function to get to std::make_unique; e.g., the core_new bridge function.
  2. Add that wrapper function to the Rust cxx::bridge definition; e.g., the core_new definition.
  3. Create a Rust structure to wrap the UniquePtr; e.g., the struct Core and Core::new.
  4. Attempt to use the Rust structure and realize that I need to wrap each method in C++ (e.g. read_network) and again in Rust (e.g. read_network) to use the FFI function.

If I follow this process I will be writing C++ wrapper functions, Rust wrapper methods, and cxx definitions all the way down the object graph--which seems both time-consuming and redundant. Is there more sugar needed somewhere, e.g. an automatic way to wrap and unwrap C++ objects in UniquePtrs? Or is there some other process I should be following for exposing the API?

@dtolnay
Copy link
Owner

dtolnay commented Jul 10, 2020

I am very interested in supporting this use case better since my work codebase also contains a lot of that style of wrappage.

So far I have not dedicated much attention to sugar, but now the foundational bits are at a level of correctness and safety where it starts to make sense to think about higher level sugar.

One possibly relevant other direction I have been planning, which is different from the approach of "automatic way to wrap and unwrap C++ objects in UniquePtrs" that you call out, is to expose an API that is more conducive to higher level code generators. Instead of baking openvino's pattern of using UniquePtr into this crate, we would provide a way that your build.rs or something could ingest Rust code (or a C++ header, or a Thrift file, or a JSON description of an API, or whatever) and programmatically transform it to stick the right UniquePtrs wherever it wants before handing off to cxx's code generator.

The great thing about higher level code generators like that is there is no need for them to be particularly principled. I have some experience with this from syn's generated syntax tree traversal traits (e.g. syn/src/gen/visit_mut.rs). It's pretty easy in the code generator for that kind of thing to insert hardcoded special cases where something needs to be handled just slightly differently for whatever reason. With a baked in "automatic way to wrap and unwrap", capturing special cases like that would generally be much harder for the downstream library author.

@dtolnay
Copy link
Owner

dtolnay commented Jul 10, 2020

FWIW you don't necessarily need all those wrappers on the C++ side. You can directly expose and call C++ member functions:

mod ffi {
    extern "C" {
        ...

        type CNNNetwork;
        pub fn setBatchSize(self: &mut CNNNetwork, size: usize);
    }
}

...

pub struct CNNNetwork {
    instance: UniquePtr<ffi::CNNNetwork>,
}

impl CNNNetwork {
    pub fn set_batch_size(&mut self, size: usize) {
        self.instance.setBatchSize(size);
    }
}

@abrown
Copy link
Author

abrown commented Jul 10, 2020

FWIW you don't necessarily need all those wrappers on the C++ side

Good point; I removed that: abrown/openvino@d9bf2d4.

higher level code generators

Trying to understand what you have in mind: I've used Thrift before so I can sort of grok that, would this be like "provide a Thrift file on one end and some C++ headers on the other end and cxx fills in the gaps in the middle?"

With a baked in "automatic way to wrap and unwrap", capturing special cases like that would generally be much harder for the downstream library author.

I agree in principle that "automatic" magic stuff is usually a pain but I'm struggling to understand the difficulty you see: with my rudimentary understanding of cxx, I thought that if I had a C++ function Foo construct() then I could define this in the cxx::bridge block as pub fn construct() -> UniquePtr<Foo> and cxx could do the necessary checks to understand "oh, I need to add a std::make_unique around Foo before returning it" in the generated code. Since I have this mental model where the cxx::bridge block is like a high-level API description language (which if I squint is sort of like the high level generators you describe), then I don't see the difficulty. What am I missing?

@abrown
Copy link
Author

abrown commented Jul 14, 2020

@dtolnay, I still would like to understand better what you see as the problem here; I've since found that I also need a way to use C++'s std::map, std::shared_ptr, and exceptions, which is quite a lot (maybe that's why you mention higher-level generators?). If it's easier to chat, I can usually be found here.

@dtolnay
Copy link
Owner

dtolnay commented Jul 14, 2020

with my rudimentary understanding of cxx, I thought that if I had a C++ function Foo construct() then I could define this in the cxx::bridge block as pub fn construct() -> UniquePtr<Foo> and cxx could do the necessary checks to understand "oh, I need to add a std::make_unique around Foo before returning it" in the generated code
...
I don't see the difficulty. What am I missing?

I think the missing piece is around cxx being able to identify that a std::make_unique needs to be inserted -- because currently we don't parse or look into the C++ header at all. Figuring out that a std::make_unique is supposed to be added requires one of three things:

  • Parse the header to figure out the required signature; or
  • Build more functionality into cxx::bridge to let the call site say they want the make_unique inserted there; or
  • Have a code generator with higher level information about the desired correspondence between the Rust and C++ side emit the cxx::bridge invocation and whatever glue on either side.

The first is not one that I consider a good direction. Either of the second two could work though, with the third being something I know I want/need for other purposes. Essentially this comes from considering cxx::bridge as being analogous to extern "C" except conducive to safety; a minimal obvious 1-1 correspondence between the languages. On top of that it's reasonable to build less 1-1 more ergonomic mappings that take the form of code generators. The core in cxx remains solely responsible for guaranteeing safety, without a high amount of complexity/sugary functionality. The higher level generator becomes responsible for ergonomics and capturing the idioms of different codebases (like #16) without risk of introducing unsafety because the safety is guaranteed by cxx::bridge as the lower level substrate.


higher level code generators

Trying to understand what you have in mind: I've used Thrift before so I can sort of grok that, would this be like "provide a Thrift file on one end and some C++ headers on the other end and cxx fills in the gaps in the middle?"

To build on the previous point, the way to understand them would be as "to cxx what bindgen is to extern "C"". It isn't exactly analogous but that is the separation.

@dtolnay
Copy link
Owner

dtolnay commented Jul 14, 2020

I've since found that I also need a way to use C++'s std::map, std::shared_ptr, and exceptions, which is quite a lot (maybe that's why you mention higher-level generators?).

I still 100% intend to support std::map and std::shared_ptr. They're listed in the readme and would be implemented exactly analogous to how std::vector and std::unique_ptr currently work. I just need to find the time to write the binding or a contributor interested in investigating that! For now the least bad workaround would be treating them as opaque C++ types which you pass around behind a unique_ptr, and include in your cxx::bridge any of the member functions you need to expose on them from C++ to Rust.

Exceptions are supported already and support translation to Rust Result in either direction:

// rust
#[cxx::bridge]
mod ffi {
    extern "C" {
        fn f() -> Result<()>;
    }
    extern "Rust" {
        fn g() -> Result<()>;
    }
}

// c++
void f() {
    throw ...;
}
void main() {
    try {
        g();
    } catch (const rust::Error &e) {...}
}

@alexxbb
Copy link

alexxbb commented Jul 15, 2020

Hey David, I didn't want to put out a new issue, I'm going to ask here, since it's related.

Would it be possible to push the priority on implementing std::shared_ptr ?
The library that I'm wrapping is heavy on std::shared_ptr and since we only have std::unique_ptr I have to do:

template <class T>
class Wrapper {
     public:
         Wrapper(T &&ptr) { inner = ptr; }
         T inner;
}

std::unique_ptr<Wrapper<Resource>> some_func() {
     std::shared_ptr<Resource> res = get_resource();
     return std::make_unique<Wrapper>(std::move(res));
}

This is far from ideal (and I might be messing things up with ownership here) but at least it works for my case.
We really need std::shared_ptr 😃

Also this:

std::unique_ptr<std::vector<Wrapper<Resource>>> get_resources();

On the Rust side I'd like to have a new owning Resource type:

struct Resource {
    ptr: UniquePtr<ffi::Resource>
}

pub fn get_resources() -> UniquePtr<CxxVector<ffi::Resource>>;

But we can't move a value out of CxxVector ( this is where std::shared_ptr is so much needed)

Cheers.

@abrown
Copy link
Author

abrown commented Jul 15, 2020

[@alexxbb: was just about to send the stuff below when I saw your message]

@dtolnay, thanks for your explanation; yeah, I see what you mean about choosing option 3 (generators) over option 2 (stuff it into cxx::bridge)--it makes the boundary more clear. This may seem weird to you, but the inclusion of include!("...") in cxx::bridge made me assume that something along the lines of option 1 (parsing headers) was happening--now that I see that it is not, perhaps specifying the header is more of a build-time configuration and would make more sense in the build.rs as a parameter on cxx_build::bridge (not a strong suggestion, just a thought that may or may not make things more clear to future users). I'm interested in discussing more what these higher-level generators would look like--what are your thoughts at this point?

For std::map and std::shared_ptr, can you point me to where to look to see if I even understand enough of this to help out?

@abrown
Copy link
Author

abrown commented Jul 17, 2020

Re: higher level generators, I saw that opencv-rust is doing something perhaps like what you are thinking: https://github.com/twistedfall/opencv-rust/tree/master/binding-generator?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration How cxx fits into the big picture of an organization's codebase and builds
Projects
None yet
Development

No branches or pull requests

3 participants