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

Wrong ABI used for small C++ classes. #18

Closed
vadimcn opened this issue Aug 12, 2017 · 6 comments
Closed

Wrong ABI used for small C++ classes. #18

vadimcn opened this issue Aug 12, 2017 · 6 comments

Comments

@vadimcn
Copy link

vadimcn commented Aug 12, 2017

So if I generate C++ types using bindgen as described in rust-lang/rust-bindgen#778, and then try to create and return such types via cpp!, I hit the exact same problem as described in that bug.

This code crashes with a segmentation fault, because C++ compiler wants to return Bar by-pointer, whereas Rust expects it in registers.

let bar = cpp!([] -> Bar as "Bar" { return MakeBar(); });

In retrospect, this kinda makes sense, because even extern "C" functions in .cpp files must obey C++ ABI.

Not sure if cpp crate should attempt to fix this, but something to keep in mind if you decide to implement "foreign types" from #16.

@vadimcn
Copy link
Author

vadimcn commented Sep 9, 2017

@mystor: So I've been thinking about this...

We could use a wrapper like this one to force the C++ compiler to return a value as "raw bytes":

    template<typename T>
    struct AsBytes {
        uint8_t bytes[sizeof(T)];

        AsBytes(const T& value) {
            memcpy(bytes, &value, sizeof(T));
        }
        AsBytes(T&& value) {
            memcpy(bytes, &value, sizeof(T));
        }
    };

In the simplest case, all closures would have to change their return types from T to AsBytes<T>. On the Rust side, we'd need to emit code to convert bytes back to properly aligned data.

Of course, this is inefficient and will hurt optimization on both sides. To deal with that, we could use type traits to detect potentially problematic types and wrap only those. The filter condition needs to be something like this:

    !std::is_trivially_copy_constructible<T>::value ||
    !std::is_trivially_destructible<T>::value ||
    std::is_polymorphic<T>::value

Thoughts?

@DemiMarie
Copy link

Could we use libclang to get the information we need?

@mystor
Copy link
Owner

mystor commented Sep 11, 2017

@vadimcn I think the easier solution would be to simply design the call surface such that we don't need to worry about these ABI concerns, by making sure to tightly control the types entering and leaving the function.

For example, we could instead generate a function sorta like this (untested :-S):

cpp!([arg1 as "A", arg2 as "B"] -> T as  "T" {
    return frob(a, b);
});
void closure(A& arg1, B& arg2, T* ret) {
    new(ret) T([&] -> T {
        return frob(a, b);
    })());
}

Which I think should move construct the return value into the outparam, which would allow us to avoid ABI concerns. It might have a perf impact for simple types though - I would want to look into it.

@DemiMarie

I would prefer to avoid rust-cpp having a dependency on clang or libclang for building, as it's nice that it doesn't need a specific C++ compiler at the moment. If I do add some libclang components, it'll be optional and only used for cross-platform linking I would think.

I think it also should be possible for getting some of this information without needing libclang right now.

@vadimcn
Copy link
Author

vadimcn commented Sep 11, 2017

@mystor: yeah, I was thinking of perf impact. Would be nice to continue returning ints and the like in registers.

@DemiMarie: libclang doesn't expose this information. Even using clang's C++ API, doing this might be challenging, because it looks like ABI info does not get computed until codegen stage.

@DemiMarie
Copy link

DemiMarie commented Sep 12, 2017 via email

@mystor
Copy link
Owner

mystor commented Sep 24, 2017

This should be fixed by #22

@mystor mystor closed this as completed Sep 24, 2017
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

No branches or pull requests

3 participants