Add basic implementation of the ComWrappers #653

kant2002 · 2021-02-07T11:17:01Z

This is attempt to resurrect dotnet/corert#8143
in order to improve status of #306

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

kant2002 · 2021-02-09T08:01:25Z

@jkotas I have future questions.
My understanding how ComWrappers currently works in CoreCLR is following.
When CCW information stored in the lazy storage called SyncBlock which stored in GC information about managed object, SyncBlock in itself has InteropSyncBlockInfo where actual CCW pointer would be stored. That's quite a machinery which I have to replicate. Okay, don't scary too much of that. But is there a way how this can be implemented in C# at least as POC without touching too much stuff? Should I replicate all that machinery, or that can be done using some global dictionary for example?

Other option is to extend runtime.lib to include necessary C++ code by modifying src\coreclr\nativeaot\Runtime\CMakeLists.txt and start with including src\coreclr\vm\interoplibinterface.cpp and when what else compilation errors require me to add.

jkotas · 2021-02-09T16:27:24Z

But is there a way how this can be implemented in C# at least as POC without touching too much stuff?

You can do that using ConditionalWeakTable, like it is done in #306 (comment).

ConditionalWeakTable and SyncBlocks are equivalent mechanisms. I think we would actually prefer ConditionalWeakTable as the permanent solution for this.

kant2002 · 2021-02-12T14:11:08Z

@jkotas I have what I would say very rough draft. I just trying understand do I move in proper direction or not?

jkotas · 2021-02-12T16:55:30Z

Yes, I think you are going in the right direction.

I think some of the internal and private "called by the runtime" methods you have copied over will have different shape at the end or won't be needed at all.

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

I have 2 ideas. CCW seems to be rathe rcompilcated. But RCW require just having Dictionary<IntrPtr, object> to keep track of RCWs. Most likely this should be some weekly referenced object, which should release COM object when no longer referenced. In fnalizer probably.

kant2002 · 2021-02-18T10:53:08Z

@jkotas do I still move in proper direction? I definitely still have some not needed flags, and what I assume I should do if I move in right direction

Implement MOW AddRef/Release/QI
Proper cleanup not in finalizer. Seems to be I should cleanup MOW on Release.
Populate Target with GC handle of wrapped runtime object from which MOW constructed. GcHandle.InternalAlloc
Release GC handle when object no longer used by COM

Questions

I assume that ComInterfaceInstance is partly what is ManagedObjectWrapper. Should I use only one structure?
What's better way for memmove at that level?

...reclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreRT.cs

jkotas

Yes, I think you are still moving in the right direction. I have left a bunch of comments.

I assume that ComInterfaceInstance is partly what is ManagedObjectWrapper. Should I use only one structure?

Yes, they look the same. It would make sense to use only one structure.

What's better way for memmove at that level?

We prefer to use Span.CopyTo where posible instead of memmove.

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

More types for memory layout.

kant2002 · 2021-02-19T12:18:25Z

@jkotas I think I more or less finish with COM creation.
What is not clear for me, how I should cleanup proxy objects created for COM.

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

src/coreclr/nativeaot/Directory.Build.props

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

MichalStrehovsky · 2021-04-09T07:35:51Z

@MichalStrehovsky while Jan is busy with work or life

Jan is on vacation :).

It seems that Type.GUID do not preserved in any kind on interfaces, or ...?

RuntimeBlockedTypeInfo

This is what we discussed in #802. You can delete __BlockAllReflectionAttribute from the test to unblock yourself right now.

src/tests/nativeaot/SmokeTests/PInvoke/PInvokeNative.cpp

src/tests/nativeaot/SmokeTests/ComWrappers/ComWrappersNative.cpp

kant2002 · 2021-04-17T08:34:41Z

@jkotas is this comments all what you have? I'm done with comments. Hope you enjoy your vacation.

jkotas · 2021-04-17T13:52:00Z

...eclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/InteropHelpers.cs

        {
            if (pUnk == null)
            {
                return IntPtr.Zero;
            }

+#if TARGET_WINDOWS
+#pragma warning disable CA1416
+            Guid interfaceGuid = convertToType.GUID;


It would be more inline with the NativeAOT design printiples for the GUID to be passed as an argument into this method by the toolchain instead of computing it at runtime via reflection.

@MichalStrehovsky Do you expect that there are going to be special measures required to make the interface reflectable going forward, or is this going to just work?

Using reflection to fetch the GUID here is also going to make the marshalling much slower compared to regular CLR.

I think the design we will have to settle with is that types are always reflectable, so fetching a GUID should be reliable, but yes, this is not going to be very fast.

It shouldn't be too much of a problem to change this signature of this helper to take an actual GUID, and in the compiler:

Use .GetDecodedCustomAttributes to get the attributes on the type.

Parse the string into a GUID.

Emit IL to call the Guid(Int32, Int16, Int16, Byte, Byte, Byte, Byte, Byte, Byte, Byte, Byte) constructor with the parsed GUID values.

We can scope out types that don't have a GuidAttribute for now (runtime reflection would not compute the correct GUID for them either).

@MichalStrehovsky I'm not sure that I properly understand how to jump from TypeDesc to EcmaType.
I plan to use following

CustomAttributeValue<TypeDesc>? customAttributeValue = ((EcmaType)this.ManagedParameterType.GetTypeDefinition()).GetDecodedCustomAttribute("", "GuidAttribute");

(this.ManagedParameterType as EcmaType)?.GetDecodedCustomAttribute("System.Runtime.InteropServices", "GuidAttribute")

The attribute namespace has to be specified

GetTypeDefinition would be only needed for generic types. Built-in COM interop does not work for generic types so you can just skip that part.

jkotas · 2021-04-17T13:57:10Z

is this comments all what you have?

Two comments left:

Fetching GUID via reflection Add basic implementation of the ComWrappers #653 (comment)
Incomplete PreserveSig change https://github.com/dotnet/runtimelab/pull/653/files#r592627769

MichalStrehovsky

This looks great, thank you!

MichalStrehovsky · 2021-04-18T07:28:30Z

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

+    {
+        public static int Main(string[] args)
+        {
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


I think if you put <CLRTestTargetUnsupported Condition="'$(TargetsWindows)' != 'true'">true</CLRTestTargetUnsupported> in the csproj, this test will be skipped. It's better than AOT compiling it only for the test to do nothing.

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

src/tests/nativeaot/SmokeTests/ComWrappers/ComWrappers.csproj

MichalStrehovsky · 2021-04-18T07:56:31Z

...eclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/InteropHelpers.cs

        {
            if (pUnk == null)
            {
                return IntPtr.Zero;
            }

+#if TARGET_WINDOWS
+#pragma warning disable CA1416
+            Guid interfaceGuid = convertToType.GUID;


I think the design we will have to settle with is that types are always reflectable, so fetching a GUID should be reliable, but yes, this is not going to be very fast.

It shouldn't be too much of a problem to change this signature of this helper to take an actual GUID, and in the compiler:

Use .GetDecodedCustomAttributes to get the attributes on the type.

Parse the string into a GUID.

Emit IL to call the Guid(Int32, Int16, Int16, Byte, Byte, Byte, Byte, Byte, Byte, Byte, Byte) constructor with the parsed GUID values.

We can scope out types that don't have a GuidAttribute for now (runtime reflection would not compute the correct GUID for them either).

Co-authored-by: Michal Strehovský <[email protected]>

jkotas · 2021-04-20T14:06:02Z

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

+            var int32Type = Context.GetWellKnownType(WellKnownType.Int32);
+            var int16Type = Context.GetWellKnownType(WellKnownType.Int16);
+            var byteType = Context.GetWellKnownType(WellKnownType.Byte);
+            guidType.GetParameterlessConstructor();


Suggested change

guidType.GetParameterlessConstructor();

jkotas · 2021-04-20T15:14:54Z

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

+            var int16Type = Context.GetWellKnownType(WellKnownType.Int16);
+            var byteType = Context.GetWellKnownType(WellKnownType.Byte);
+            guidType.GetParameterlessConstructor();
+            int param1 = int.Parse(parts[0], System.Globalization.NumberStyles.HexNumber);


It would be simpler and faster to do:

byte[] bytes = Guid.Parse(guidValue); codeStream.EmitLdc(BinaryPrimitives.ReadInt32LittleEndian(bytes)); codeStream.EmitLdc(BinaryPrimitives.ReadInt16LittleEndian(bytes.Slice(4))); codeStream.EmitLdc(BinaryPrimitives.ReadInt16LittleEndian(bytes.Slice(6))); for (int i = 8; i < 16; i++) codeStream.EmitLdc(bytes[i]);

kant2002 · 2021-04-21T02:00:45Z

@jkotas ready for you

jkotas

Thank you for all you hard work on this!

kant2002 · 2021-04-21T02:29:22Z

I do not believe my eyes that this is happens :)

jkotas reviewed Feb 7, 2021

View reviewed changes

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

jkotas reviewed Feb 7, 2021

View reviewed changes

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

kant2002 force-pushed the kant/comwrappers branch from 476636a to 9c5744a Compare February 8, 2021 17:43

jkotas reviewed Feb 16, 2021

View reviewed changes

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

jkotas reviewed Feb 16, 2021

View reviewed changes

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

kant2002 added 5 commits February 18, 2021 13:33

Add basic implementation of the ComWrappers

28bf64b

Fixes after refactoring in dotnet/runtime

d8daea1

Rough attempt to move forward. Attempt to create CCW

3328c6d

Move configuration for FeatureComWrappers outside of project file

f8165c6

kant2002 force-pushed the kant/comwrappers branch from 31f8365 to f8165c6 Compare February 18, 2021 07:38

More implementation for the MOW

fb2233f

jkotas reviewed Feb 18, 2021

View reviewed changes

...reclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreRT.cs Outdated Show resolved Hide resolved

jkotas reviewed Feb 18, 2021

View reviewed changes

kant2002 added 4 commits February 19, 2021 14:11

Slightly more changes based on feedback.

e35695b

More types for memory layout.

Delagate IUnknown implementation to MOW

868b1bf

Implement IUnknown interface for CCW

017e907

Return proper pointer to IUnknown

d4e3188

jkotas reviewed Feb 19, 2021

View reviewed changes

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

jkotas reviewed Feb 19, 2021

View reviewed changes

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

jkotas reviewed Feb 19, 2021

View reviewed changes

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

jkotas reviewed Feb 19, 2021

View reviewed changes

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

jkotas reviewed Feb 19, 2021

View reviewed changes

src/coreclr/nativeaot/Directory.Build.props Outdated Show resolved Hide resolved

jkotas reviewed Feb 19, 2021

View reviewed changes

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

charlesroddie mentioned this pull request Apr 10, 2021

Question: .NET 5 UWP apps are already possible, so when will official support arrive? microsoft/WindowsAppSDK#105

Closed

kant2002 added 2 commits April 16, 2021 19:42

Create separate tests for ComWrappers feature

3b2848a

Fix Pinvoke name of the DLL with test functions

87213bb

jkotas reviewed Apr 17, 2021

View reviewed changes

src/tests/nativeaot/SmokeTests/PInvoke/PInvokeNative.cpp Outdated Show resolved Hide resolved

jkotas reviewed Apr 17, 2021

View reviewed changes

src/tests/nativeaot/SmokeTests/ComWrappers/ComWrappersNative.cpp Outdated Show resolved Hide resolved

Fix code review comments

acfaca8

jkotas reviewed Apr 17, 2021

View reviewed changes

Remove incomplete PreserveSig implementation

ecbd14e

MichalStrehovsky reviewed Apr 18, 2021

View reviewed changes

kant2002 and others added 6 commits April 18, 2021 14:37

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

3dce725

Co-authored-by: Michal Strehovský <[email protected]>

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

359c53d

Co-authored-by: Michal Strehovský <[email protected]>

Update src/tests/nativeaot/SmokeTests/ComWrappers/ComWrappers.csproj

e761497

Co-authored-by: Michal Strehovský <[email protected]>

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

5a6d6eb

Co-authored-by: Michal Strehovský <[email protected]>

Fix compilation errors

716d585

Read GUID directly from metadata instead of relying on Reflection

218a300

jkotas reviewed Apr 20, 2021

View reviewed changes

kant2002 added 6 commits April 20, 2021 21:26

Remove leftover

e7b0b59

Improve GUID creation

bdba4ea

Fix stupid compilation error

1db71ff

Add missing using

2f826ce

Another round of stupidity

0b90983

One more try

f9acd5c

jkotas approved these changes Apr 21, 2021

View reviewed changes

jkotas merged commit 4ab2b2b into dotnet:feature/NativeAOT Apr 21, 2021

kant2002 deleted the kant/comwrappers branch April 21, 2021 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic implementation of the ComWrappers #653

Add basic implementation of the ComWrappers #653

kant2002 commented Feb 7, 2021 •

edited

Loading

kant2002 commented Feb 9, 2021

jkotas commented Feb 9, 2021 •

edited

Loading

kant2002 commented Feb 12, 2021

jkotas commented Feb 12, 2021

kant2002 commented Feb 18, 2021

jkotas left a comment

kant2002 commented Feb 19, 2021

MichalStrehovsky commented Apr 9, 2021

kant2002 commented Apr 17, 2021

jkotas Apr 17, 2021

jkotas Apr 17, 2021

MichalStrehovsky Apr 18, 2021

kant2002 Apr 18, 2021

jkotas Apr 18, 2021

jkotas commented Apr 17, 2021 •

edited

Loading

MichalStrehovsky left a comment

MichalStrehovsky Apr 18, 2021

MichalStrehovsky Apr 18, 2021

jkotas Apr 20, 2021

jkotas Apr 20, 2021

kant2002 commented Apr 21, 2021

jkotas left a comment

kant2002 commented Apr 21, 2021

Add basic implementation of the ComWrappers #653

Add basic implementation of the ComWrappers #653

Conversation

kant2002 commented Feb 7, 2021 • edited Loading

kant2002 commented Feb 9, 2021

jkotas commented Feb 9, 2021 • edited Loading

kant2002 commented Feb 12, 2021

jkotas commented Feb 12, 2021

kant2002 commented Feb 18, 2021

jkotas left a comment

Choose a reason for hiding this comment

kant2002 commented Feb 19, 2021

MichalStrehovsky commented Apr 9, 2021

kant2002 commented Apr 17, 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

jkotas commented Apr 17, 2021 • edited Loading

MichalStrehovsky left a 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

kant2002 commented Apr 21, 2021

jkotas left a comment

Choose a reason for hiding this comment

kant2002 commented Apr 21, 2021

kant2002 commented Feb 7, 2021 •

edited

Loading

jkotas commented Feb 9, 2021 •

edited

Loading

jkotas commented Apr 17, 2021 •

edited

Loading