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

Add opt-in support for GeneratedComInterface/ComImport RCW interop #87583

Merged
merged 21 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4c34ffb
Add initial implementation of GeneratedComInterface/ComImport RCW int…
jkoritzinsky Jun 13, 2023
34f4120
Actually define the .override entry for the dynamically emitted imple…
jkoritzinsky Jun 14, 2023
5500bde
Interfaces don't have parents.
jkoritzinsky Jun 14, 2023
bb808ac
Add basic test for the RCW interop scenario and fix various IL emit i…
jkoritzinsky Jun 14, 2023
9d09747
Suppress a diagnostic that fixing would make the test useless.
jkoritzinsky Jun 14, 2023
971a2a5
Add a linker test and a feature flag to ensure that the ComImport int…
jkoritzinsky Jun 14, 2023
dcd4402
Add negative test
jkoritzinsky Jun 14, 2023
5406981
PR feedback and fix build.
jkoritzinsky Jun 16, 2023
b058b40
Merge branch 'main' into com-adoption-configuration-rcw
jkoritzinsky Jun 19, 2023
d0c0b3e
Update the trimming test project template to allow unsafe blocks and …
jkoritzinsky Jun 20, 2023
9e915fc
Merge branch 'main' into com-adoption-configuration-rcw
jkoritzinsky Jun 21, 2023
670799a
Update src/libraries/System.Runtime.InteropServices/src/System/Runtim…
jkoritzinsky Jun 23, 2023
af91b01
PR feedback.
jkoritzinsky Jun 28, 2023
3124755
Remove suppression and fix fallback for COM interop flag.
jkoritzinsky Jun 28, 2023
cc068b3
Disable the warning with a pragma again for Vitek.
jkoritzinsky Jun 29, 2023
e680980
Merge branch 'main' into com-adoption-configuration-rcw
jkoritzinsky Jun 29, 2023
7f36032
Change back to an unconditional suppress to get the PR green again.
jkoritzinsky Jul 12, 2023
c42589d
Implement feedback on form factor attributes.
jkoritzinsky Jul 13, 2023
682ba71
Refactor strategy selection to avoid non-windows failures.
jkoritzinsky Jul 13, 2023
35f25a8
Redo how we set suppressions to make sure they always are "needed".
jkoritzinsky Jul 14, 2023
3b5ae8b
PR feedback and handle custom modifiers.
jkoritzinsky Jul 17, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<linker>
<assembly fullname="System.Runtime.InteropServices">
<type fullname="System.Runtime.InteropServices.Marshalling.ComObject" feature="System.Runtime.InteropServices.Marshalling.EnableGeneratedComInterfaceComImportInterop" featurevalue="false">
<method signature="System.Boolean get_ComImportInteropEnabled()" body="stub" value="false" />
</type>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
<Compile Include="System\Runtime\InteropServices\ImportedFromTypeLibAttribute.cs" />
<Compile Include="System\Runtime\InteropServices\ManagedToNativeComInteropStubAttribute.cs" />
<Compile Include="System\Runtime\InteropServices\Marshalling\ComExposedClassAttribute.cs" />
<Compile Include="System\Runtime\InteropServices\Marshalling\ComImportInteropInterfaceDetailsStrategy.cs" />
<Compile Include="System\Runtime\InteropServices\Marshalling\UniqueComInterfaceMarshaller.cs" />
<Compile Include="System\Runtime\InteropServices\Marshalling\ComInterfaceMarshaller.cs" />
<Compile Include="System\Runtime\InteropServices\Marshalling\ComObject.cs" />
Expand Down Expand Up @@ -71,6 +72,10 @@
<Compile Include="$(CommonPath)System\Obsoletions.cs" Link="Common\System\Obsoletions.cs" />
</ItemGroup>

<ItemGroup>
<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.xml" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="$(CoreLibProject)" />
<ProjectReference Include="$(LibrariesProjectRoot)System.Collections.Concurrent\src\System.Collections.Concurrent.csproj" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.CompilerServices;

namespace System.Runtime.InteropServices.Marshalling
{
[RequiresDynamicCode("Enabling interop between source-generated and runtime-generated COM requires dynamic code generation.")]
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
internal sealed class ComImportInteropInterfaceDetailsStrategy : IIUnknownInterfaceDetailsStrategy
{
public static readonly IIUnknownInterfaceDetailsStrategy Instance = new ComImportInteropInterfaceDetailsStrategy();

private readonly ConditionalWeakTable<Type, Type> _forwarderInterfaceCache = new();

// TODO: Support exposing ComImport interfaces through StrategyBasedComWrappers?
public IComExposedDetails? GetComExposedTypeDetails(RuntimeTypeHandle type) => DefaultIUnknownInterfaceDetailsStrategy.Instance.GetComExposedTypeDetails(type);

[UnconditionalSuppressMessage("Trimming", "IL2065", Justification = "Runtime-based COM interop is not supported with trimming enabled.")]
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
[UnconditionalSuppressMessage("Trimming", "IL2067", Justification = "Runtime-based COM interop is not supported with trimming enabled.")]
[UnconditionalSuppressMessage("Trimming", "IL2070", Justification = "Runtime-based COM interop is not supported with trimming enabled.")]
[UnconditionalSuppressMessage("Trimming", "IL2075", Justification = "Runtime-based COM interop is not supported with trimming enabled.")]
public IIUnknownDerivedDetails? GetIUnknownDerivedDetails(RuntimeTypeHandle type)
{
Type runtimeType = Type.GetTypeFromHandle(type)!;
if (!runtimeType.IsImport)
{
return DefaultIUnknownInterfaceDetailsStrategy.Instance.GetIUnknownDerivedDetails(type);
}

Type implementationType = _forwarderInterfaceCache.GetValue(runtimeType, runtimeType =>
{
AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("ComImportForwarder"), AssemblyBuilderAccess.RunAndCollect);
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("ComImportForwarder"), AssemblyBuilderAccess.RunAndCollect);
AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("ComImportForwarder"), runtimeType.IsCollectible ? AssemblyBuilderAccess.RunAndCollect : AssemblyBuilderAccess.Run);

This should reduce the overhead quite a bit for typical non-collectible use case.

ModuleBuilder module = assembly.DefineDynamicModule("ComImportForwarder");

ConstructorInfo ignoresAccessChecksToAttributeConstructor = GetIgnoresAccessChecksToAttributeConstructor(module);

assembly.SetCustomAttribute(new CustomAttributeBuilder(ignoresAccessChecksToAttributeConstructor, new object[] { typeof(IComImportAdapter).Assembly.GetName().Name! }));

TypeBuilder implementation = module.DefineType("InterfaceForwarder", TypeAttributes.Interface | TypeAttributes.Abstract, parent: null, interfaces: runtimeType.GetInterfaces());
implementation.AddInterfaceImplementation(runtimeType);
implementation.SetCustomAttribute(new CustomAttributeBuilder(typeof(DynamicInterfaceCastableImplementationAttribute).GetConstructor(Array.Empty<Type>())!, Array.Empty<object>()));

foreach (Type iface in implementation.GetInterfaces())
{
assembly.SetCustomAttribute(new CustomAttributeBuilder(ignoresAccessChecksToAttributeConstructor, new object[] { iface.Assembly.GetName().Name! }));
foreach (MethodInfo method in iface.GetMethods())
{
ParameterInfo[] parameters = method.GetParameters();
var parameterTypes = new Type[parameters.Length];
for (int i = 0; i < parameters.Length; i++)
{
parameterTypes[i] = parameters[i].ParameterType;
}
MethodBuilder builder = implementation.DefineMethod(method.Name, MethodAttributes.Private | MethodAttributes.Final | MethodAttributes.HideBySig | MethodAttributes.Virtual, method.ReturnType, parameterTypes);
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
ILGenerator il = builder.GetILGenerator();
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Castclass, typeof(IComImportAdapter));
il.Emit(OpCodes.Callvirt, IComImportAdapter.GetRuntimeCallableWrapperMethod);
il.Emit(OpCodes.Castclass, iface);
for (int i = 0; i < parameters.Length; i++)
{
il.Emit(OpCodes.Ldarg, i + 1);
}
il.Emit(OpCodes.Callvirt, method);
il.Emit(OpCodes.Ret);
implementation.DefineMethodOverride(builder, method);
}
}

return implementation.CreateType();
});

