Skip to content

Commit

Permalink
Merge pull request #21269 from VSadov/peverify
Browse files Browse the repository at this point in the history 
Introduces a peverify-compat feature flag
  • Loading branch information
@VSadov
VSadov authored Aug 8, 2017
2 parents 596eb2c + 243afaf commit d3ea5d2
Show file tree
Hide file tree
Showing 11 changed files with 547 additions and 40 deletions.
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ private bool IsDebugPlus()
return _module.Compilation.Options.DebugPlusMode;
}

private bool EnablePEVerifyCompat()
{
return _module.Compilation.FeaturePEVerifyCompatEnabled;
}

private LocalDefinition LazyReturnTemp
{
get
Expand Down
26 changes: 22 additions & 4 deletions src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable)
return false;
}

if (!needWriteable)
if (!needWriteable && !EnablePEVerifyCompat())
{
return true;
}
Expand All @@ -398,9 +398,27 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable)

if (!field.IsReadOnly)
{
//PROTOTYPE(verifier): should we dig through struct receivers?
// roField.a.b.c.d.Method() // roField is readonly, all structs
// is it cheaper to copy "d" than "roField", but getting rw ref of roField could upset verifier
// in a case if we have a writeable struct field with a receiver that only has a readable home we would need to pass it via a temp.
// it would be advantageous to make a temp for the field, not for the the outer struct, since the field is smaller and we can get to is by feching references.
// NOTE: this would not be profitable if we have to satisfy verifier, since for verifiability
// we would not be able to dig for the inner field using references and the outer struct will have to be copied to a temp anyways.
if (!EnablePEVerifyCompat())
{
Debug.Assert(needWriteable == true);

var receiver = fieldAccess.ReceiverOpt;
if (receiver?.Type.IsValueType == true)
{
// Check receiver:
// has writeable home -> return true - the whole chain has writeable home (also a more common case)
// has readable home -> return false - we need to copy the field
// otherwise -> return true - the copy will be made at higher level so the leaf field can have writeable home

return HasHome(receiver, needWriteable: true) ||
!HasHome(receiver, needWriteable: false);
}
}

return true;
}

Expand Down
11 changes: 8 additions & 3 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,14 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces
}
else
{
//PROTOTYPE(verifier): this does not need to be writeable
// we may call "HasValue" on this, but it is not mutating
receiverTemp = EmitReceiverRef(receiver, AddressKind.Writeable);
// this does not need to be writeable
// we may call "HasValue" on this, but it is not mutating
// (however verification does not know that and considers all calls mutating)
var addressKind = EnablePEVerifyCompat()?
AddressKind.Writeable:
AddressKind.ReadOnly;

receiverTemp = EmitReceiverRef(receiver, addressKind);
_builder.EmitOpCode(ILOpCode.Dup);
// here we have loaded two copies of a reference { O, O } or {&nub, &nub}
}
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,14 @@ internal override CommonAnonymousTypeManager CommonAnonymousTypeManager
/// </summary>
internal bool FeatureStrictEnabled => Feature("strict") != null;

/// <summary>
/// True when "peverify-compat" is set
/// With this flag we will avoid certain patterns known not be compatible with PEVerify.
/// The code may be less efficient and may deviate from spec in corner cases.
/// The flag is only to be used if PEVerify pass is extremely important.
/// </summary>
internal bool FeaturePEVerifyCompatEnabled => Feature("peverify-compat") != null;

/// <summary>
/// The language version that was used to parse the syntax trees of this compilation.
/// </summary>
Expand Down
132 changes: 131 additions & 1 deletion src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ .locals init (int V_0,
}

[Fact]
[CompilerTrait(CompilerFeature.PEVerifyCompat)]
public void InParamPassRoField()
{
var text = @"
Expand Down Expand Up @@ -213,6 +214,23 @@ .maxstack 1
IL_0000: ldarg.0
IL_0001: ret
}");

comp = CompileAndVerify(text, verify: false, expectedOutput: "42", parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature());

comp.VerifyIL("Program.Main()", @"
{
// Code size 20 (0x14)
.maxstack 1
.locals init (int V_0)
IL_0000: ldsfld ""int Program.F""
IL_0005: stloc.0
IL_0006: ldloca.s V_0
IL_0008: call ""ref readonly int Program.M(in int)""
IL_000d: ldind.i4
IL_000e: call ""void System.Console.WriteLine(int)""
IL_0013: ret
}");

}

[Fact]
Expand Down Expand Up @@ -1082,6 +1100,7 @@ public struct S1
}

