Skip to content

Commit

Permalink
[UpdateEngine] AssemblyProcessor was generating invalid ldftn for vir…
Browse files Browse the repository at this point in the history 
…tual/interface calls (causing crashes with .NET Core 3). Switch to ldvirtftn for those instead.

Now, we use the following approach:
- For normal calls, we use ldftn and an instance calls
- For virtual and interface calls, we generate a dispatch function that calls ldvirtftn on the actual object, then call the method on the object.
Note: this dispatcher method is static, so the calli has a different signature (currently a if/else, but could later be done by separate class implementations).
  • Loading branch information
@xen2
xen2 committed Oct 17, 2019
1 parent d1f1883 commit e42121a
Show file tree
Hide file tree
Showing 10 changed files with 213 additions and 47 deletions.
4 changes: 2 additions & 2 deletions deps/AssemblyProcessor/Xenko.Core.AssemblyProcessor.Packed.exe
View file
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/AssemblyProcessor/Xenko.Core.AssemblyProcessor.Packed.pdb
View file
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/AssemblyProcessor/Xenko.Core.AssemblyProcessor.exe
View file
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/AssemblyProcessor/Xenko.Core.AssemblyProcessor.pdb
View file
Git LFS file not shown
34 changes: 32 additions & 2 deletions sources/core/Xenko.Core.AssemblyProcessor/UpdateEngineProcessor.IL.cs
View file
Expand Up @@ -153,6 +153,8 @@ class UpdatablePropertyCodeGenerator : UpdatablePropertyBaseCodeGenerator
{
private readonly FieldDefinition updatablePropertyGetter;
private readonly FieldDefinition updatablePropertySetter;
private readonly FieldDefinition updatablePropertyVirtualDispatchGetter;
private readonly FieldDefinition updatablePropertyVirtualDispatchSetter;
protected TypeDefinition declaringTypeForObjectMethods;

public UpdatablePropertyCodeGenerator(AssemblyDefinition assembly) : base(assembly)
Expand All @@ -163,6 +165,8 @@ public UpdatablePropertyCodeGenerator(AssemblyDefinition assembly) : base(assemb

updatablePropertyGetter = declaringTypeForObjectMethods.Fields.First(x => x.Name == "Getter");
updatablePropertySetter = declaringTypeForObjectMethods.Fields.First(x => x.Name == "Setter");
updatablePropertyVirtualDispatchGetter = declaringTypeForObjectMethods.Fields.First(x => x.Name == "VirtualDispatchGetter");
updatablePropertyVirtualDispatchSetter = declaringTypeForObjectMethods.Fields.First(x => x.Name == "VirtualDispatchSetter");
}

public override void GenerateUpdatablePropertyCode()
Expand All @@ -188,16 +192,42 @@ public override void GenerateUpdatablePropertyCode()

public override void EmitGetCode(ILProcessor il, TypeReference type)
{
var calliInstance = Instruction.Create(OpCodes.Calli, new CallSite(type) { HasThis = true });
var calliVirtualDispatch = Instruction.Create(OpCodes.Calli, new CallSite(type) { HasThis = false, Parameters = { new ParameterDefinition(assembly.MainModule.TypeSystem.Object) } });
var postCalli = Instruction.Create(OpCodes.Nop);

il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldfld, updatablePropertyGetter);
il.Emit(OpCodes.Calli, new CallSite(type) { HasThis = true });
il.Emit(OpCodes.Ldarg_0);
// For normal calls, we use ldftn and an instance calls
// For virtual and interface calls, we generate a dispatch function that calls ldvirtftn on the actual object, then call the method on the object
// this dispatcher method is static, so the calli has a different signature
// Note: we could later optimize the bool check by having two variant of Get/SetObject
// and two different implementations of both UpdatableProperty<T> and UpdatablePropertyObject<T>
// (not sure if worth it)
il.Emit(OpCodes.Ldfld, updatablePropertyVirtualDispatchGetter);
il.Emit(OpCodes.Brfalse, calliInstance);
il.Append(calliVirtualDispatch);
il.Emit(OpCodes.Br, postCalli);
il.Append(calliInstance);
il.Append(postCalli);
}

