From ac2adb131350e07f05a99e67251a561ee02a7418 Mon Sep 17 00:00:00 2001 From: vsadov Date: Tue, 1 Aug 2017 15:01:17 -0700 Subject: [PATCH 1/6] - Introduced the flag. - Made readonly fields homeless outside the constructor when flag is set. --- .../CSharp/Portable/CodeGen/CodeGenerator.cs | 6 + .../CSharp/Portable/CodeGen/EmitAddress.cs | 2 +- .../Portable/Compilation/CSharpCompilation.cs | 2 + .../Emit/CodeGen/CodeGenInParametersTests.cs | 129 +++++++++++++++++- .../CodeGen/CodeGenReadonlyStructTests.cs | 113 +++++++++++++++ .../CodeGen/CodeGenRefReadonlyReturnTests.cs | 42 ++++++ .../CSharp/Test/Emit/CodeGen/CodeGenTests.cs | 33 +++++ .../Test/Utilities/CSharp/TestOptions.cs | 5 + 8 files changed, 330 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs index 7e265ab65ab8c..83a44820e6928 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs @@ -137,6 +137,12 @@ private bool IsDebugPlus() return _module.Compilation.Options.DebugPlusMode; } + private bool IsPEVerifyCompatible() + { + //return true; + return _module.Compilation.FeaturePEVerifyCompatEnabled; + } + private LocalDefinition LazyReturnTemp { get diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs index 16e074f09ae65..6179c7fefb5d9 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs @@ -385,7 +385,7 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable) return false; } - if (!needWriteable) + if (!needWriteable && !IsPEVerifyCompatible()) { return true; } diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index b36a7d30ec7b1..d59dde8c8167a 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -163,6 +163,8 @@ internal override CommonAnonymousTypeManager CommonAnonymousTypeManager /// internal bool FeatureStrictEnabled => Feature("strict") != null; + internal bool FeaturePEVerifyCompatEnabled => Feature("peverify-compat") != null; + /// /// The language version that was used to parse the syntax trees of this compilation. /// diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs index f5d5c3e753ca0..82c7eaee87954 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs @@ -213,6 +213,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] @@ -1164,6 +1181,21 @@ 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"); } @@ -1302,7 +1334,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.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 V_2, + S1 V_3, + S1 V_4, + S1 V_5, + System.Exception V_6) + IL_0000: ldarg.0 + IL_0001: ldfld ""int Program.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 Program.GetT(S1)"" + IL_0014: callvirt ""System.Runtime.CompilerServices.TaskAwaiter System.Threading.Tasks.Task.GetAwaiter()"" + IL_0019: stloc.2 + IL_001a: ldloca.s V_2 + IL_001c: call ""bool System.Runtime.CompilerServices.TaskAwaiter.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.d__5.<>1__state"" + IL_002c: ldarg.0 + IL_002d: ldloc.2 + IL_002e: stfld ""System.Runtime.CompilerServices.TaskAwaiter Program.d__5.<>u__1"" + IL_0033: ldarg.0 + IL_0034: ldflda ""System.Runtime.CompilerServices.AsyncTaskMethodBuilder Program.d__5.<>t__builder"" + IL_0039: ldloca.s V_2 + IL_003b: ldarg.0 + IL_003c: call ""void System.Runtime.CompilerServices.AsyncTaskMethodBuilder.AwaitUnsafeOnCompleted, Program.d__5>(ref System.Runtime.CompilerServices.TaskAwaiter, ref Program.d__5)"" + IL_0041: leave.s IL_00b6 + IL_0043: ldarg.0 + IL_0044: ldfld ""System.Runtime.CompilerServices.TaskAwaiter Program.d__5.<>u__1"" + IL_0049: stloc.2 + IL_004a: ldarg.0 + IL_004b: ldflda ""System.Runtime.CompilerServices.TaskAwaiter Program.d__5.<>u__1"" + IL_0050: initobj ""System.Runtime.CompilerServices.TaskAwaiter"" + IL_0056: ldarg.0 + IL_0057: ldc.i4.m1 + IL_0058: dup + IL_0059: stloc.0 + IL_005a: stfld ""int Program.d__5.<>1__state"" + IL_005f: ldloca.s V_2 + IL_0061: call ""S1 System.Runtime.CompilerServices.TaskAwaiter.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.d__5.<>1__state"" + IL_0094: ldarg.0 + IL_0095: ldflda ""System.Runtime.CompilerServices.AsyncTaskMethodBuilder Program.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.d__5.<>1__state"" + IL_00ab: ldarg.0 + IL_00ac: ldflda ""System.Runtime.CompilerServices.AsyncTaskMethodBuilder Program.d__5.<>t__builder"" + IL_00b1: call ""void System.Runtime.CompilerServices.AsyncTaskMethodBuilder.SetResult()"" + IL_00b6: ret +} +"); + } } } diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs index 10d243d947eb4..f1c024e809ea3 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs @@ -57,6 +57,27 @@ .maxstack 1 IL_001f: call ""void System.Console.Write(string)"" IL_0024: ret }"); + + comp = CompileAndVerify(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature(), verify: true, expectedOutput: @"12"); + + comp.VerifyIL("Program.Main", @" +{ + // Code size 43 (0x2b) + .maxstack 1 + .locals init (Program.S1 V_0) + IL_0000: ldsfld ""Program.S1 Program.sf"" + IL_0005: stloc.0 + IL_0006: ldloca.s V_0 + IL_0008: call ""string Program.S1.M1()"" + IL_000d: call ""void System.Console.Write(string)"" + IL_0012: ldsfld ""Program.S1 Program.sf"" + IL_0017: stloc.0 + IL_0018: ldloca.s V_0 + IL_001a: constrained. ""Program.S1"" + IL_0020: callvirt ""string object.ToString()"" + IL_0025: call ""void System.Console.Write(string)"" + IL_002a: ret +}"); } [Fact] @@ -106,6 +127,27 @@ .maxstack 1 IL_001f: call ""void System.Console.Write(string)"" IL_0024: ret }"); + + comp = CompileAndVerify(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef, ref1 }, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature(), verify: true, expectedOutput: @"12"); + + comp.VerifyIL("Program.Main", @" +{ + // Code size 43 (0x2b) + .maxstack 1 + .locals init (S1 V_0) + IL_0000: ldsfld ""S1 Program.sf"" + IL_0005: stloc.0 + IL_0006: ldloca.s V_0 + IL_0008: call ""string S1.M1()"" + IL_000d: call ""void System.Console.Write(string)"" + IL_0012: ldsfld ""S1 Program.sf"" + IL_0017: stloc.0 + IL_0018: ldloca.s V_0 + IL_001a: constrained. ""S1"" + IL_0020: callvirt ""string object.ToString()"" + IL_0025: call ""void System.Console.Write(string)"" + IL_002a: ret +}"); } [Fact] @@ -152,6 +194,29 @@ .maxstack 2 IL_0025: call ""void System.Console.Write(string)"" IL_002a: ret }"); + + comp = CompileAndVerify(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature(), verify: true, expectedOutput: @"12"); + + comp.VerifyIL("Program.Main", @" +{ + // Code size 49 (0x31) + .maxstack 2 + .locals init (Program.S1 V_0) + IL_0000: newobj ""Program..ctor()"" + IL_0005: dup + IL_0006: ldfld ""Program.S1 Program.f"" + IL_000b: stloc.0 + IL_000c: ldloca.s V_0 + IL_000e: call ""string Program.S1.M1()"" + IL_0013: call ""void System.Console.Write(string)"" + IL_0018: ldfld ""Program.S1 Program.f"" + IL_001d: stloc.0 + IL_001e: ldloca.s V_0 + IL_0020: constrained. ""Program.S1"" + IL_0026: callvirt ""string object.ToString()"" + IL_002b: call ""void System.Console.Write(string)"" + IL_0030: ret +}"); } [Fact] @@ -202,6 +267,30 @@ .maxstack 3 IL_002a: call ""void System.Console.Write(string)"" IL_002f: ret }"); + + comp = CompileAndVerify(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature(), verify: true, expectedOutput: @"hello2"); + + comp.VerifyIL("Program.Main", @" +{ + // Code size 54 (0x36) + .maxstack 3 + .locals init (Program.S1 V_0) + IL_0000: newobj ""Program..ctor()"" + IL_0005: dup + IL_0006: ldfld ""Program.S1 Program.f"" + IL_000b: stloc.0 + IL_000c: ldloca.s V_0 + IL_000e: ldstr ""hello"" + IL_0013: call ""string Program.S1.M1(string)"" + IL_0018: call ""void System.Console.Write(string)"" + IL_001d: ldfld ""Program.S1 Program.f"" + IL_0022: stloc.0 + IL_0023: ldloca.s V_0 + IL_0025: constrained. ""Program.S1"" + IL_002b: callvirt ""string object.ToString()"" + IL_0030: call ""void System.Console.Write(string)"" + IL_0035: ret +}"); } [Fact] @@ -257,6 +346,30 @@ .maxstack 3 IL_002a: call ""void System.Console.Write(string)"" IL_002f: ret }"); + + comp = CompileAndVerify(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef, ref1 }, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature(), verify: true, expectedOutput: @"hello2"); + + comp.VerifyIL("Program.Main", @" +{ + // Code size 54 (0x36) + .maxstack 3 + .locals init (S1 V_0) + IL_0000: newobj ""Program..ctor()"" + IL_0005: dup + IL_0006: ldfld ""S1 Program.f"" + IL_000b: stloc.0 + IL_000c: ldloca.s V_0 + IL_000e: ldstr ""hello"" + IL_0013: call ""string S1.M1(string)"" + IL_0018: call ""void System.Console.Write(string)"" + IL_001d: ldfld ""S1 Program.f"" + IL_0022: stloc.0 + IL_0023: ldloca.s V_0 + IL_0025: constrained. ""S1"" + IL_002b: callvirt ""string object.ToString()"" + IL_0030: call ""void System.Console.Write(string)"" + IL_0035: ret +}"); } [Fact] diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs index 5aef2b906c28f..daaa8861298fc 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs @@ -485,6 +485,48 @@ .locals init (bool V_0) //b IL_002e: ldflda ""(int Alice, int Bob) Program.S.F1"" IL_0033: ldflda ""int System.ValueTuple.Item1"" IL_0038: ret +}"); + comp = CompileAndVerify(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature(), verify: false); + + comp.VerifyIL("Program.Test", @" +{ + // Code size 70 (0x46) + .maxstack 1 + .locals init (bool V_0, //b + int V_1, + System.ValueTuple V_2, + int V_3, + System.ValueTuple V_4) + IL_0000: ldc.i4.1 + IL_0001: stloc.0 + IL_0002: ldloc.0 + IL_0003: brfalse.s IL_0020 + IL_0005: ldloc.0 + IL_0006: brfalse.s IL_0012 + IL_0008: ldarg.0 + IL_0009: ldfld ""int Program.F"" + IL_000e: stloc.1 + IL_000f: ldloca.s V_1 + IL_0011: ret + IL_0012: ldsfld ""(int Alice, int Bob) Program.F1"" + IL_0017: stloc.2 + IL_0018: ldloca.s V_2 + IL_001a: ldflda ""int System.ValueTuple.Item1"" + IL_001f: ret + IL_0020: ldloc.0 + IL_0021: brfalse.s IL_0032 + IL_0023: ldarg.0 + IL_0024: ldfld ""Program.S Program.S1"" + IL_0029: ldfld ""int Program.S.F"" + IL_002e: stloc.3 + IL_002f: ldloca.s V_3 + IL_0031: ret + IL_0032: ldsfld ""Program.S Program.S2"" + IL_0037: ldfld ""(int Alice, int Bob) Program.S.F1"" + IL_003c: stloc.s V_4 + IL_003e: ldloca.s V_4 + IL_0040: ldflda ""int System.ValueTuple.Item1"" + IL_0045: ret }"); } diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs index 269f94a9f846e..d62b4808f3d16 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs @@ -12055,6 +12055,39 @@ .locals init (MyManagedStruct V_0) IL_0046: call ""void System.Console.WriteLine(int)"" IL_004b: ret } +"); + + compilation = CompileAndVerify(source, expectedOutput: @"42", verify: true, parseOptions:TestOptions.Regular.WithPEVerifyCompatFeature()); + + // Dev10 + compilation.VerifyIL("Program.Main", +@" +{ + // Code size 76 (0x4c) + .maxstack 3 + .locals init (MyManagedStruct V_0) + IL_0000: newobj ""cls1..ctor()"" + IL_0005: dup + IL_0006: ldfld ""MyManagedStruct cls1.y"" + IL_000b: stloc.0 + IL_000c: ldloca.s V_0 + IL_000e: ldc.i4.s 123 + IL_0010: call ""void MyManagedStruct.mutate(int)"" + IL_0015: dup + IL_0016: ldfld ""MyManagedStruct cls1.y"" + IL_001b: stloc.0 + IL_001c: ldloca.s V_0 + IL_001e: ldflda ""MyManagedStruct.Nested MyManagedStruct.n"" + IL_0023: ldflda ""MyManagedStruct.Nested.Nested1 MyManagedStruct.Nested.n"" + IL_0028: ldc.i4 0x1c8 + IL_002d: call ""void MyManagedStruct.Nested.Nested1.mutate(int)"" + IL_0032: ldfld ""MyManagedStruct cls1.y"" + IL_0037: ldfld ""MyManagedStruct.Nested MyManagedStruct.n"" + IL_003c: ldfld ""MyManagedStruct.Nested.Nested1 MyManagedStruct.Nested.n"" + IL_0041: ldfld ""int MyManagedStruct.Nested.Nested1.num"" + IL_0046: call ""void System.Console.WriteLine(int)"" + IL_004b: ret +} "); } diff --git a/src/Compilers/Test/Utilities/CSharp/TestOptions.cs b/src/Compilers/Test/Utilities/CSharp/TestOptions.cs index cf98ad0462a6a..eb23668520f24 100644 --- a/src/Compilers/Test/Utilities/CSharp/TestOptions.cs +++ b/src/Compilers/Test/Utilities/CSharp/TestOptions.cs @@ -58,6 +58,11 @@ public static CSharpParseOptions WithStrictFeature(this CSharpParseOptions optio return options.WithFeatures(options.Features.Concat(new[] { new KeyValuePair("strict", "true") })); } + public static CSharpParseOptions WithPEVerifyCompatFeature(this CSharpParseOptions options) + { + return options.WithFeatures(options.Features.Concat(new[] { new KeyValuePair("peverify-compat", "true") })); + } + public static CSharpParseOptions WithLocalFunctionsFeature(this CSharpParseOptions options) { return options; From 6b3776b5082632edb4143324f63e343174afb6ef Mon Sep 17 00:00:00 2001 From: vsadov Date: Tue, 1 Aug 2017 16:31:43 -0700 Subject: [PATCH 2/6] one more case for ?. --- .../CSharp/Portable/CodeGen/EmitExpression.cs | 11 ++- .../CodeGenShortCircuitOperatorTests.cs | 77 ++++++++++++------- 2 files changed, 59 insertions(+), 29 deletions(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs index 0542440a0659c..53b7dddedc422 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs @@ -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 = IsPEVerifyCompatible()? + 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} } diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs index e6f3e1ee3e8ad..ef929827394e0 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs @@ -7236,33 +7236,58 @@ static void Main() } } "; - var verifier = CompileAndVerify(source, options: TestOptions.DebugExe, expectedOutput: @""); + var comp = CompileAndVerify(source, options: TestOptions.DebugExe, expectedOutput: @"", verify: false); - verifier.VerifyIL("Program.Main", @" - { - // Code size 47 (0x2f) - .maxstack 2 - .locals init (System.Guid? V_0, - System.Guid V_1) - IL_0000: nop - IL_0001: ldsfld ""System.Guid? Program.g"" - IL_0006: stloc.0 - IL_0007: ldloca.s V_0 - IL_0009: dup - IL_000a: call ""bool System.Guid?.HasValue.get"" - IL_000f: brtrue.s IL_0015 - IL_0011: pop - IL_0012: ldnull - IL_0013: br.s IL_0028 - IL_0015: call ""System.Guid System.Guid?.GetValueOrDefault()"" - IL_001a: stloc.1 - IL_001b: ldloca.s V_1 - IL_001d: constrained. ""System.Guid"" - IL_0023: callvirt ""string object.ToString()"" - IL_0028: call ""void System.Console.WriteLine(string)"" - IL_002d: nop - IL_002e: ret - }"); + comp.VerifyIL("Program.Main", @" +{ + // Code size 44 (0x2c) + .maxstack 2 + .locals init (System.Guid V_0) + IL_0000: nop + IL_0001: ldsflda ""System.Guid? Program.g"" + IL_0006: dup + IL_0007: call ""bool System.Guid?.HasValue.get"" + IL_000c: brtrue.s IL_0012 + IL_000e: pop + IL_000f: ldnull + IL_0010: br.s IL_0025 + IL_0012: call ""System.Guid System.Guid?.GetValueOrDefault()"" + IL_0017: stloc.0 + IL_0018: ldloca.s V_0 + IL_001a: constrained. ""System.Guid"" + IL_0020: callvirt ""string object.ToString()"" + IL_0025: call ""void System.Console.WriteLine(string)"" + IL_002a: nop + IL_002b: ret +}"); + + comp = CompileAndVerify(source, options: TestOptions.DebugExe, expectedOutput: @"", parseOptions:TestOptions.Regular.WithPEVerifyCompatFeature(), verify: true); + + comp.VerifyIL("Program.Main", @" +{ + // Code size 47 (0x2f) + .maxstack 2 + .locals init (System.Guid? V_0, + System.Guid V_1) + IL_0000: nop + IL_0001: ldsfld ""System.Guid? Program.g"" + IL_0006: stloc.0 + IL_0007: ldloca.s V_0 + IL_0009: dup + IL_000a: call ""bool System.Guid?.HasValue.get"" + IL_000f: brtrue.s IL_0015 + IL_0011: pop + IL_0012: ldnull + IL_0013: br.s IL_0028 + IL_0015: call ""System.Guid System.Guid?.GetValueOrDefault()"" + IL_001a: stloc.1 + IL_001b: ldloca.s V_1 + IL_001d: constrained. ""System.Guid"" + IL_0023: callvirt ""string object.ToString()"" + IL_0028: call ""void System.Console.WriteLine(string)"" + IL_002d: nop + IL_002e: ret +}"); } [Fact] From bc819febc9d8167653f9b26df1163f733d2d781a Mon Sep 17 00:00:00 2001 From: vsadov Date: Wed, 2 Aug 2017 13:04:03 -0700 Subject: [PATCH 3/6] Another case for nested structs --- .../CSharp/Portable/CodeGen/EmitAddress.cs | 24 +++- .../CSharp/Test/Emit/CodeGen/CodeGenTests.cs | 135 ++++++++++++++++-- 2 files changed, 144 insertions(+), 15 deletions(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs index 6179c7fefb5d9..dddfb44bab9cc 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs @@ -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 (!IsPEVerifyCompatible()) + { + 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; } diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs index d62b4808f3d16..63179285c2d63 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs @@ -12024,15 +12024,15 @@ public MyManagedStruct(int x) n.n.num = x; } }"; - var compilation = CompileAndVerify(source, expectedOutput: @"42", verify: false); + var comp = CompileAndVerify(source, expectedOutput: @"42", verify: false); - // Dev10 - compilation.VerifyIL("Program.Main", + comp.VerifyIL("Program.Main", @" { // Code size 76 (0x4c) .maxstack 3 - .locals init (MyManagedStruct V_0) + .locals init (MyManagedStruct V_0, + MyManagedStruct.Nested.Nested1 V_1) IL_0000: newobj ""cls1..ctor()"" IL_0005: dup IL_0006: ldfld ""MyManagedStruct cls1.y"" @@ -12041,11 +12041,11 @@ .locals init (MyManagedStruct V_0) IL_000e: ldc.i4.s 123 IL_0010: call ""void MyManagedStruct.mutate(int)"" IL_0015: dup - IL_0016: ldfld ""MyManagedStruct cls1.y"" - IL_001b: stloc.0 - IL_001c: ldloca.s V_0 - IL_001e: ldflda ""MyManagedStruct.Nested MyManagedStruct.n"" - IL_0023: ldflda ""MyManagedStruct.Nested.Nested1 MyManagedStruct.Nested.n"" + IL_0016: ldflda ""MyManagedStruct cls1.y"" + IL_001b: ldflda ""MyManagedStruct.Nested MyManagedStruct.n"" + IL_0020: ldfld ""MyManagedStruct.Nested.Nested1 MyManagedStruct.Nested.n"" + IL_0025: stloc.1 + IL_0026: ldloca.s V_1 IL_0028: ldc.i4 0x1c8 IL_002d: call ""void MyManagedStruct.Nested.Nested1.mutate(int)"" IL_0032: ldflda ""MyManagedStruct cls1.y"" @@ -12057,10 +12057,9 @@ .locals init (MyManagedStruct V_0) } "); - compilation = CompileAndVerify(source, expectedOutput: @"42", verify: true, parseOptions:TestOptions.Regular.WithPEVerifyCompatFeature()); + comp = CompileAndVerify(source, expectedOutput: @"42", verify: true, parseOptions:TestOptions.Regular.WithPEVerifyCompatFeature()); - // Dev10 - compilation.VerifyIL("Program.Main", + comp.VerifyIL("Program.Main", @" { // Code size 76 (0x4c) @@ -12091,6 +12090,118 @@ .locals init (MyManagedStruct V_0) "); } + public void MutateReadonlyNested1() + { + string source = @" + + class Program + { + static void Main(string[] args) + { + GetRoRef().ro.ro.ro.ro.ToString(); + System.Console.Write(GetRoRef().ro.ro.ro.ro.x); + } + + private static ref readonly Largest GetRoRef() + { + return ref (new Largest[1])[0]; + } + } + + struct Largest + { + public int x; + public readonly Large ro; + } + + struct Large + { + public int x; + public Medium ro; + } + + struct Medium + { + public int x; + public Small ro; + } + + struct Small + { + public int x; + public Smallest ro; + } + + struct Smallest + { + public int x; + + public override string ToString() + { + x = -1; + System.Console.Write(x); + return null; + } + }"; + var comp = CompileAndVerify(source, expectedOutput: @"-10", verify: false); + + comp.VerifyIL("Program.Main", +@" +{ + // Code size 76 (0x4c) + .maxstack 1 + .locals init (Smallest V_0) + IL_0000: call ""ref readonly Largest Program.GetRoRef()"" + IL_0005: ldflda ""Large Largest.ro"" + IL_000a: ldflda ""Medium Large.ro"" + IL_000f: ldflda ""Small Medium.ro"" + IL_0014: ldfld ""Smallest Small.ro"" + IL_0019: stloc.0 + IL_001a: ldloca.s V_0 + IL_001c: constrained. ""Smallest"" + IL_0022: callvirt ""string object.ToString()"" + IL_0027: pop + IL_0028: call ""ref readonly Largest Program.GetRoRef()"" + IL_002d: ldflda ""Large Largest.ro"" + IL_0032: ldflda ""Medium Large.ro"" + IL_0037: ldflda ""Small Medium.ro"" + IL_003c: ldflda ""Smallest Small.ro"" + IL_0041: ldfld ""int Smallest.x"" + IL_0046: call ""void System.Console.Write(int)"" + IL_004b: ret +} +"); + + comp = CompileAndVerify(source, expectedOutput: @"-10", verify: true, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature()); + + comp.VerifyIL("Program.Main", +@" + { + // Code size 76 (0x4c) + .maxstack 1 + .locals init (Large V_0) + IL_0000: call ""ref readonly Largest Program.GetRoRef()"" + IL_0005: ldfld ""Large Largest.ro"" + IL_000a: stloc.0 + IL_000b: ldloca.s V_0 + IL_000d: ldflda ""Medium Large.ro"" + IL_0012: ldflda ""Small Medium.ro"" + IL_0017: ldflda ""Smallest Small.ro"" + IL_001c: constrained. ""Smallest"" + IL_0022: callvirt ""string object.ToString()"" + IL_0027: pop + IL_0028: call ""ref readonly Largest Program.GetRoRef()"" + IL_002d: ldfld ""Large Largest.ro"" + IL_0032: ldfld ""Medium Large.ro"" + IL_0037: ldfld ""Small Medium.ro"" + IL_003c: ldfld ""Smallest Small.ro"" + IL_0041: ldfld ""int Smallest.x"" + IL_0046: call ""void System.Console.Write(int)"" + IL_004b: ret + } +"); + } + [Fact] public void LocalNumericConstToString() { From 11fc778645e4b6c3bedc55487568624a0d715f0a Mon Sep 17 00:00:00 2001 From: vsadov Date: Wed, 2 Aug 2017 15:05:13 -0700 Subject: [PATCH 4/6] remove commented out code used for testing. --- src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs index 83a44820e6928..6bb85c86482a1 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs @@ -139,7 +139,6 @@ private bool IsDebugPlus() private bool IsPEVerifyCompatible() { - //return true; return _module.Compilation.FeaturePEVerifyCompatEnabled; } From b05c21a0a7732540f338acf0daa7c9be75ea30e6 Mon Sep 17 00:00:00 2001 From: vsadov Date: Fri, 4 Aug 2017 12:49:43 -0700 Subject: [PATCH 5/6] PR feedback --- src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs | 2 +- src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs | 4 ++-- src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs | 2 +- .../CSharp/Portable/Compilation/CSharpCompilation.cs | 6 ++++++ .../CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs | 3 +++ .../CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs | 5 +++++ .../Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs | 1 + .../Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs | 1 + src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs | 3 +++ src/Test/Utilities/Portable/Traits/CompilerFeature.cs | 1 + 10 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs index 6bb85c86482a1..d6a58243f0d5a 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs @@ -137,7 +137,7 @@ private bool IsDebugPlus() return _module.Compilation.Options.DebugPlusMode; } - private bool IsPEVerifyCompatible() + private bool EnablePEVerifyCompat() { return _module.Compilation.FeaturePEVerifyCompatEnabled; } diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs index dddfb44bab9cc..df34a3785a12a 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs @@ -385,7 +385,7 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable) return false; } - if (!needWriteable && !IsPEVerifyCompatible()) + if (!needWriteable && !EnablePEVerifyCompat()) { return true; } @@ -402,7 +402,7 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable) // 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 (!IsPEVerifyCompatible()) + if (!EnablePEVerifyCompat()) { Debug.Assert(needWriteable == true); diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs index 53b7dddedc422..70d9ab7acc7f4 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs @@ -427,7 +427,7 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces // 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 = IsPEVerifyCompatible()? + var addressKind = EnablePEVerifyCompat()? AddressKind.Writeable: AddressKind.ReadOnly; diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index d59dde8c8167a..96a7a74e82fd2 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -163,6 +163,12 @@ internal override CommonAnonymousTypeManager CommonAnonymousTypeManager /// internal bool FeatureStrictEnabled => Feature("strict") != null; + /// + /// 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. + /// internal bool FeaturePEVerifyCompatEnabled => Feature("peverify-compat") != null; /// diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs index 82c7eaee87954..17e64fb5fcee3 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs @@ -173,6 +173,7 @@ .locals init (int V_0, } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void InParamPassRoField() { var text = @" @@ -1099,6 +1100,7 @@ public struct S1 } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void ReadonlyParamAsyncSpillReadOnlyStructThis() { var text = @" @@ -1200,6 +1202,7 @@ public void M1(in int arg2, in int arg3, in int arg4) } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void ReadonlyParamAsyncSpillReadOnlyStructThis_NoValCapture() { var text = @" diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs index f1c024e809ea3..d39f7393af424 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs @@ -17,6 +17,7 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests public class CodeGenReadOnlyStructTests : CompilingTestBase { [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void InvokeOnReadOnlyStaticField() { var text = @" @@ -81,6 +82,7 @@ .locals init (Program.S1 V_0) } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void InvokeOnReadOnlyStaticFieldMetadata() { var text1 = @" @@ -151,6 +153,7 @@ .locals init (S1 V_0) } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void InvokeOnReadOnlyInstanceField() { var text = @" @@ -220,6 +223,7 @@ .locals init (Program.S1 V_0) } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void InvokeOnReadOnlyInstanceFieldGeneric() { var text = @" @@ -294,6 +298,7 @@ .locals init (Program.S1 V_0) } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void InvokeOnReadOnlyInstanceFieldGenericMetadata() { var text1 = @" diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs index daaa8861298fc..7f9b2fddcb660 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs @@ -408,6 +408,7 @@ .locals init (bool V_0) //b } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void ReadonlyFieldCanReturnByRefReadonly() { var text = @" diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs index ef929827394e0..eef16c5ab1567 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs @@ -7221,6 +7221,7 @@ .locals init (T V_0, //v } [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void ConditionalAccessOffReadOnlyNullable1() { var source = @" diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs index 63179285c2d63..5406682f7420f 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs @@ -11973,6 +11973,7 @@ public override V PrintField(V v, W w) #endregion [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void MutateReadonlyNested() { string source = @" @@ -12090,6 +12091,8 @@ .locals init (MyManagedStruct V_0) "); } + [Fact] + [CompilerTrait(CompilerFeature.PEVerifyCompat)] public void MutateReadonlyNested1() { string source = @" diff --git a/src/Test/Utilities/Portable/Traits/CompilerFeature.cs b/src/Test/Utilities/Portable/Traits/CompilerFeature.cs index faed61585a64f..15fb04c3fa233 100644 --- a/src/Test/Utilities/Portable/Traits/CompilerFeature.cs +++ b/src/Test/Utilities/Portable/Traits/CompilerFeature.cs @@ -23,5 +23,6 @@ public enum CompilerFeature Patterns, DefaultLiteral, AsyncMain, + PEVerifyCompat, } } From 243afaf72e3468c6c16d9e146bb72ccb4edf44cf Mon Sep 17 00:00:00 2001 From: vsadov Date: Fri, 4 Aug 2017 13:51:53 -0700 Subject: [PATCH 6/6] missing using --- .../CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs index eef16c5ab1567..0b27e42cd4c47 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenShortCircuitOperatorTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.CodeAnalysis.CSharp.Test.Utilities; +using Microsoft.CodeAnalysis.Test.Utilities; using Roslyn.Test.Utilities; using Xunit;