Allocated Classes #163

rylev · 2020-08-11T14:51:03Z

Fixes #161

This (hopefully) fixes the issues in #161 which are:

COM interface methods must be called on objects that are heap allocated: We don't force the user to conform to this for regular methods because it's possible to safely use methods with &self. However, the IUnknown methods assume that self is heap allocated and won't move. We model this by taking self as self: &Pin<Box<Self>>. This makes it not possible to call IUnknown methods on a stack allocated object. If users must assume that their code is heap allocated they can declare their methods with the same signature as the IUnknown methods but if the assumption is not needed, they can just use &self.
There is no safe query_interface: classes now come with a safe query interface method called query. This may only be used when the type is heap allocated and pinned.
It is confusing how to properly instantiate a class: Classes now come with an alloc associated function which heap allocates the class, pins it, and queries the class for a given interface.
Memory management of the class is potentially wrong: classes now properly use their ref count to manage memory. The ref count starts at 1 and is incremented and decremented with add_ref and release respectively. On drop, the ref count is also decremented. When the count is 0, the class's contents are dropped. This should only happen inside of the release thunk or if the class is manually allocated by the user and never shared.

rylev · 2020-08-12T10:33:46Z

macros/support/src/class/iunknown_impl.rs

@@ -35,25 +35,28 @@ impl IUnknownAbi {
        quote! {
            unsafe extern "stdcall" fn release(this: #this_ptr) -> u32 {
                #munge
-                #class_name::release(&this)
+                let count = #class_name::release(&munged);
+                if count == 1 {


I'm not sure how I feel about this. We're relying on a ref count of 1 meaning that the only handle to the class is the original one which should have been "forgotten" through the use of ManuallyDrop. If the user forgets to wrap the class in a ManuallyDrop, and calls query to get an interface then it will have a ref count of 1 (the handle to the interface requested in query. When release is called on that interface the count will be 0. This is unfortunate as it leaks memory, though it is not unsafe.

An alternate implementation would drop the class when the count is 0. However, normally, this would leak memory in the normal case of "forgetting" the class when created by a class factory.

It's actually also possible to have a dangling pointers if this function is called with a class that has not been "forgotten":

If I return an interface from IClassFactory::create_instance that has been created from a class struct that was then dropped, it's ref count will be 1 (just the handle to interface instead of 2 for the handle and the original class object). If I then make another handle, and it gets released through the call to release ABI function, it will free the memory even though there is still a handle to an interface out there.

This requires improperly implementing an unsafe function though so I don't think it's not an obviously wrong way to go.

The usual way to handle this is for the "make" function to allocate the object and return the only reference to the object as a COM interface. That COM interface holds that first reference count.

Yes, this is what we're currently doing, though the user can allocate on their own (i.e., there's nothing stopping them other than us documenting that they shouldn't). Our "make" function is called alloc and it does just this. I'm looking for a way to prevent users from holding on to COM classes at all since they should use alloc.

Ah, so in C++/WinRT we have a virtual function called use_make_function_to_create_this_object that solves this problem. Perhaps in Rust we could have the extra fields needed for COM (e.g. vfptrs and ref count) such that they are not Default so you cannot simply construct the object yourself.

Yea I'm currently thinking how we want to do this. The code is being generated by the macro so it's currently as if the user wrote the code themselves (i.e., everything the macro produces, the user must also be able to write themselves). I'm thinking about how we can give the macro the ability to create the class but not give this ability to the user.

I pushed a change that should help.

It's still technically possible inside of the module where the class is declared to instantiate the class on the stack, but this has be done by hand and is not trivial (it's about 225 lines of total code in the basic server example). The class is not Default so there's no way to instantiate that way (though you can call MyClass::allocate_default() to get a proper heap allocated version where all the user fields are defaulted). Outside of the current module it's not possible at all to stack allocate the class.

Obviously we need docs to clarify how this all should work, but it's now highly unlikely that the user will do the wrong thing.

kennykerr · 2020-08-14T20:56:40Z

examples/basic/client/src/main.rs

    let factory = get_class_object::<IClassFactory>(&CLSID_CAT_CLASS)
        .unwrap_or_else(|hr| panic!("Failed to get cat class object 0x{:x}", hr));
    println!("Got cat class object");

+    // Get an instance of a `BritishShortHairCat` as the `IUnknown` interface


I assume this is just an example and not a requirement. 😉

Do you mean you hope it's not a requirement to get the IUnknown interface? It's not as get_instance is generic over the interface you want to get the class as.

Good, the example just made it look like I needed to retrieve the interface and then later query for the desired interface.

kennykerr · 2020-08-14T20:59:06Z

examples/basic/client/src/main.rs


+    // Get a `BritishShortHairCat` class factory
    let factory = get_class_object::<IClassFactory>(&CLSID_CAT_CLASS)


Is there also an equivalent of CoCreateInstance(Ex)?

This is a call to CoGetClassObject. We use create_instance below which directly maps to CoCreateInstance.

kennykerr · 2020-08-14T20:59:48Z

examples/basic/client/src/main.rs

    let factory = get_class_object::<IClassFactory>(&CLSID_CAT_CLASS)
        .unwrap_or_else(|hr| panic!("Failed to get cat class object 0x{:x}", hr));
    println!("Got cat class object");

+    // Get an instance of a `BritishShortHairCat` as the `IUnknown` interface
    let unknown = factory
        .get_instance::<IUnknown>()


Why is CreateInstance renamed to get_instance?

This is because there is no method overloading. I decided to rename the safe functions since they have different signatures and thus calling the safe version create_instance might be confusing. I'm starting to think that for the raw win32 COM API calls we should use the CamelCase names and the safe versions can use the snake_case convention. This might be a better way to distinguish the two. Thoughts?

Yeah, I don't renaming is such a good idea. It implies they do something else.

kennykerr · 2020-08-14T21:00:14Z

examples/basic/client/src/main.rs

    let unknown = factory
        .get_instance::<IUnknown>()
        .expect("Failed to get IUnknown");
    println!("Got IUnknown");

+    // Now get a handle to the `IAnimal` interface
    let animal = unknown
        .get_interface::<IAnimal>()


Why is QueryInterface renamed to get_interface?

Same as above. This is the safe wrapper around query_interface. We could rename the unsafe version QueryInterface and the safe version could then be query_interface.

Yes that may be better.

kennykerr · 2020-08-14T21:01:34Z

macros/support/src/class/class_constructor.rs

-// User field input as parameters to the allocate function.
-fn gen_allocate_user_fields(class: &Class) -> TokenStream {
-    let field_idents = class.fields.iter().map(|f| &f.ident);
+/// Function used to instantiate a default version COM class


Not sure what this comment is trying to tell me. Version?

Whoops. This is a function used to create an instance of the class where all the user fields are default. We don't want to provide a normal Default implementation because that would allow the user to stack allocate the class which we don't want.

MarijnS95 · 2020-08-17T14:18:45Z

macros/support/src/class/class.rs

+        let default_constructor = super::class_constructor::generate_default(self);
+        let unsafe_constructor = super::class_constructor::generate_unsafe(self);


Similar to derive(Debug) a while ago not all user-specified members may have Default implemented; this currently makes it impossible to create a COM class with such members.

Would it be possible to emit these only when the class definition is annotated with #[derive(Default)]?

I think we don't want to allow annotating the class with #[derive(Default)] as either this creates a Default::default implementation which would allow for stack allocation (which we don't want to allow) or it doesn't which is confusing. I think we'll need some other attribute to indicate that your type is defaultable. This is needed for use with CoCreateInstance as the implementation of that function creates an instance of the class. I'll need to think more about this.

@MarijnS95 I added the ability to add a #[no_class_factory] attribute to your class. This removes the class factory generation (meaning you can't use CoCreateInstance) along with any need for your data members to be Default

I'll probably chain that into Linux support at some point as CoCreateInstance isn't available there anyway.

generate_unsafe is still there emitting an almost identical constructor to generate_default including forced defaults for members. I guess that should be behind has_class_factory as well, which is what you need this for?

Besides allocate has to return some type that implements the Interface trait, which is not the case for the class itself. This means it's currently impossible to receive Self (or a Pin<Box<Self>>) to interact with these members after constructing. Is that intended?

Oh whoops. Forgot to not return early from generate_unsafe if has_class_factory is false. Will change that.

Yes this is on purpose but that's usually what you want. Can you explain your use case? You want to be able to call a regular method on the class object without going through an interface?

I think putting deallocation in the Class trait would complicate things. You'd then have to make sure that all fields are still valid to keep around after deallocation. So for instance if your class had a Vec as one of its fields, you'd have to wrap it in a ManuallyDrop to get things to work. Right now, the class object only actually gets dropped when it should be deallocated.

I see, ClassAllocation::drop and the release function can't easily be generalized as the drop takes by mutable reference while release intentionally takes the ClassAllocation by value to implicitly call drop(). No way we should go back to wrapping all members in ManuallyDrop and that kind of shenanigans 😁

We could switch to atomics for the ref count, but I believe more work would be needed to ensure thread safety. Write now we assume that the class is not accessed from different threads. I'm not sure how we'd handle user defined fields that are not thread safe.

While tidying up the implementation (getting rid of get+set pairs) this might give a false sense of thread-safe security, your call.

I'm not sure if Self: Pin<Box> is entirely necessary though it is definitely more correct than &self since Self in that case is the class not the ClassAllocation. I'd prefer to keep it.

I wonder if query/query_interface/add_ref should worry about such a thing. Thanks to Deref (if it were to return &Self) it would anyway be lifetime-bound to a particular ClassAllocation instance. Removing it makes the thing segfault, so let's leave it in 🤷

I'll add a debug impl for the ClassAllocation.

@rylev the idea was to forward the formatting to inner in case it supports debugging (as per the code suggestion). Showing a raw pointer to the user seems rather pointless IMO, except for developers on com-rs itself.

The error is pretty helpful too, in case someone forgot to implement debug on their com class but try to dbg it:

error[E0277]: `wrapper::DxcIncludeHandlerWrapper` doesn't implement `std::fmt::Debug` --> src/wrapper.rs:270:9 | 270 | dbg!(&cls); | ^^^^^^^^^^^ `wrapper::DxcIncludeHandlerWrapper` cannot be formatted using `{:?}` | = help: the trait `std::fmt::Debug` is not implemented for `wrapper::DxcIncludeHandlerWrapper` = note: add `#[derive(Debug)]` or manually implement `std::fmt::Debug` = note: required because of the requirements on the impl of `std::fmt::Debug` for `com::production::ClassAllocation<wrapper::DxcIncludeHandlerWrapper>`

Unfortunately + ?std::fmt::Debug isn't a thing, otherwise we could have both. For now I'd suggest/request to implement Debug only when T implements it too.

Thar's fair. The implementation I included was actually very helpful for debugging the segfault issue we were seeing, but I can see how that's potentially not helpful in the general case where COM-rs is not buggy 😉.

@rylev I guess a ClassAllocation@0x<address> { <your struct> }?

But then we don't want that to segfault before the address has been printed 🤔

Or we keep it as is and add an AsRef impl for ClassAllocation so it's very easy to get a &T which can be debug printed when T: Debug. Either way this is now being tracked: #169

rylev · 2020-08-17T15:52:38Z

@kennykerr - I think this PR is ready to go. You had some concerns about naming, but that's not really a part of this PR (as that exists in master). Shall we merge this and open an issue for your concerns about names?

rylev · 2020-09-02T15:38:41Z

@kennykerr @MarijnS95 I'm going to merge this and add issues for the things we mentioned during the review:

Casing for COM methods (CamelCase for raw COM methods and snake_case for any pure Rust versions)
Passing cfg done into COM class methods.

MarijnS95 · 2020-09-02T19:59:35Z

@rylev Yeah it seems totally ready from my side, thanks for working through the remaining issues 🙂!

rylev added 5 commits August 11, 2020 14:33

Pin<Box<Class>>

568223b

query

0ac00bf

Drop all

8433067

Pin self when calling method

7726c09

Remove unnecessary calls to IUknown methods

3670285

rylev mentioned this pull request Aug 11, 2020

Provide mechanism for creating new boxed version of a COM class casted as a particular interface. #161

Closed

Add more docs to example

2c760f5

rylev requested a review from kennykerr August 11, 2020 15:20

Increase MSRV to 1.41.1

23dae7e

rylev commented Aug 12, 2020

View reviewed changes

rylev mentioned this pull request Aug 12, 2020

[Help] Using IOleObject interface #162

Closed

rylev added 2 commits August 13, 2020 13:15

Make stack allocation harder

1c1c773

Remove unneeded safe_query function

dbcc1ec

kennykerr reviewed Aug 14, 2020

View reviewed changes

MarijnS95 reviewed Aug 17, 2020

View reviewed changes

rylev and others added 2 commits August 17, 2020 16:42

Merge branch 'master' into class-improvements

6137c0d

no_class_factory

cc0677d

Return just the class on allocate

f079c35

kennykerr previously approved these changes Aug 25, 2020

View reviewed changes

rylev dismissed kennykerr’s stale review via 5b615ea September 1, 2020 13:06

rylev added 2 commits September 1, 2020 15:06

New allocation scheme

5b615ea

Add debug impl for ClassAllocation

0eecb16

rylev merged commit 69daf6e into master Sep 2, 2020

rylev deleted the class-improvements branch September 2, 2020 15:38

MarijnS95 mentioned this pull request Sep 2, 2020

Linux compatibility: Guard Windows-only modules with cfg macro #153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allocated Classes #163

Allocated Classes #163

rylev commented Aug 11, 2020

rylev Aug 12, 2020

rylev Aug 12, 2020

kennykerr Aug 12, 2020

rylev Aug 12, 2020

kennykerr Aug 12, 2020

rylev Aug 12, 2020

rylev Aug 13, 2020

kennykerr Aug 14, 2020

rylev Aug 17, 2020

kennykerr Aug 25, 2020

kennykerr Aug 14, 2020

rylev Aug 17, 2020

kennykerr Aug 14, 2020

rylev Aug 17, 2020

kennykerr Aug 25, 2020

kennykerr Aug 14, 2020

rylev Aug 17, 2020

kennykerr Aug 25, 2020

kennykerr Aug 14, 2020 •

edited

Loading

rylev Aug 17, 2020

MarijnS95 Aug 17, 2020

rylev Aug 17, 2020

rylev Aug 17, 2020

MarijnS95 Aug 17, 2020 •

edited

Loading

rylev Aug 18, 2020

MarijnS95 Sep 2, 2020

MarijnS95 Sep 2, 2020 •

edited

Loading

rylev Sep 7, 2020

MarijnS95 Sep 7, 2020

rylev Sep 7, 2020

rylev commented Aug 17, 2020

rylev commented Sep 2, 2020

MarijnS95 commented Sep 2, 2020


		// Get a `BritishShortHairCat` class factory
		let factory = get_class_object::<IClassFactory>(&CLSID_CAT_CLASS)

		let default_constructor = super::class_constructor::generate_default(self);
		let unsafe_constructor = super::class_constructor::generate_unsafe(self);

Allocated Classes #163

Allocated Classes #163

Conversation

rylev commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennykerr Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented Aug 17, 2020

rylev commented Sep 2, 2020

MarijnS95 commented Sep 2, 2020

kennykerr Aug 14, 2020 •

edited

Loading

MarijnS95 Aug 17, 2020 •

edited

Loading

MarijnS95 Sep 2, 2020 •

edited

Loading