public override void EmitSetCodeAfterValue(ILProcessor il, TypeReference type)
{
var calliInstance = Instruction.Create(OpCodes.Calli, new CallSite(assembly.MainModule.TypeSystem.Void) { HasThis = true, Parameters = { new ParameterDefinition(type) } });
var calliVirtualDispatch = Instruction.Create(OpCodes.Calli, new CallSite(assembly.MainModule.TypeSystem.Void) { HasThis = false, Parameters = { new ParameterDefinition(assembly.MainModule.TypeSystem.Object), new ParameterDefinition(type) } });
var postCalli = Instruction.Create(OpCodes.Nop);

il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldfld, updatablePropertySetter);
il.Emit(OpCodes.Calli, new CallSite(assembly.MainModule.TypeSystem.Void) { HasThis = true, Parameters = { new ParameterDefinition(type) } });
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldfld, updatablePropertyVirtualDispatchSetter);
il.Emit(OpCodes.Brfalse, calliInstance);
il.Append(calliVirtualDispatch);
il.Emit(OpCodes.Br, postCalli);
il.Append(calliInstance);
il.Append(postCalli);
}
}

Expand Down
130 changes: 100 additions & 30 deletions sources/core/Xenko.Core.AssemblyProcessor/UpdateEngineProcessor.cs
View file
Expand Up @@ -34,6 +34,54 @@ internal partial class UpdateEngineProcessor : ICecilSerializerProcessor

private MethodReference getTypeFromHandleMethod;

int prepareMethodCount = 0;

private MethodDefinition CreateUpdateMethod(AssemblyDefinition assembly)
{
// Get or create method
var updateEngineType = GetOrCreateUpdateType(assembly, true);
var mainPrepareMethod = new MethodDefinition($"UpdateMain{prepareMethodCount++}", MethodAttributes.HideBySig | MethodAttributes.Assembly | MethodAttributes.Static, assembly.MainModule.TypeSystem.Void);
updateEngineType.Methods.Add(mainPrepareMethod);

// Make sure it is called at module startup
var xenkoCoreModule = assembly.GetXenkoCoreModule();
var moduleInitializerAttribute = xenkoCoreModule.GetType("Xenko.Core.ModuleInitializerAttribute");
var ctorMethod = moduleInitializerAttribute.GetConstructors().Single(x => !x.IsStatic && !x.HasParameters);
mainPrepareMethod.CustomAttributes.Add(new CustomAttribute(assembly.MainModule.ImportReference(ctorMethod)));

return mainPrepareMethod;
}

private MethodReference CreateDispatcher(AssemblyDefinition assembly, MethodReference method)
{
var updateEngineType = GetOrCreateUpdateType(assembly, true);

var dispatcherMethod = new MethodDefinition($"Dispatcher_{method.Name}", MethodAttributes.HideBySig | MethodAttributes.Assembly | MethodAttributes.Static, assembly.MainModule.ImportReference(method.ReturnType));
updateEngineType.Methods.Add(dispatcherMethod);

dispatcherMethod.Parameters.Add(new ParameterDefinition("this", Mono.Cecil.ParameterAttributes.None, method.DeclaringType));
foreach (var param in method.Parameters)
{
dispatcherMethod.Parameters.Add(new ParameterDefinition(param.Name, param.Attributes, param.ParameterType));
}

var il = dispatcherMethod.Body.GetILProcessor();
il.Emit(OpCodes.Ldarg_0);
foreach (var param in dispatcherMethod.Parameters.Skip(1)) // first parameter is "this"
{
il.Emit(OpCodes.Ldarg, param);
}
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldvirtftn, method);
var callsite = new Mono.Cecil.CallSite(method.ReturnType) { HasThis = true };
foreach (var param in method.Parameters)
callsite.Parameters.Add(param);
il.Emit(OpCodes.Calli, callsite);
il.Emit(OpCodes.Ret);

return dispatcherMethod;
}

public void ProcessSerializers(CecilSerializerContext context)
{
var references = new HashSet<AssemblyDefinition>();
Expand Down Expand Up @@ -62,13 +110,7 @@ public void ProcessSerializers(CecilSerializerContext context)
return;
}

// Get or create method
var updateEngineType = GetOrCreateUpdateType(context.Assembly, true);
var mainPrepareMethod = new MethodDefinition("UpdateMain", MethodAttributes.HideBySig | MethodAttributes.Assembly | MethodAttributes.Static, context.Assembly.MainModule.TypeSystem.Void);
updateEngineType.Methods.Add(mainPrepareMethod);

// Get some useful Cecil objects from Xenko.Core
var xenkoCoreModule = context.Assembly.GetXenkoCoreModule();

var xenkoEngineAssembly = context.Assembly.Name.Name == "Xenko.Engine"
? context.Assembly
Expand Down Expand Up @@ -109,10 +151,7 @@ public void ProcessSerializers(CecilSerializerContext context)
var typeType = coreAssembly.MainModule.GetTypeResolved(typeof(Type).FullName);
getTypeFromHandleMethod = context.Assembly.MainModule.ImportReference(typeType.Methods.First(x => x.Name == "GetTypeFromHandle"));

// Make sure it is called at module startup
var moduleInitializerAttribute = xenkoCoreModule.GetType("Xenko.Core.ModuleInitializerAttribute");
var ctorMethod = moduleInitializerAttribute.GetConstructors().Single(x => !x.IsStatic && !x.HasParameters);
mainPrepareMethod.CustomAttributes.Add(new CustomAttribute(context.Assembly.MainModule.ImportReference(ctorMethod)));
var mainPrepareMethod = CreateUpdateMethod(context.Assembly);

// Emit serialization code for all the types we care about
var processedTypes = new HashSet<TypeDefinition>(TypeReferenceEqualityComparer.Default);
Expand Down Expand Up @@ -245,7 +284,9 @@ public void ProcessType(CecilSerializerContext context, TypeReference type, Meth
}

var il = updateCurrentMethod.Body.GetILProcessor();
var emptyObjectField = updateMainMethod.DeclaringType.Fields.FirstOrDefault(x => x.Name == "emptyObject");
var typeIsValueType = type.IsResolvedValueType();
var emptyObjectField = typeIsValueType ? null : updateMainMethod.DeclaringType.Fields.FirstOrDefault(x => x.Name == "emptyObject");
VariableDefinition emptyStruct = null;