return new ComImportDetails(runtimeType.GUID, implementationType);
}

private static ConstructorInfo GetIgnoresAccessChecksToAttributeConstructor(ModuleBuilder moduleBuilder)
{
var magicAttribute = EmitIgnoresAccessChecksToAttribute(moduleBuilder);
return magicAttribute.GetConstructor(new Type[] { typeof(string) })!;
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
}

[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]
private static Type EmitIgnoresAccessChecksToAttribute(ModuleBuilder moduleBuilder)
{
var tb = moduleBuilder.DefineType(
"System.Runtime.CompilerServices.IgnoresAccessChecksToAttribute",
TypeAttributes.NotPublic,
typeof(Attribute));

var attributeUsage = new CustomAttributeBuilder(
s_attributeUsageCtor,
new object[] { AttributeTargets.Assembly },
new PropertyInfo[] { s_attributeUsageAllowMultipleProperty },
new object[] { false });
tb.SetCustomAttribute(attributeUsage);

var cb = tb.DefineConstructor(
MethodAttributes.Public |
MethodAttributes.HideBySig |
MethodAttributes.SpecialName |
MethodAttributes.RTSpecialName,
CallingConventions.Standard,
new Type[] { typeof(string) });
cb.DefineParameter(1, ParameterAttributes.None, "assemblyName");

var il = cb.GetILGenerator();
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Call, s_attributeBaseClassCtor);
il.Emit(OpCodes.Ret);

return tb.CreateType()!;
}

/// <summary>
/// The <see cref="Attribute()"/> constructor.
/// </summary>
private static readonly ConstructorInfo s_attributeBaseClassCtor = typeof(Attribute).GetConstructors(BindingFlags.NonPublic | BindingFlags.Instance)[0];

/// <summary>
/// The <see cref="AttributeUsageAttribute(AttributeTargets)"/> constructor.
/// </summary>
private static readonly ConstructorInfo s_attributeUsageCtor = typeof(AttributeUsageAttribute).GetConstructor(new Type[] { typeof(AttributeTargets) })!;

/// <summary>
/// The <see cref="AttributeUsageAttribute.AllowMultiple"/> property.
/// </summary>
private static readonly PropertyInfo s_attributeUsageAllowMultipleProperty = typeof(AttributeUsageAttribute).GetProperty(nameof(AttributeUsageAttribute.AllowMultiple))!;

private sealed class ComImportDetails(Guid iid, Type implementation) : IIUnknownDerivedDetails
{
public Guid Iid { get; } = iid;

public Type Implementation { get; } = implementation;

public unsafe void** ManagedVirtualMethodTable => null;
}

