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

Don't generate bindings for deleted functions #426

Closed
martinboehme opened this issue Apr 27, 2021 · 8 comments · Fixed by #469
Closed

Don't generate bindings for deleted functions #426

martinboehme opened this issue Apr 27, 2021 · 8 comments · Fixed by #469
Labels
cpp-feature C++ feature not yet supported

Comments

@martinboehme
Copy link
Collaborator

autocxx currently attempts to generate bindings for deleted functions, and the C++ code for these then fails to compile.

Here is a simple example:

class A {
public:
    void foo() = delete;
};

This fails with the error "use of deleted function ‘void A::foo()’’".

Here is an example with an implicitly deleted copy constructor:

class A {
public:
    A(A&&);
};

This fails with the error "use of deleted function ‘constexpr A::A(const A&)'".

I will shortly submit a PR with failing tests.

@martinboehme
Copy link
Collaborator Author

Failing tests in #427.

@adetaylor adetaylor added the cpp-feature C++ feature not yet supported label Apr 27, 2021
@adetaylor
Copy link
Collaborator

In both cases I don't think we're getting the information required from bindgen to be able to do anything about this.

For example here is what bindgen gives us for the first case:

    #[allow(non_snake_case, non_camel_case_types, non_upper_case_globals)]
    pub mod root {
        #[allow(unused_imports)]
        use self::super::root;
        #[repr(C)]
        pub struct A {
            pub _address: u8,
        }
        extern "C" {
            #[bindgen_original_name("foo")]
            #[link_name = "\u{1}__ZN1A3fooEv"]
            pub fn A_foo(this: *mut root::A);
        }
        impl A {
            #[inline]
            pub unsafe fn foo(&mut self) {
                A_foo(self)
            }
        }
    }

So I think this is another item to add to the list in #124 of annotations/metadata we need from bindgen. I know you're working on it. Thanks!

@martinboehme
Copy link
Collaborator Author

In both cases I don't think we're getting the information required from bindgen to be able to do anything about this.

For example here is what bindgen gives us for the first case:

[snip]

Hm. This came from autocxx's invocation of bindgen, I assume (judging by the pub mod root)?

Interestingly, if I run the bindgen command-line tool (from the head of autocxx-bindgen), I don't get a binding for foo(). If I then remove the = delete, the binding gets generated.

So it seems that bindgen is in fact able to distinguish between functions declared as deleted and normal function declarations -- it's just not doing it when it's called by autocxx for some reason. Time to dig deeper into how autocxx is calling bindgen.

@martinboehme
Copy link
Collaborator Author

I've now worked out why there is a behavior difference between the bindgen command line tool and autocxx's invocation of bindgen.

This is because autocxx sets the equivalent of the flag --generate-inline-functions. If I set this flag on the command-line tool, it also generates a binding for A::foo().

The reason for this is that deleted functions are implicitly inline, and Clang's Sema::SetDeclDeleted() correspondingly marks them as such. So bindgen isn't actually picking up on the fact that these functions are deleted -- it just looks that way because bindgen's default is not to generate bindings for deleted functions.

Unfortunately, from looking at the libclang API, I think there is currently no way to query whether a function is deleted. I think this will make it very hard to retrofit this functionality in bindgen because we would first have to introduce the functionality in libclang, then wait for a libclang release and update the clang-sys crate before we could use the functionality in bindgen.

It's possible that there's something I've missed though -- I'll keep looking.

@martinboehme
Copy link
Collaborator Author

FWIW, I've filed a bindgen issue here:

rust-lang/rust-bindgen#2043

@martinboehme
Copy link
Collaborator Author

Some more notes.

In the second case, bindgen isn't actually emitting a binding for the implicitly deleted copy constructor. It's only emitting a binding for the (explicitly declared) move constructor.

The problem is that bindgen generates the same Rust function signature for copy constructors and move constructors. Here's the signature that bindgen generates for the move constructor:

extern "C" {
    #[link_name = "\u{1}_ZN1AC1EOS_"]
    pub fn A_A(this: *mut A, arg1: *mut A);
}

For comparison, here's what bindgen would generate for a copy constructor A(A&):

extern "C" {
    #[link_name = "\u{1}_ZN1AC1ERS_"]
    pub fn A_A(this: *mut A, arg1: *mut A);
}

So autocxx sees the move constructor signature, thinks it's a copy constructor, and generates the following C++ code for make_unique() that tries to call the (deleted) copy constructor:

inline std::unique_ptr<A> make_unique_autocxx_wrapper(A& arg0) { return std::make_unique<A>(arg0); }

So the underlying problem here is that autocxx can't distinguish between copy constructors and move constructors. It looks as if we'll need to emit some annotation in bindgen that tells us whether we're looking at a copy constructor or a move constructor.

@martinboehme
Copy link
Collaborator Author

I've pulled the question of move constructors out into a separate issue: #456

On the actual issue of deleted functions, I've submitted a PR upstream: rust-lang/rust-bindgen#2044

Once that lands, we should pull from upstream and remove the #[ignore] on the currently failing test. At that point, this issue can then be closed.

@martinboehme
Copy link
Collaborator Author

Upstream PR has been merged.

@adetaylor , could you merge the upstream changes to autocxx-bindgen and release a new version? I'll then submit a PR that revs the dependency on autocxx-bindgen and removes the #[ignore] from the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp-feature C++ feature not yet supported
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants