ir: Add the possibility to auto-generate destructors of tagged unions. by emilio · Pull Request #333 · mozilla/cbindgen

emilio · 2019-05-02T14:08:19Z

This trades the ABI-safety (making the struct potentially unsafe to pass by
value), thus the opt-in rather than auto-generating it.

The TL;DR is that I need a way to share types that contain boxed slices from the
style system. That's fine when they're inside regular structs, since C++ does
the right thing, but for unions you need to make it explicit like this.

The actual Gecko setup will hook into the leak logging machinery to avoid silly
leaks and such of course.

This trades the ABI-safety (making the struct potentially unsafe to pass by value), thus the opt-in rather than auto-generating it. The TL;DR is that I need a way to share types that contain boxed slices from the style system. That's fine when they're inside regular structs, since C++ does the right thing, but for unions you need to make it explicit like this. The actual Gecko setup will hook into the leak logging machinery to avoid silly leaks and such of course.

emilio · 2019-05-02T14:13:26Z

r? @eqrion

I'd prefer an speedy review of this so that @gankro can update cbindgen in Gecko with this change too (so as to minimize other dev's disruption). But only if this is uncontroversial generally. If not I'm happy to wait.

emilio · 2019-05-02T14:16:07Z

Oh I thought #332 had landed already. Then forget about speediness :)

Gankra · 2019-05-02T16:56:58Z

What's the point of this if we don't ever emit struct destructors? Can't this only ever run no-op (POD) destructors?

Gankra · 2019-05-02T17:09:54Z

The implementation otherwise seems fine, although I'd appreciate a repr(C, u8) enum test as well (edit: is it worth it to try to eliminate the default case when all cases have contents?)

emilio · 2019-05-02T17:33:45Z

What's the point of this if we don't ever emit struct destructors? Can't this only ever run no-op (POD) destructors?

Structs do the right thing by default in C++, it's only when they contain unions when the compiler cannot infer it.

Gankra · 2019-05-02T17:38:04Z

But that's the exact same problem, where are types with real destructors showing up in the system? Template substitutions?

emilio · 2019-05-02T17:44:48Z

But that's the exact same problem, where are types with real destructors showing up in the system? Template substitutions?

I noted in the example, you can add a destructor manually via either type substitution or arbitrary body contents (export.body).

Gankra · 2019-05-02T17:57:18Z

Ok so just to further clarify how we want to use this, will the non-trivial-destructor types live strictly in C++, or will they actually get passed across the FFI boundary (by reference, of course)? If the latter, will the Rust side also have equivalent Drop impls?

Gankra

impl LGTM

emilio · 2019-05-02T18:01:17Z

Ok so just to further clarify how we want to use this, will the non-trivial-destructor types live strictly in C++, or will they actually get passed across the FFI boundary (by reference, of course)? If the latter, will the Rust side also have equivalent Drop impls?

Rust will "move" the types into C++ (putting them in a struct for example, or passing them by reference), and C++ is the responsible to drop them.

RReverser · 2019-05-02T19:24:46Z

you can add a destructor manually via either type substitution or arbitrary body contents (export.body).

Why the same can't be done for tagged enums? I'm all for supporting destructors in cbindgen, but it feels wrong to support them only for tagged enums in a special manner, different from regular structs...

Gankra · 2019-05-02T19:38:09Z

This feature is more-so creating greater parity, as C++'s native understanding of structs automagically gets us default destructors that just drop all the fields, but C++ doesn't natively understand tagged unions, so we need to go out of our way to tell it what the default destructor should be.

However, because it is a user-defined destructor, it makes the type non-trivial, and therefore non-pod. It should still have standard layout, though (I Am Not A C++ Language Lawyer).

emilio · 2019-05-02T21:38:16Z

Why the same can't be done for tagged enums? I'm all for supporting destructors in cbindgen, but it feels wrong to support them only for tagged enums in a special manner, different from regular structs...

Right, regular structs already destruct on a per-field basis. You can override this with export.body.

RReverser · 2019-05-02T21:55:16Z

Right, regular structs already destruct on a per-field basis. You can override this with export.body.

Yes, but this works only for POD structs, right? IIRC, cbindgen doesn't expose custom Drop for structs?

emilio · 2019-05-02T22:08:51Z

Well, it works if you have a struct with an inner type that has a destructor, so if you have:

struct Foo { unsigned something; ~Foo(); };

struct Bar { Foo b; }; // This is cbindgen-generated.

Dropping Bar will correctly drop Foo in C++ But yeah, you can only generate the destructor in Foo with export.body or such, it's not clear how would you go about automatically translating drop implementations.

RReverser · 2019-05-03T13:00:22Z

it's not clear how would you go about automatically translating drop implementations

I was actually thinking about this for a while - could cbindgen take similar approach as bindgen does in terms of relying on mangled symbols? (but in the opposite direction)

Currently any Drop implementation of a public type is kept in the output binary, e.g.:

pub struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {}
}

emits

_ZN54_$LT$example..Foo$u20$as$u20$core..ops..drop..Drop$GT$4drop17h05763556a065b101E:
        ret

and it should be possible to mangle to the same name and link in C/C++ headers.

Not sure if it's actually feasible or aligns with cbindgen goals though :)

emilio · 2019-05-03T17:48:57Z

Currently any Drop implementation of a public type is kept in the output binary, e.g.:

Even in optimized builds? That'd be surprising. bindgen can only generate non-inline destructors.

and it should be possible to mangle to the same name and link in C/C++ headers.

Does C++ have something similar to the #[link_name] attribute in rust? Even if so, cbindgen right now doesn't have any info from rustc itself, so it'd be quite hard to implement / fragile.

RReverser · 2019-05-03T18:18:25Z

Even in optimized builds? That'd be surprising. bindgen can only generate non-inline destructors.

Yup, always, just like any public API methods.

Does C++ have something similar to the #[link_name] attribute in rust?

I'd expect generated destructor to be defined in C++ but also to link the Rust destructor by its mangled name internally.

so it'd be quite hard to implement / fragile

Yeah this is true.

emilio · 2019-05-03T18:50:30Z

Yup, always, just like any public API methods.

Just checked and it doesn't do it for #[inline] destructors, or generics, for that matter.

I'd expect generated destructor to be defined in C++ but also to link the Rust destructor by its mangled name internally.

Right, my point is how to do this (asm("call $symbol") I guess?)

RReverser · 2019-05-03T20:32:19Z

or generics, for that matter

Oh yeah, definitely not for generics, because they have to be monomorphised. Surprised about inline marker though :/

Right, may point is how to do this (asm("call $symbol") I guess?)

Hmm, I would've just used an extern function definition.

emilio · 2019-05-04T08:53:38Z

Symbols like _ZN54_$LT$example..Foo$u20$as$u20$core..ops..drop..Drop$GT$4drop17h05763556a065b101E aren't valid C function names AFAIK.

…r=heycam Passing these by value won't be ok of course, but that's fine. I plan to combine this with mozilla/cbindgen#333 to actually be able to share representation for ~all the things, this is just the first bit. Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them without issues. It's boxed slices the only ones I should need something like this. I could avoid it if I rely on Rust's internal representation, which we can per [1], but then I need to teach cbindgen all about slices, which is generally hard, I think. [1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md Differential Revision: https://phabricator.services.mozilla.com/D29768 --HG-- extra : moz-landing-system : lando

…nates. r=TYLin,heycam This enables destructors for tagged unions in cbindgen, implemented in: * mozilla/cbindgen#333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box<BasicShape> to UniquePtr<BasicShape>. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769 --HG-- extra : moz-landing-system : lando

…r=heycam Passing these by value won't be ok of course, but that's fine. I plan to combine this with mozilla/cbindgen#333 to actually be able to share representation for ~all the things, this is just the first bit. Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them without issues. It's boxed slices the only ones I should need something like this. I could avoid it if I rely on Rust's internal representation, which we can per [1], but then I need to teach cbindgen all about slices, which is generally hard, I think. [1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md Differential Revision: https://phabricator.services.mozilla.com/D29768

…nates. r=TYLin,heycam This enables destructors for tagged unions in cbindgen, implemented in: * mozilla/cbindgen#333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box<BasicShape> to UniquePtr<BasicShape>. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769

Passing these by value won't be ok of course, but that's fine. I plan to combine this with mozilla/cbindgen#333 to actually be able to share representation for ~all the things, this is just the first bit. Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them without issues. It's boxed slices the only ones I should need something like this. I could avoid it if I rely on Rust's internal representation, which we can per [1], but then I need to teach cbindgen all about slices, which is generally hard, I think. [1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md Differential Revision: https://phabricator.services.mozilla.com/D29768

This enables destructors for tagged unions in cbindgen, implemented in: * mozilla/cbindgen#333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box<BasicShape> to UniquePtr<BasicShape>. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769