internal interface IComImportAdapter
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
{
internal static readonly MethodInfo GetRuntimeCallableWrapperMethod = typeof(IComImportAdapter).GetMethod(nameof(GetRuntimeCallableWrapper))!;

object GetRuntimeCallableWrapper();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,22 @@
// This API need to be exposed to implement the COM source generator in one form or another.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.Versioning;

namespace System.Runtime.InteropServices.Marshalling
{
/// <summary>
/// Base class for all COM source generated Runtime Callable Wrapper (RCWs).
/// </summary>
public sealed unsafe class ComObject : IDynamicInterfaceCastable, IUnmanagedVirtualMethodTableProvider
public sealed unsafe class ComObject : IDynamicInterfaceCastable, IUnmanagedVirtualMethodTableProvider, ComImportInteropInterfaceDetailsStrategy.IComImportAdapter
{
internal static bool ComImportInteropEnabled { get; } = AppContext.TryGetSwitch("System.Runtime.InteropServices.Marshalling.EnableGeneratedComInterfaceComImportInterop", out bool enabled) ? enabled : false;
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved

private readonly void* _instancePointer;

private readonly object? _runtimeCallableWrapper;
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Initialize ComObject instance.
/// </summary>
Expand All @@ -28,6 +34,10 @@ internal ComObject(IIUnknownInterfaceDetailsStrategy interfaceDetailsStrategy, I
IUnknownStrategy = iunknownStrategy;
CacheStrategy = cacheStrategy;
_instancePointer = IUnknownStrategy.CreateInstancePointer(thisPointer);
if (OperatingSystem.IsWindows() && ComImportInteropEnabled)
{
_runtimeCallableWrapper = Marshal.GetObjectForIUnknown((nint)thisPointer);
}
}

~ComObject()
Expand Down Expand Up @@ -135,5 +145,10 @@ VirtualMethodTableInfo IUnmanagedVirtualMethodTableProvider.GetVirtualMethodTabl

return new(result.ThisPtr, result.Table);
}

object ComImportInteropInterfaceDetailsStrategy.IComImportAdapter.GetRuntimeCallableWrapper()
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
{
return _runtimeCallableWrapper!;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return _runtimeCallableWrapper!;
Debug.Assert(_runtimeCallableWrapper != null);
return _runtimeCallableWrapper;

Is this guaranteed to be non-zero when this method is called? If it is the case, Debug.Assert would be better than !.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.Versioning;

namespace System.Runtime.InteropServices.Marshalling
{
Expand All @@ -17,7 +20,15 @@ public class StrategyBasedComWrappers : ComWrappers

protected static IIUnknownCacheStrategy CreateDefaultCacheStrategy() => new DefaultCaching();

protected virtual IIUnknownInterfaceDetailsStrategy GetOrCreateInterfaceDetailsStrategy() => DefaultIUnknownInterfaceDetailsStrategy;
[UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "The member with the attribute is in a IsDynamicCodeSupported block.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird - you should not need this - you would need the pragma, but you should not need UnconditionalSuppressMessage - that means the feature switch is not working correctly for some reason.
Do you get warnings from trimmer/AOT compiler if the feature switch is off ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I get the warning without applying it.

protected virtual IIUnknownInterfaceDetailsStrategy GetOrCreateInterfaceDetailsStrategy()
{
if (RuntimeFeature.IsDynamicCodeSupported && OperatingSystem.IsWindows() && ComObject.ComImportInteropEnabled)
{
return ComImportInteropInterfaceDetailsStrategy.Instance;
}
return DefaultIUnknownInterfaceDetailsStrategy;
}

protected virtual IIUnknownStrategy GetOrCreateIUnknownStrategy() => DefaultIUnknownStrategy;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<!-- These tests pull the attributes from Ancillary.Interop, so we don't need to include the attribute sources in this assembly. -->
<IncludeLibraryImportGeneratorSources>false</IncludeLibraryImportGeneratorSources>
<ReferencesNativeExports>true</ReferencesNativeExports>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
</PropertyGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.InteropServices;
using Microsoft.DotNet.RemoteExecutor;
using SharedTypes.ComInterfaces;
using Xunit;

namespace ComInterfaceGenerator.Tests
{
[ConditionalClass(typeof(GeneratedComInterfaceComImportInteropTests), nameof(IsSupported))]
public unsafe partial class GeneratedComInterfaceComImportInteropTests
{
public static bool IsSupported =>
RemoteExecutor.IsSupported
&& PlatformDetection.IsWindows
&& PlatformDetection.IsNotMonoRuntime
&& PlatformDetection.IsNotNativeAot;

[LibraryImport(NativeExportsNE.NativeExportsNE_Binary, EntryPoint = "new_get_and_set_int")]
private static partial IGetAndSetInt NewNativeObject();

[ComImport]
[Guid(_guid)]
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
[SuppressMessage("Interoperability", "SYSLIB1096:Convert to 'GeneratedComInterface'", Justification = "This interface is for us to test interop between GeneratedComInterface and ComImport.")]
internal interface IGetAndSetIntComImport
{
int GetInt();

public void SetInt(int x);

public const string _guid = "2c3f9903-b586-46b1-881b-adfce9af47b1";
}

[Fact]
public void CallComImportInterfaceMethodsOnGeneratedComObject()
{
using var _ = RemoteExecutor.Invoke(() =>
{
IGetAndSetInt obj = NewNativeObject();
#pragma warning disable SYSLIB1099 // Casting between a 'ComImport' type and a source-generated COM type is not supported
IGetAndSetIntComImport runtimeObj = (IGetAndSetIntComImport)obj;
#pragma warning restore SYSLIB1099 // Casting between a 'ComImport' type and a source-generated COM type is not supported
obj.SetInt(1234);
Assert.Equal(1234, runtimeObj.GetInt());
runtimeObj.SetInt(4321);
Assert.Equal(4321, obj.GetInt());

}, new RemoteInvokeOptions
{
RuntimeConfigurationOptions =
{
{ "System.Runtime.InteropServices.Marshalling.EnableGeneratedComInterfaceComImportInterop", true }
}
});
}

[Fact]
public void CallComImportInterfaceMethodsOnGeneratedComObject_FeatureFalse_Fails()
{
using var _ = RemoteExecutor.Invoke(() =>
{
IGetAndSetInt obj = NewNativeObject();
#pragma warning disable SYSLIB1099 // Casting between a 'ComImport' type and a source-generated COM type is not supported
Assert.Throws<InvalidCastException>(() => (IGetAndSetIntComImport)obj);
#pragma warning restore SYSLIB1099 // Casting between a 'ComImport' type and a source-generated COM type is not supported
}, new RemoteInvokeOptions
{
RuntimeConfigurationOptions =
{
{ "System.Runtime.InteropServices.Marshalling.EnableGeneratedComInterfaceComImportInterop", false }
}
});
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;

Type comObject = RemoveTypeTrimAnalysis(typeof(ComObject));

// Ensure that the interop details strategy and all of its nested types are fully trimmed away.
if (GetTypeWithoutTrimAnalysis("System.Runtime.InteropServices.Marshalling.ComImportInteropInterfaceDetailsStrategy", comObject.Assembly) != null)
{
return -1;
}

// Ensure that the ComInterop object field is trimmed away as well.
if (comObject.GetFields(BindingFlags.NonPublic | BindingFlags.Instance).Any(f => f.Name == "_runtimeCallableWrapper"))
{
return -2;
}

var comWrappers = new StrategyBasedComWrappers();

var managedObject = new ComClass();
var nativeObject = comWrappers.GetOrCreateComInterfaceForObject(managedObject, CreateComInterfaceFlags.None);
var wrapper = comWrappers.GetOrCreateObjectForComInstance(nativeObject, CreateObjectFlags.None);
Marshal.Release(nativeObject);

// We can't use the wrapper as trimming tests don't reference the source generators from the live ref pack
// due to how we construct it. Tracking this issue as https://github.com/dotnet/runtime/issues/87582.
return 100;

[MethodImpl(MethodImplOptions.NoInlining)]
static Type RemoveTypeTrimAnalysis(Type type) => type;

[MethodImpl(MethodImplOptions.NoInlining)]
static Type GetTypeWithoutTrimAnalysis(string typeName, Assembly assembly)
{
return assembly.GetType(typeName, throwOnError: false);
}

[GeneratedComInterface]
[Guid("ad358058-2b72-4801-8d98-043d44dc42c4")]
partial interface IComInterface
{
int Method();
}

[GeneratedComClass]
partial class ComClass : IComInterface
{
public int Method() => 100;
}
Loading