// Note: forcing fields and properties to be processed in all cases
foreach (var serializableItem in ComplexSerializerRegistry.GetSerializableItems(type, true, ComplexTypeSerializerFlags.SerializePublicFields | ComplexTypeSerializerFlags.SerializePublicProperties | ComplexTypeSerializerFlags.Updatable))
Expand All @@ -254,32 +295,50 @@ public void ProcessType(CecilSerializerContext context, TypeReference type, Meth
{
var field = fieldReference.Resolve();

// First time it is needed, let's create empty object: var emptyObject = new object();
if (emptyObjectField == null)
// First time it is needed, let's create empty object in the class (var emptyObject = new object()) or empty local struct in the method
if (typeIsValueType)
{
emptyObjectField = new FieldDefinition("emptyObject", FieldAttributes.Static | FieldAttributes.Private, context.Assembly.MainModule.TypeSystem.Object);

// Create static ctor that will initialize this object
var staticConstructor = new MethodDefinition(".cctor",
MethodAttributes.Private | MethodAttributes.HideBySig | MethodAttributes.Static | MethodAttributes.SpecialName | MethodAttributes.RTSpecialName,
context.Assembly.MainModule.TypeSystem.Void);
var staticConstructorIL = staticConstructor.Body.GetILProcessor();
staticConstructorIL.Emit(OpCodes.Newobj, context.Assembly.MainModule.ImportReference(emptyObjectField.FieldType.Resolve().GetConstructors().Single(x => !x.IsStatic && !x.HasParameters)));
staticConstructorIL.Emit(OpCodes.Stsfld, emptyObjectField);
staticConstructorIL.Emit(OpCodes.Ret);

updateMainMethod.DeclaringType.Fields.Add(emptyObjectField);
updateMainMethod.DeclaringType.Methods.Add(staticConstructor);
if (emptyStruct == null)
{
emptyStruct = new VariableDefinition(type);
updateMainMethod.Body.Variables.Add(emptyStruct);
}
}
else
{
if (emptyObjectField == null)
{
emptyObjectField = new FieldDefinition("emptyObject", FieldAttributes.Static | FieldAttributes.Private, context.Assembly.MainModule.TypeSystem.Object);

// Create static ctor that will initialize this object
var staticConstructor = new MethodDefinition(".cctor",
MethodAttributes.Private | MethodAttributes.HideBySig | MethodAttributes.Static | MethodAttributes.SpecialName | MethodAttributes.RTSpecialName,
context.Assembly.MainModule.TypeSystem.Void);
var staticConstructorIL = staticConstructor.Body.GetILProcessor();
staticConstructorIL.Emit(OpCodes.Newobj, context.Assembly.MainModule.ImportReference(emptyObjectField.FieldType.Resolve().GetConstructors().Single(x => !x.IsStatic && !x.HasParameters)));
staticConstructorIL.Emit(OpCodes.Stsfld, emptyObjectField);
staticConstructorIL.Emit(OpCodes.Ret);

updateMainMethod.DeclaringType.Fields.Add(emptyObjectField);
updateMainMethod.DeclaringType.Methods.Add(staticConstructor);
}
}


il.Emit(OpCodes.Ldtoken, type);
il.Emit(OpCodes.Call, getTypeFromHandleMethod);
il.Emit(OpCodes.Ldstr, field.Name);

il.Emit(OpCodes.Ldsfld, emptyObjectField);
if (typeIsValueType)
il.Emit(OpCodes.Ldloca, emptyStruct);
else
il.Emit(OpCodes.Ldsfld, emptyObjectField);
il.Emit(OpCodes.Ldflda, context.Assembly.MainModule.ImportReference(fieldReference));
il.Emit(OpCodes.Conv_I);
il.Emit(OpCodes.Ldsfld, emptyObjectField);
if (typeIsValueType)
il.Emit(OpCodes.Ldloca, emptyStruct);
else
il.Emit(OpCodes.Ldsfld, emptyObjectField);
il.Emit(OpCodes.Conv_I);
il.Emit(OpCodes.Sub);
il.Emit(OpCodes.Conv_I4);
Expand All @@ -300,12 +359,21 @@ public void ProcessType(CecilSerializerContext context, TypeReference type, Meth
il.Emit(OpCodes.Call, getTypeFromHandleMethod);
il.Emit(OpCodes.Ldstr, property.Name);

// If it's a virtual or interface call, we need to create a dispatcher using ldvirtftn
if (property.GetMethod.IsVirtual)
propertyGetMethod = CreateDispatcher(context.Assembly, propertyGetMethod);

il.Emit(OpCodes.Ldftn, propertyGetMethod);

// Only get setter if it exists and it's public
// Set whether getter method uses a VirtualDispatch (static call) or instance call
il.Emit(property.GetMethod.IsVirtual ? OpCodes.Ldc_I4_1 : OpCodes.Ldc_I4_0);

// Only uses setter if it exists and it's public
if (property.SetMethod != null && property.SetMethod.IsPublic)
{
var propertySetMethod = context.Assembly.MainModule.ImportReference(property.SetMethod).MakeGeneric(updateCurrentMethod.GenericParameters.ToArray());
if (property.SetMethod.IsVirtual)
propertySetMethod = CreateDispatcher(context.Assembly, propertySetMethod);
il.Emit(OpCodes.Ldftn, propertySetMethod);
}
else
Expand All @@ -315,6 +383,9 @@ public void ProcessType(CecilSerializerContext context, TypeReference type, Meth
il.Emit(OpCodes.Conv_I);
}

// Set whether setter method uses a VirtualDispatch (static call) or instance call
il.Emit((property.SetMethod?.IsVirtual ?? false) ? OpCodes.Ldc_I4_1 : OpCodes.Ldc_I4_0);

var propertyType = context.Assembly.MainModule.ImportReference(replaceGenericsVisitor != null ? replaceGenericsVisitor.VisitDynamic(property.PropertyType) : property.PropertyType);

var updatablePropertyInflatedCtor = GetOrCreateUpdatablePropertyCtor(context.Assembly, propertyType);
Expand All @@ -338,7 +409,6 @@ public void ProcessType(CecilSerializerContext context, TypeReference type, Meth
}
}


private static string ComputeUpdateMethodName(TypeDefinition typeDefinition)
{
var typeName = ComputeTypeName(typeDefinition);
Expand Down

0 comments on commit e42121a

Please sign in to comment.