Add RCW support #992

kant2002 · 2021-04-21T07:39:16Z

See #306

Implemented RCW creation for COM interfaces.
Implemented returning original object, if COM interface is ManagedObjectWrapper
Do not return original object if unique instance requested.

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

jkotas · 2021-04-21T13:24:24Z

Add some basic tests?

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

src/tests/nativeaot/SmokeTests/ComWrappers/ComWrappers.cs

kant2002 · 2021-04-26T05:48:39Z

@jkotas I am definitely ready for you to review this.

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

jkotas · 2021-04-26T13:11:39Z

Add CCW support

This shold say RCW. I had it swapped in the tracking issue - I have fixed it there.

kant2002 · 2021-04-26T14:41:11Z

I had it swapped in the tracking issue - I have fixed it there.

Thanks. It was a big linguistic battle inside me. Since I have a lot of other question, in order to not too much distract you, I decide that I do not understand some language tricks.

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

jkotas · 2021-04-28T05:15:17Z

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

+
+            ~NativeObjectWrapper()
+            {
+                _comWrappers._rcwCache.Remove(_externalComObject);


Does this need to take the lock?

jkotas · 2021-04-28T05:19:03Z

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

@@ -317,7 +349,7 @@ public object GetOrRegisterObjectForComInstance(IntPtr externalComObject, Create
        /// <remarks>
        /// If <paramref name="impl" /> is <c>null</c>, the global instance (if registered) will be used.
        /// </remarks>
-        private static bool TryGetOrCreateObjectForComInstanceInternal(
+        private static unsafe bool TryGetOrCreateObjectForComInstanceInternal(
            ComWrappers impl,


Can this be an instance method instead? I do not see the point of passing in impl explicitly. (CreateCCW has the same issue.)

jkotas · 2021-04-28T05:23:28Z

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

+            {
+                if (impl._rcwCache.TryGetValue(externalComObject, out var existingHandle))
+                {
+                    if (existingHandle.IsAllocated)


IsAllocated should be always true here. We should not ever get unallocated GCHandle from the Dictionary.

jkotas · 2021-04-28T05:26:05Z

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

+                }
+                else
+                {
+                    GCHandle proxyHandle = GCHandle.Alloc(retValue, GCHandleType.Weak);


For symetry, it would be nice to allocated the GCHandle and add it to the table in NativeObjectWrapper constructor - since the opposite is done in the NativeObjectWrapper finalizer.

kant2002 · 2021-04-28T05:32:22Z

@jkotas I think I address your feedback.

jkotas · 2021-04-28T05:40:30Z

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

-            public delegate* unmanaged<IntPtr, uint> Release;
+            private IntPtr _externalComObject;
+            private ComWrappers _comWrappers;
+            public GCHandle ProxyHandle;


I would keep it as _proxyHandle. (Some of the places in the file are actually still refering to the field as _proxyHandle.)

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

jkotas · 2021-04-28T05:41:23Z

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

+            ~NativeObjectWrapper()
+            {
+                _comWrappers.RemoveRCWFromCache(_externalComObject);
+                _proxyHandle.Free();


This can be unallocated handle if GCHandle.Alloc in the constructor fails. This should get the IsAllocated check.

…e/InteropServices/ComWrappers.CoreRT.cs Co-authored-by: Jan Kotas <[email protected]>

kant2002 · 2021-04-28T06:01:39Z

@jkotas I think I address your feedback again

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

…e/InteropServices/ComWrappers.CoreRT.cs Co-authored-by: Jan Kotas <[email protected]>

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs

…e/InteropServices/ComWrappers.CoreRT.cs

jkotas · 2021-04-29T03:11:15Z

src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.Aot.cs

+
+            StoreManagedValue(codeStream);
+
+            codeStream.EmitLabel(lNull);


Is it correct to skip storing the managed value for null?

I think yes. At least this pattern is everywhere.

Managed value initialized as null by runtime guarantee, so load/store usually going in pairs. Then somebody do actual return. At least this is how I understand this.

Managed value initialized as null by runtime guarantee,

Where is it guaranteed?

I went through all TransformNativeToManaged methods and all of them end with Store. I have not found one that skips the store.

LayoutClassPtrMarshaller, AsAnyMarshaller for example

The managed value for these marshallers is a pointer to the structure that they are marshalling. It is not the case here.

What I was trying to say that

.method public hidebysig specialname instance int32 get_Set() cil managed { .maxstack 1 .locals init (int32 V_0) IL_0009: ldloc.0 IL_000a: ret } // end of method SetProperty::get_Set

is equivalent for

int Set => 0;

I miss your previous message. Will have to check what kind of IL actually generated to validate that what I'm saying is correct.

You may be right, but I would still simplify this to a simple ConvertNativeComInterfaceToManaged. If the store is redudandant, the JIT will eliminate it after inlining ConvertNativeComInterfaceToManaged.

jkotas · 2021-04-29T03:14:19Z

LGTM otherwise. Thank you!

src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.Aot.cs

Add RCW support

77e92a1

kant2002 changed the title ~~Add RCW support~~ Add CCW support Apr 21, 2021

Fix merge errors

037a6e0

jkotas reviewed Apr 21, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

jkotas reviewed Apr 21, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

kant2002 added 3 commits April 21, 2021 20:59

Add basic tests

c964db1

Rearrange override

aabb326

Fix compilation errrors

d5a24b0

kant2002 mentioned this pull request Apr 23, 2021

ComWrappers API missing de-registration method dotnet/runtime#51730

Closed

Do not register ComWrappers again.

022aa19

jkotas reviewed Apr 23, 2021

View reviewed changes

src/tests/nativeaot/SmokeTests/ComWrappers/ComWrappers.cs Outdated Show resolved Hide resolved

kant2002 added 5 commits April 25, 2021 19:52

Make test for CCW creation pass.

9924646

Add RCW roundtrip

421c24d

Ensure that UniqueInstance flag respected during CCW creation

e601d36

Add test that RCW non retreived from cache

736c489

Fix typo

cac937a

jkotas reviewed Apr 26, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

jkotas reviewed Apr 26, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

kant2002 changed the title ~~Add CCW support~~ Add RCW support Apr 26, 2021

Add PR feedback

499a54c

jkotas reviewed Apr 26, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

kant2002 added 3 commits April 27, 2021 17:08

Add roundtrip

a5fd897

Release wrapper from cache

d7730dc

Add implementation for some Marshal API

927faa7

jkotas reviewed Apr 27, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

Clear all items in the rcwCache

b1a7cb1

kant2002 added 3 commits April 28, 2021 01:31

Fix typo

324735a

Stupid error

fa93c47

Do not use handle for storing reference to ComWrappers

deef189

jkotas reviewed Apr 28, 2021

View reviewed changes

kant2002 added 2 commits April 28, 2021 11:29

Fix PR feedback

4f37712

Remove checks for handle allocation

83ad777

jkotas reviewed Apr 28, 2021

View reviewed changes

kant2002 and others added 3 commits April 28, 2021 11:53

Update src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtim…

dcb171f

…e/InteropServices/ComWrappers.CoreRT.cs Co-authored-by: Jan Kotas <[email protected]>

Address feedback

3e0578d

Add check for allocation status of handle.

c3b8a0b

Fix compilation error

19fc11f

jkotas reviewed Apr 28, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Show resolved Hide resolved

kant2002 and others added 2 commits April 29, 2021 08:47

Update src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtim…

0152f01

…e/InteropServices/ComWrappers.CoreRT.cs Co-authored-by: Jan Kotas <[email protected]>

Reorder construction to shield AddRef from GCHandle failures

fb48e20

jkotas reviewed Apr 29, 2021

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreRT.cs Outdated Show resolved Hide resolved

Update src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtim…

68050e9

…e/InteropServices/ComWrappers.CoreRT.cs

jkotas reviewed Apr 29, 2021

View reviewed changes

src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.Aot.cs Outdated Show resolved Hide resolved

kant2002 added 3 commits April 29, 2021 09:21

Remove not used method

a5e8175

Fix compilation error

967d2be

Remove check for null

c237a31

jkotas approved these changes Apr 29, 2021

View reviewed changes

jkotas merged commit c09295d into dotnet:feature/NativeAOT Apr 29, 2021

kant2002 deleted the kant/com-rcw branch April 29, 2021 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RCW support #992

Add RCW support #992

kant2002 commented Apr 21, 2021 •

edited

Loading

jkotas commented Apr 21, 2021

kant2002 commented Apr 26, 2021

jkotas commented Apr 26, 2021

kant2002 commented Apr 26, 2021

jkotas Apr 28, 2021

jkotas Apr 28, 2021

jkotas Apr 28, 2021

jkotas Apr 28, 2021

kant2002 commented Apr 28, 2021

jkotas Apr 28, 2021

kant2002 Apr 28, 2021

jkotas Apr 28, 2021

kant2002 Apr 28, 2021

kant2002 commented Apr 28, 2021

jkotas Apr 29, 2021

kant2002 Apr 29, 2021

kant2002 Apr 29, 2021

jkotas Apr 29, 2021

kant2002 Apr 29, 2021

jkotas Apr 29, 2021

kant2002 Apr 29, 2021

kant2002 Apr 29, 2021

jkotas Apr 29, 2021

jkotas commented Apr 29, 2021

Add RCW support #992

Add RCW support #992

Conversation

kant2002 commented Apr 21, 2021 • edited Loading

jkotas commented Apr 21, 2021

kant2002 commented Apr 26, 2021

jkotas commented Apr 26, 2021

kant2002 commented Apr 26, 2021

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

kant2002 commented Apr 28, 2021

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

kant2002 commented Apr 28, 2021

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

jkotas commented Apr 29, 2021

kant2002 commented Apr 21, 2021 •

edited

Loading