[Fact]
[CompilerTrait(CompilerFeature.PEVerifyCompat)]
public void ReadonlyParamAsyncSpillReadOnlyStructThis()
{
var text = @"
Expand Down Expand Up @@ -1164,10 +1183,26 @@ public void M1(in int arg2, in int arg3, in int arg4)
1
42
3
3");

comp = CreateCompilationWithMscorlib46(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature());
CompileAndVerify(comp, verify: false, expectedOutput: @"
3
42
2
3
1
42
2
3
1
42
2
3");
}

[Fact]
[CompilerTrait(CompilerFeature.PEVerifyCompat)]
public void ReadonlyParamAsyncSpillReadOnlyStructThis_NoValCapture()
{
var text = @"
Expand Down Expand Up @@ -1302,7 +1337,102 @@ .locals init (int V_0,
IL_00a9: ret
}
");
}

comp = CreateCompilationWithMscorlib46(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature());
v = CompileAndVerify(comp, verify: true, expectedOutput: @"
1
2
3
4");

// NOTE: s1, s3 and s4 are all directly loaded via ldsflda and not spilled.
v.VerifyIL("Program.<Test>d__5.System.Runtime.CompilerServices.IAsyncStateMachine.MoveNext()", @"
{
// Code size 183 (0xb7)
.maxstack 4
.locals init (int V_0,
S1 V_1,
System.Runtime.CompilerServices.TaskAwaiter<S1> V_2,
S1 V_3,
S1 V_4,
S1 V_5,
System.Exception V_6)
IL_0000: ldarg.0
IL_0001: ldfld ""int Program.<Test>d__5.<>1__state""
IL_0006: stloc.0
.try
{
IL_0007: ldloc.0
IL_0008: brfalse.s IL_0043
IL_000a: ldsfld ""S1 Program.s3""
IL_000f: call ""System.Threading.Tasks.Task<S1> Program.GetT<S1>(S1)""
IL_0014: callvirt ""System.Runtime.CompilerServices.TaskAwaiter<S1> System.Threading.Tasks.Task<S1>.GetAwaiter()""
IL_0019: stloc.2
IL_001a: ldloca.s V_2
IL_001c: call ""bool System.Runtime.CompilerServices.TaskAwaiter<S1>.IsCompleted.get""
IL_0021: brtrue.s IL_005f
IL_0023: ldarg.0
IL_0024: ldc.i4.0
IL_0025: dup
IL_0026: stloc.0
IL_0027: stfld ""int Program.<Test>d__5.<>1__state""
IL_002c: ldarg.0
IL_002d: ldloc.2
IL_002e: stfld ""System.Runtime.CompilerServices.TaskAwaiter<S1> Program.<Test>d__5.<>u__1""
IL_0033: ldarg.0
IL_0034: ldflda ""System.Runtime.CompilerServices.AsyncTaskMethodBuilder Program.<Test>d__5.<>t__builder""
IL_0039: ldloca.s V_2
IL_003b: ldarg.0
IL_003c: call ""void System.Runtime.CompilerServices.AsyncTaskMethodBuilder.AwaitUnsafeOnCompleted<System.Runtime.CompilerServices.TaskAwaiter<S1>, Program.<Test>d__5>(ref System.Runtime.CompilerServices.TaskAwaiter<S1>, ref Program.<Test>d__5)""
IL_0041: leave.s IL_00b6
IL_0043: ldarg.0
IL_0044: ldfld ""System.Runtime.CompilerServices.TaskAwaiter<S1> Program.<Test>d__5.<>u__1""
IL_0049: stloc.2
IL_004a: ldarg.0
IL_004b: ldflda ""System.Runtime.CompilerServices.TaskAwaiter<S1> Program.<Test>d__5.<>u__1""
IL_0050: initobj ""System.Runtime.CompilerServices.TaskAwaiter<S1>""
IL_0056: ldarg.0
IL_0057: ldc.i4.m1
IL_0058: dup
IL_0059: stloc.0
IL_005a: stfld ""int Program.<Test>d__5.<>1__state""
IL_005f: ldloca.s V_2
IL_0061: call ""S1 System.Runtime.CompilerServices.TaskAwaiter<S1>.GetResult()""
IL_0066: stloc.1
IL_0067: ldsfld ""S1 Program.s1""
IL_006c: stloc.3
IL_006d: ldloca.s V_3
IL_006f: ldsfld ""S1 Program.s2""
IL_0074: stloc.s V_4
IL_0076: ldloca.s V_4
IL_0078: ldloca.s V_1
IL_007a: ldsfld ""S1 Program.s4""
IL_007f: stloc.s V_5
IL_0081: ldloca.s V_5
IL_0083: call ""void S1.M1(in S1, in S1, in S1)""
IL_0088: leave.s IL_00a3
}
catch System.Exception
{
IL_008a: stloc.s V_6
IL_008c: ldarg.0
IL_008d: ldc.i4.s -2
IL_008f: stfld ""int Program.<Test>d__5.<>1__state""
IL_0094: ldarg.0
IL_0095: ldflda ""System.Runtime.CompilerServices.AsyncTaskMethodBuilder Program.<Test>d__5.<>t__builder""
IL_009a: ldloc.s V_6
IL_009c: call ""void System.Runtime.CompilerServices.AsyncTaskMethodBuilder.SetException(System.Exception)""
IL_00a1: leave.s IL_00b6
}
IL_00a3: ldarg.0
IL_00a4: ldc.i4.s -2
IL_00a6: stfld ""int Program.<Test>d__5.<>1__state""
IL_00ab: ldarg.0
IL_00ac: ldflda ""System.Runtime.CompilerServices.AsyncTaskMethodBuilder Program.<Test>d__5.<>t__builder""
IL_00b1: call ""void System.Runtime.CompilerServices.AsyncTaskMethodBuilder.SetResult()""
IL_00b6: ret
}
");
}
}
}
Loading

0 comments on commit d3ea5d2

Please sign in to comment.