Skip to content

Commit

Permalink
Don't crash on recursive native layout (dotnet#74630)
Browse files Browse the repository at this point in the history 
Fixes dotnet/runtimelab#163.

* We don't have a nice way to pass context around when computing the native type. Use a threadstatic. This is pretty rare.
* We had one more spot where we ran out of stack when we were trying to look for delegates. Remove the need for that code by using the generic `Marshal` API (that we recognize elsewhere). Move the code responsible for warnings to dataflow analysis.
  • Loading branch information
@MichalStrehovsky
MichalStrehovsky authored Aug 27, 2022
1 parent 26b90d3 commit e98fc2a
Show file tree
Hide file tree
Showing 8 changed files with 45 additions and 70 deletions.
17 changes: 8 additions & 9 deletions src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1917,10 +1917,10 @@ protected override void AllocAndTransformManagedToNative(ILCodeStream codeStream
codeStream.Emit(ILOpcode.brfalse, lNullPointer);

codeStream.Emit(ILOpcode.call, _ilCodeStreams.Emitter.NewToken(
InteropTypes.GetPInvokeMarshal(Context).GetKnownMethod("GetFunctionPointerForDelegate",
new MethodSignature(MethodSignatureFlags.Static, 0, Context.GetWellKnownType(WellKnownType.IntPtr),
new TypeDesc[] { Context.GetWellKnownType(WellKnownType.MulticastDelegate).BaseType }
))));
InteropTypes.GetMarshal(Context).GetKnownMethod("GetFunctionPointerForDelegate",
new MethodSignature(MethodSignatureFlags.Static, 1, Context.GetWellKnownType(WellKnownType.IntPtr),
new TypeDesc[] { Context.GetSignatureVariable(0, method: true) }
)).MakeInstantiatedMethod(ManagedType)));

codeStream.Emit(ILOpcode.br, lDone);

Expand All @@ -1941,13 +1941,12 @@ protected override void TransformNativeToManaged(ILCodeStream codeStream)
LoadNativeValue(codeStream);
codeStream.Emit(ILOpcode.dup);
codeStream.Emit(ILOpcode.brfalse, lNullPointer);
codeStream.Emit(ILOpcode.ldtoken, _ilCodeStreams.Emitter.NewToken(ManagedType));

codeStream.Emit(ILOpcode.call, _ilCodeStreams.Emitter.NewToken(
InteropTypes.GetPInvokeMarshal(Context).GetKnownMethod("GetDelegateForFunctionPointer",
new MethodSignature(MethodSignatureFlags.Static, 0, Context.GetWellKnownType(WellKnownType.MulticastDelegate).BaseType,
new TypeDesc[] { Context.GetWellKnownType(WellKnownType.IntPtr), Context.GetWellKnownType(WellKnownType.RuntimeTypeHandle) }
))));
InteropTypes.GetMarshal(Context).GetKnownMethod("GetDelegateForFunctionPointer",
new MethodSignature(MethodSignatureFlags.Static, 1, Context.GetSignatureVariable(0, method: true),
new TypeDesc[] { Context.GetWellKnownType(WellKnownType.IntPtr) }
)).MakeInstantiatedMethod(ManagedType)));

codeStream.Emit(ILOpcode.br, lDone);

Expand Down
19 changes: 18 additions & 1 deletion src/coreclr/tools/Common/TypeSystem/Interop/IL/NativeStructType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ public FieldDesc[] Fields
}
}

[ThreadStatic]
static Stack<MetadataType> s_typesBeingLookedAt;

public NativeStructType(ModuleDesc owningModule, MetadataType managedStructType, InteropStateManager interopStateManager)
{
Debug.Assert(!managedStructType.IsGenericDefinition);
Expand All @@ -168,7 +171,21 @@ public NativeStructType(ModuleDesc owningModule, MetadataType managedStructType,
ManagedStructType = managedStructType;
_interopStateManager = interopStateManager;
_hasInvalidLayout = false;
CalculateFields();

Stack<MetadataType> typesBeingLookedAt = (s_typesBeingLookedAt ??= new Stack<MetadataType>());
if (typesBeingLookedAt.Contains(managedStructType))
ThrowHelper.ThrowTypeLoadException(managedStructType);

typesBeingLookedAt.Push(managedStructType);
try
{
CalculateFields();
}
finally
{
MetadataType popped = typesBeingLookedAt.Pop();
Debug.Assert(popped == managedStructType);
}
}

private void CalculateFields()
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/AnalysisBasedInteropStubManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public override void AddCompilationRoots(IRootingServiceProvider rootProvider)
}
}

public override void AddDependenciesDueToPInvoke(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
public override void AddDependenciesDueToMethodCodePresence(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
{
}

Expand Down
14 changes: 14 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ public static bool HandleCall(

ParameterMetadata returnParamMetadata = Array.Find(paramMetadata, m => m.Index == 0);

bool aotUnsafeDelegate = IsAotUnsafeDelegate(calledMethod.Signature.ReturnType);
bool comDangerousMethod = IsComInterop(returnParamMetadata.MarshalAsDescriptor, calledMethod.Signature.ReturnType);
for (int paramIndex = 0; paramIndex < calledMethod.Signature.Length; paramIndex++)
{
Expand All @@ -327,9 +328,15 @@ public static bool HandleCall(
marshalAsDescriptor = paramMetadata[metadataIndex].MarshalAsDescriptor;
}

aotUnsafeDelegate |= IsAotUnsafeDelegate(calledMethod.Signature[paramIndex]);
comDangerousMethod |= IsComInterop(marshalAsDescriptor, calledMethod.Signature[paramIndex]);
}

if (aotUnsafeDelegate)
{
diagnosticContext.AddDiagnostic(DiagnosticId.CorrectnessOfAbstractDelegatesCannotBeGuaranteed, calledMethod.GetDisplayName());
}

if (comDangerousMethod)
{
diagnosticContext.AddDiagnostic(DiagnosticId.CorrectnessOfCOMCannotBeGuaranteed, calledMethod.GetDisplayName());
Expand Down Expand Up @@ -563,6 +570,13 @@ void AddReturnValue(MultiValue value)
}
}

static bool IsAotUnsafeDelegate(TypeDesc parameterType)
{
TypeSystemContext context = parameterType.Context;
return parameterType.IsWellKnownType(Internal.TypeSystem.WellKnownType.MulticastDelegate)
|| parameterType == context.GetWellKnownType(Internal.TypeSystem.WellKnownType.MulticastDelegate).BaseType;
}

static bool IsComInterop(MarshalAsDescriptor? marshalInfoProvider, TypeDesc parameterType)
{
// This is best effort. One can likely find ways how to get COM without triggering these alarms.
Expand Down
2 changes: 1 addition & 1 deletion ...tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CodeBasedDependencyAlgorithm.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public static void AddDependenciesDueToMethodCodePresence(ref DependencyList dep
{
factory.MetadataManager.GetDependenciesDueToMethodCodePresence(ref dependencies, factory, method, methodIL);

factory.InteropStubManager.AddDependenciesDueToPInvoke(ref dependencies, factory, method);
factory.InteropStubManager.AddDependenciesDueToMethodCodePresence(ref dependencies, factory, method);

if (method.OwningType is MetadataType mdType)
ModuleUseBasedDependencyAlgorithm.AddDependenciesDueToModuleUse(ref dependencies, factory, mdType.Module);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/EmptyInteropStubManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public override PInvokeILProvider CreatePInvokeILProvider()
return null;
}

public override void AddDependenciesDueToPInvoke(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
public override void AddDependenciesDueToMethodCodePresence(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
{
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/InteropStubManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace ILCompiler
/// </summary>
public abstract class InteropStubManager : ICompilationRootProvider
{
public abstract void AddDependenciesDueToPInvoke(ref DependencyList dependencies, NodeFactory factory, MethodDesc method);
public abstract void AddDependenciesDueToMethodCodePresence(ref DependencyList dependencies, NodeFactory factory, MethodDesc method);

public abstract void AddInterestingInteropConstructedTypeDependencies(ref DependencyList dependencies, NodeFactory factory, TypeDesc type);

Expand Down
57 changes: 1 addition & 56 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedInteropStubManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,70 +28,15 @@ public UsageBasedInteropStubManager(InteropStateManager interopStateManager, PIn
_logger = logger;
}

public override void AddDependenciesDueToPInvoke(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
public override void AddDependenciesDueToMethodCodePresence(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
{
if (method.IsPInvoke && method.OwningType is MetadataType type && MarshalHelpers.IsRuntimeMarshallingEnabled(type.Module))
{
dependencies = dependencies ?? new DependencyList();

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

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

if (method.HasInstantiation)
{
dependencies = dependencies ?? new DependencyList();
AddMarshalAPIsGenericDependencies(ref dependencies, factory, method);
}
}

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))
{
// If we hit this p/invoke as part of delegate marshalling (i.e. this is a delegate
// that has another delegate in the signature), blame the delegate type, not the marshalling thunk.
// This should ideally warn from the use site (e.g. where GetDelegateForFunctionPointer
// is called) but it's currently hard to get a warning from those spots and this guarantees
// we won't miss a spot (e.g. a p/invoke that has a delegate and that delegate contains
// a System.Delegate parameter).
MethodDesc reportedMethod = method;
if (reportedMethod is Internal.IL.Stubs.DelegateMarshallingMethodThunk delegateThunkMethod)
{
reportedMethod = delegateThunkMethod.InvokeMethod;
}

_logger.LogWarning(reportedMethod, DiagnosticId.CorrectnessOfAbstractDelegatesCannotBeGuaranteed, DiagnosticUtilities.GetMethodSignatureDisplayName(method));
}

// struct may contain delegate fields, hence we need to add dependencies for it
if (type.IsByRef)
type = ((ParameterizedType)type).ParameterType;

if (MarshalHelpers.IsStructMarshallingRequired(type))
{
foreach (FieldDesc field in type.GetFields())
{
if (field.IsStatic)
continue;

AddParameterMarshallingDependencies(ref dependencies, factory, method, field.FieldType);
}
}
}

public override void AddInterestingInteropConstructedTypeDependencies(ref DependencyList dependencies, NodeFactory factory, TypeDesc type)
{
if (type.IsDelegate)
Expand Down

0 comments on commit e98fc2a

Please sign in to comment.