Skip to content

NativeAOT: Allow Delegate and MulticastDelegate marshalling #63219

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jan 17, 2022
Merged

NativeAOT: Allow Delegate and MulticastDelegate marshalling #63219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
27eca73
NativeAOT: Allow Delegate and MulticastDelegate marshalling
kant2002 Dec 30, 2021
09a661b
Produce warning during analysis
kant2002 Jan 10, 2022
54afafb
Update src/coreclr/tools/aot/ILLink.Shared/SharedStrings.resx
kant2002 Jan 11, 2022
ea43a6b
Use resources for compiler message
kant2002 Jan 11, 2022
ebc5282
Apply PR feedback
kant2002 Jan 12, 2022
76955d3
Add smoke tests for abstract delegates marshalling
kant2002 Jan 12, 2022
86589b4
Apply PR feedback
kant2002 Jan 13, 2022
51d00cb
Remove not used code.
kant2002 Jan 13, 2022
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
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ internal static MarshallerKind GetMarshallerKind(
else
return MarshallerKind.Invalid;
}
else if (type.IsDelegate)
else if (type.IsDelegate || InteropTypes.IsSystemDelegate(context, type) || InteropTypes.IsSystemMulticastDelegate(context, type))
Copy link
Member

Choose a reason for hiding this comment

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

System.Delegate and System.MulticastDelegate marshaling is not AOT friendly. We cannot tell the delegate type to use during AOT compilation for these cases. The marshaling logic is suppressing the AOT warning for the delegates, but this suppression is not correct for the abstract delegate types.

This change also needs a matching change in the marshalling logic to avoid suppressing the AOT compatibility warning for these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Short version: I miss some part of puzzle about who produce AOT warnings. Please advice.

Longer version

I look at places where mentioned MulticastDelegate in src/coreclr/tools/

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Line 2371 in b2dc37b

else if (parameterType.IsDelegate || parameterType.IsWellKnownType(WellKnownType.MulticastDelegate)

and others

Then for IsDelegate in same folder. Since this checks for something derived from MulticastDelegate I clearly do not see

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedInteropStubManager.cs

Line 109 in b2dc37b

if (type.IsDelegate)

and others

DelegateMarshaller relies on PInvokeMarshal which does not annotated for AOT warnings.

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

Lines 2023 to 2027 in 6e286dd

InteropTypes.GetPInvokeMarshal(Context).GetKnownMethod("GetFunctionPointerForDelegate",
#endif
new MethodSignature(MethodSignatureFlags.Static, 0, Context.GetWellKnownType(WellKnownType.IntPtr),
new TypeDesc[] { Context.GetWellKnownType(WellKnownType.MulticastDelegate).BaseType }
))));

PInvokeMarshal class itself appears to not care at all about marshalling. I assume this is low-level logic.

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/PInvokeMarshal.cs

Line 46 in b2dc37b

public static IntPtr GetFunctionPointerForDelegate(Delegate del)

Copy link
Member

Choose a reason for hiding this comment

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

AOT warnings are produced by calling methods annotated with RequiresUnreferencedCode. I think you may want to create a new methods on PInvokeMarshal that are annotated with RequiresUnreferencedCode and just forward to the unannotated methods, and call these methods for abstract delegates.

Copy link
Member

Choose a reason for hiding this comment

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

This will produce a warning, but it won't be a particularly useful warning (it will produce a warning saying that a compiler-generated method is calling a method annotated RequiresUnreferencedCode, which is not particularly actionable).

The proper fix would be to add handling of this somewhere around here (and a new warning code):

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 2178 to 2203 in d0631be

if (calledMethod.IsPInvoke)
{
// Is the PInvoke dangerous?
ParameterMetadata[] paramMetadata = calledMethod.GetParameterMetadata();
ParameterMetadata returnParamMetadata = Array.Find(paramMetadata, m => m.Index == 0);
bool comDangerousMethod = IsComInterop(returnParamMetadata.MarshalAsDescriptor, calledMethod.Signature.ReturnType);
for (int paramIndex = 0; paramIndex < calledMethod.Signature.Length; paramIndex++)
{
MarshalAsDescriptor marshalAsDescriptor = null;
for (int metadataIndex = 0; metadataIndex < paramMetadata.Length; metadataIndex++)
{
if (paramMetadata[metadataIndex].Index == paramIndex + 1)
marshalAsDescriptor = paramMetadata[metadataIndex].MarshalAsDescriptor;
}
comDangerousMethod |= IsComInterop(marshalAsDescriptor, calledMethod.Signature[paramIndex]);
}
if (comDangerousMethod)
{
reflectionContext.AnalyzingPattern();
reflectionContext.RecordUnrecognizedPattern(2050, $"P/invoke method '{calledMethod.GetDisplayName()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.");
}
}

There's a pull request in flight that moves the warning codes to someplace else, so I would wait for that (#63230) to land first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume that plan was to revert last commit which generate annotated code and made changes in analysis. Force push changes.