* Require C++11 to run the test-suite (#341, test-only) * Improve mangling error message (#340) * Add the ability to automatically derive copy-constructors for tagged enums (#339) * Use placement new for constructing in tagged unions' helper methods (#333)

…r=heycam Passing these by value won't be ok of course, but that's fine. I plan to combine this with mozilla/cbindgen#333 to actually be able to share representation for ~all the things, this is just the first bit. Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them without issues. It's boxed slices the only ones I should need something like this. I could avoid it if I rely on Rust's internal representation, which we can per [1], but then I need to teach cbindgen all about slices, which is generally hard, I think. [1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md Differential Revision: https://phabricator.services.mozilla.com/D29768 UltraBlame original commit: 41acb048244d7d2bad371761bf54c80670fe683b

…nates. r=TYLin,heycam This enables destructors for tagged unions in cbindgen, implemented in: * mozilla/cbindgen#333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box<BasicShape> to UniquePtr<BasicShape>. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769 UltraBlame original commit: fe11fc11ec5b0ee0111351c01cf601109798ca85

…r=heycam Passing these by value won't be ok of course, but that's fine. I plan to combine this with mozilla/cbindgen#333 to actually be able to share representation for ~all the things, this is just the first bit. Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them without issues. It's boxed slices the only ones I should need something like this. I could avoid it if I rely on Rust's internal representation, which we can per [1], but then I need to teach cbindgen all about slices, which is generally hard, I think. [1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md Differential Revision: https://phabricator.services.mozilla.com/D29768 UltraBlame original commit: 41acb048244d7d2bad371761bf54c80670fe683b

…nates. r=TYLin,heycam This enables destructors for tagged unions in cbindgen, implemented in: * mozilla/cbindgen#333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box<BasicShape> to UniquePtr<BasicShape>. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769 UltraBlame original commit: fe11fc11ec5b0ee0111351c01cf601109798ca85

…r=heycam Passing these by value won't be ok of course, but that's fine. I plan to combine this with mozilla/cbindgen#333 to actually be able to share representation for ~all the things, this is just the first bit. Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them without issues. It's boxed slices the only ones I should need something like this. I could avoid it if I rely on Rust's internal representation, which we can per [1], but then I need to teach cbindgen all about slices, which is generally hard, I think. [1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md Differential Revision: https://phabricator.services.mozilla.com/D29768 UltraBlame original commit: 41acb048244d7d2bad371761bf54c80670fe683b

…nates. r=TYLin,heycam This enables destructors for tagged unions in cbindgen, implemented in: * mozilla/cbindgen#333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box<BasicShape> to UniquePtr<BasicShape>. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769 UltraBlame original commit: fe11fc11ec5b0ee0111351c01cf601109798ca85

…r=heycam Passing these by value won't be ok of course, but that's fine. I plan to combine this with mozilla/cbindgen#333 to actually be able to share representation for ~all the things, this is just the first bit. Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them without issues. It's boxed slices the only ones I should need something like this. I could avoid it if I rely on Rust's internal representation, which we can per [1], but then I need to teach cbindgen all about slices, which is generally hard, I think. [1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md Differential Revision: https://phabricator.services.mozilla.com/D29768

…nates. r=TYLin,heycam This enables destructors for tagged unions in cbindgen, implemented in: * mozilla/cbindgen#333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box<BasicShape> to UniquePtr<BasicShape>. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769

emilio requested a review from eqrion May 2, 2019 14:09

Test both repr(u8) and repr(C, u8).

a4ce6c8

Make it more similar to the other options.

49c056a

Gankra approved these changes May 2, 2019

View reviewed changes

emilio merged commit cec3803 into mozilla:master May 2, 2019

emilio deleted the tagged-union-dtor branch May 2, 2019 21:38

Conversation

emilio commented May 2, 2019

Uh oh!

emilio commented May 2, 2019

Uh oh!

emilio commented May 2, 2019

Uh oh!

Gankra commented May 2, 2019

Uh oh!

Gankra commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio commented May 2, 2019

Uh oh!

Gankra commented May 2, 2019

Uh oh!

emilio commented May 2, 2019

Uh oh!

Gankra commented May 2, 2019

Uh oh!

Gankra left a comment

Choose a reason for hiding this comment

Uh oh!

emilio commented May 2, 2019

Uh oh!

RReverser commented May 2, 2019

Uh oh!

Gankra commented May 2, 2019

Uh oh!

emilio commented May 2, 2019

Uh oh!

RReverser commented May 2, 2019

Uh oh!

emilio commented May 2, 2019

Uh oh!

RReverser commented May 3, 2019

Uh oh!

emilio commented May 3, 2019

Uh oh!

RReverser commented May 3, 2019

Uh oh!

emilio commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented May 3, 2019

Uh oh!

emilio commented May 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Gankra commented May 2, 2019 •

edited

Loading

emilio commented May 3, 2019 •

edited

Loading