{
if (type.HasInstantiation)
{
Expand Down
10 changes: 10 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Interop/InteropTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@ public static bool IsSystemDecimal(TypeSystemContext context, TypeDesc type)
return IsCoreNamedType(context, type, "System", "Decimal");
}

public static bool IsSystemDelegate(TypeSystemContext context, TypeDesc type)
{
return IsCoreNamedType(context, type, "System", "Delegate");
}

public static bool IsSystemMulticastDelegate(TypeSystemContext context, TypeDesc type)
{
return IsCoreNamedType(context, type, "System", "MulticastDelegate");
}

public static bool IsSystemGuid(TypeSystemContext context, TypeDesc type)
{
return IsCoreNamedType(context, type, "System", "Guid");
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2199,7 +2199,9 @@ public override bool HandleCall(MethodIL callingMethodBody, MethodDesc calledMet
if (comDangerousMethod)
{
reflectionContext.AnalyzingPattern();
reflectionContext.RecordUnrecognizedPattern(2050, $"P/invoke method '{calledMethod.GetDisplayName()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.");
reflectionContext.RecordUnrecognizedPattern(
(int)DiagnosticId.CorrectnessOfCOMCannotBeGuaranteed,
new DiagnosticString(DiagnosticId.CorrectnessOfCOMCannotBeGuaranteed).GetMessage(DiagnosticUtilities.GetMethodSignatureDisplayName(calledMethod)));
}
}

Expand Down
27 changes: 22 additions & 5 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedInteropStubManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
using Internal.TypeSystem;
using Internal.TypeSystem.Interop;

using ILCompiler.Dataflow;
Copy link
Member

Choose a reason for hiding this comment

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

I think the new usings are not needed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both usings are needed for DiagnosticUtilities and DiagnosticId.

using ILCompiler.DependencyAnalysis;
using ILLink.Shared;

using Debug = System.Diagnostics.Debug;
using DependencyList = ILCompiler.DependencyAnalysisFramework.DependencyNodeCore<ILCompiler.DependencyAnalysis.NodeFactory>.DependencyList;
Expand All @@ -18,9 +20,12 @@ namespace ILCompiler
/// </summary>
public class UsageBasedInteropStubManager : CompilerGeneratedInteropStubManager
{
public UsageBasedInteropStubManager(InteropStateManager interopStateManager, PInvokeILEmitterConfiguration pInvokeILEmitterConfiguration)
private Logger _logger;

public UsageBasedInteropStubManager(InteropStateManager interopStateManager, PInvokeILEmitterConfiguration pInvokeILEmitterConfiguration, Logger logger)
: base(interopStateManager, pInvokeILEmitterConfiguration)
{
_logger = logger;
}

public override void AddDependeciesDueToPInvoke(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
Expand All @@ -30,11 +35,11 @@ public override void AddDependeciesDueToPInvoke(ref DependencyList dependencies,
dependencies = dependencies ?? new DependencyList();

MethodSignature methodSig = method.Signature;
AddParameterMarshallingDependencies(ref dependencies, factory, methodSig.ReturnType);
AddParameterMarshallingDependencies(ref dependencies, factory, method, methodSig.ReturnType);

for (int i = 0; i < methodSig.Length; i++)
{
AddParameterMarshallingDependencies(ref dependencies, factory, methodSig[i]);
AddParameterMarshallingDependencies(ref dependencies, factory, method, methodSig[i]);
}
}

Expand All @@ -45,13 +50,25 @@ public override void AddDependeciesDueToPInvoke(ref DependencyList dependencies,
}
}

private static void AddParameterMarshallingDependencies(ref DependencyList dependencies, NodeFactory factory, TypeDesc type)
private void AddParameterMarshallingDependencies(ref DependencyList dependencies, NodeFactory factory, MethodDesc method, TypeDesc type)
{
if (type.IsDelegate)
{
dependencies.Add(factory.DelegateMarshallingData((DefType)type), "Delegate marshaling");
}

TypeSystemContext context = type.Context;
if ((type.IsWellKnownType(WellKnownType.MulticastDelegate)
|| type == context.GetWellKnownType(WellKnownType.MulticastDelegate).BaseType))
{
var message = new DiagnosticString(DiagnosticId.CorrectnessOfAbstractDelegatesCannotBeGuaranteed).GetMessage(DiagnosticUtilities.GetMethodSignatureDisplayName(method));
_logger.LogWarning(
message,
(int)DiagnosticId.CorrectnessOfAbstractDelegatesCannotBeGuaranteed,
method,
MessageSubCategory.AotAnalysis);
}

// struct may contain delegate fields, hence we need to add dependencies for it
if (type.IsByRef)
type = ((ParameterizedType)type).ParameterType;
Expand All @@ -63,7 +80,7 @@ private static void AddParameterMarshallingDependencies(ref DependencyList depen
if (field.IsStatic)
continue;

AddParameterMarshallingDependencies(ref dependencies, factory, field.FieldType);
AddParameterMarshallingDependencies(ref dependencies, factory, method, field.FieldType);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ static string ILLinkify(string rootedAssembly)
_trimmedAssemblies);

InteropStateManager interopStateManager = new InteropStateManager(typeSystemContext.GeneratedAssembly);
InteropStubManager interopStubManager = new UsageBasedInteropStubManager(interopStateManager, pinvokePolicy);
InteropStubManager interopStubManager = new UsageBasedInteropStubManager(interopStateManager, pinvokePolicy, logger);

// Unless explicitly opted in at the command line, we enable scanner for retail builds by default.
// We also don't do this for multifile because scanner doesn't simulate inlining (this would be
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/tools/aot/ILLink.Shared/DiagnosticId.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ public enum DiagnosticId

// Dynamic code diagnostic ids.
RequiresDynamicCode = 3050,
RequiresDynamicCodeAttributeMismatch = 3051
RequiresDynamicCodeAttributeMismatch = 3051,
// TODO: these are all unique to NativeAOT - mono/linker repo is not aware these error codes usage.
// IL3052 - COM
// IL3053 - AOT analysis warnings
// IL3054 - Generic cycle
CorrectnessOfAbstractDelegatesCannotBeGuaranteed = 3055,
}

public static class DiagnosticIdExtensions
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/tools/aot/ILLink.Shared/SharedStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,7 @@
<data name="CorrectnessOfCOMCannotBeGuaranteedMessage" xml:space="preserve">
<value>P/invoke method '{0}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.</value>
</data>
<data name="CorrectnessOfAbstractDelegatesCannotBeGuaranteedMessage" xml:space="preserve">
<value>P/invoke method '{0}' declares a parameter with an abstract delegate. Correctness of interop for abstract delegates cannot be guaranteed after native compilation: the marshalling code for the delegate might not be available. Use a non-abstract delegate type or ensure any delegate instance passed as parameter is marked with `UnmanagedFunctionPointerAttribute`.</value>
</data>
</root>
32 changes: 32 additions & 0 deletions src/tests/nativeaot/SmokeTests/PInvoke/PInvoke.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,28 @@ private static extern bool VerifySizeParamIndex(
[DllImport("PInvokeNative", CallingConvention = CallingConvention.StdCall)]
static extern bool ReversePInvoke_String(Delegate_String del);

[DllImport("PInvokeNative", EntryPoint="ReversePInvoke_String", CallingConvention = CallingConvention.StdCall)]
static extern bool ReversePInvoke_String_Delegate(Delegate del);

[DllImport("PInvokeNative", EntryPoint="ReversePInvoke_String", CallingConvention = CallingConvention.StdCall)]
static extern bool ReversePInvoke_String_MulticastDelegate(MulticastDelegate del);

struct FieldDelegate
{
public Delegate d;
}

struct FieldMulticastDelegate
{
public MulticastDelegate d;
}

[DllImport("PInvokeNative", EntryPoint="ReversePInvoke_DelegateField", CallingConvention = CallingConvention.StdCall)]
static extern bool ReversePInvoke_Field_Delegate(FieldDelegate del);

[DllImport("PInvokeNative", EntryPoint="ReversePInvoke_DelegateField", CallingConvention = CallingConvention.StdCall)]
static extern bool ReversePInvoke_Field_MulticastDelegate(FieldMulticastDelegate del);

[UnmanagedFunctionPointer(CallingConvention.StdCall, CharSet = CharSet.Ansi)]
delegate bool Delegate_OutString([MarshalAs(0x30)] out string s);
[DllImport("PInvokeNative", CallingConvention = CallingConvention.StdCall)]
Expand Down Expand Up @@ -616,6 +638,16 @@ private static void TestDelegate()
Delegate_String ds = new Delegate_String((new ClosedDelegateCLass()).GetString);
ThrowIfNotEquals(true, ReversePInvoke_String(ds), "Delegate marshalling failed.");

ThrowIfNotEquals(true, ReversePInvoke_String_Delegate(ds), "Delegate marshalling failed.");
ThrowIfNotEquals(true, ReversePInvoke_String_MulticastDelegate(ds), "Delegate marshalling failed.");

FieldDelegate fd;
fd.d = ds;
ThrowIfNotEquals(true, ReversePInvoke_Field_Delegate(fd), "Delegate marshalling failed.");
FieldMulticastDelegate fmd;
fmd.d = ds;
ThrowIfNotEquals(true, ReversePInvoke_Field_MulticastDelegate(fmd), "Delegate marshalling failed.");

Delegate_OutString dos = new Delegate_OutString((out string x) =>
{
x = "Hello there!";
Expand Down
11 changes: 11 additions & 0 deletions src/tests/nativeaot/SmokeTests/PInvoke/PInvokeNative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,17 @@ DLL_EXPORT bool __stdcall ReversePInvoke_String(StringFuncPtr fnPtr)
return fnPtr(str);
}

struct DelegateFieldStruct
{
StringFuncPtr fnPtr;
};

DLL_EXPORT bool __stdcall ReversePInvoke_DelegateField(DelegateFieldStruct p)
{
char str[] = "Hello World";
return p.fnPtr(str);
}

typedef bool(__stdcall *OutStringFuncPtr) (char **);
DLL_EXPORT bool __stdcall ReversePInvoke_OutString(OutStringFuncPtr fnPtr)
{
Expand Down