Enable peverify-compat mode for langver < 7.2 #22772

VSadov · 2017-10-19T19:50:21Z

Ensures that the behavior of the compiler with respect to verification compat is:

Langver < 7.2 enables compat mode automatically
Fature flag ”peverify-compat” enables compat mode
In compat mode - no unverifiable optimizations in code that used to verify in the past.
In compat mode - new intrinsically unverifiable features/patterns work correctly, even if unverifiable code needs to be emitted (correctness wins in this case).

Customer scenario

This change primarily impacts users that use CAS attributes such as [SecurityTransparent] in the code. The situation is not common, but the experience could be bad enough to be an adoption blocker.

user upgrades to the new compiler and upon recompile app crashes with security exceptions even though nothing whatsoever changed.

Note -
User may keep langver < 7.2 as it was, in such case indeed nothing changed, or
user may switch to langver == 7.2 or latest, but not use new features or use only a subset such
as relaxed rules for named arguments.
In either case user may need to have a way to compile code into something that does not crash.

Bugs this fixes:

#22707

Workarounds, if any

User may use feature flag "peverify-compat", but if he uses new features, that may result in code that is both not verifiable and not correct.

Ideally, as long as langver is the same as before, no changes should be required.
Also presence of the flag should not result in bad codegen for new features.

Basically without this fix user will 1) have to set the flag and then 2) end up in a strict either/or situation for the new features.

Even if not common [SecurityTransparent] code still exists, for whatever reasons. We have already had 3 separate reports about breaks as described above.
There is a concern that experience as it is now could become an adoption blocker for a subset of customers, expecialy with large legacy code bases.

Risk

Risk is low since this is basically enabling a compat mode automatically in situations when user already can enable it.
The change also makes the compat mode to not interfere with new code. That is specifically to reduce the risk of using the compat mode.

Performance impact

Low, since most changes are just around choosing different IL emit strategies based on a flag or a langver value.
There are no new allocations or complicated logic.

Is this a regression from a previous update?

No.

Root cause analysis:

Early feedback from previews and internal use, such as using new compiler in CoreFx, reveals that while use of features like [SecurityTransparent] is uncommon, it does exist and user experience is not as good as we would like.
In the light of this new data the risk of leaving the situation as-is now seems higher.

How was the bug found?

Build beaks in System.Immutable.Collections in CoreFx repo
Customer reports.

Test documentation updated?
N/A
new tests added.

VSO bug:
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=514815&_a=edit

…y-compat.

MeiChin-Tsai · 2017-10-25T18:36:52Z

Please get code reviewed. Approved for the fix as long as code reviewers have no objection.

agocke

Do you have any ref readonly local tests?

agocke · 2017-10-25T19:15:36Z

src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs

@@ -139,7 +139,7 @@ private bool IsDebugPlus()

        private bool EnablePEVerifyCompat()


This reads like a mutating method, but is actually flag-returning. How about IsPEVerifyCompatEnabled? #WontFix

Someone suggested this name in past CR. I would be ok with renaming this and perhaps other helpers (like "HasValue") to be more matching with their purpose, but not in this change. We can rename/refactor later.

In reply to: 146959260 [](ancestors = 146959260)

agocke · 2017-10-25T19:19:04Z

src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs

@@ -339,7 +345,7 @@ private LocalSymbol DigForValueLocal(BoundSequence topSequence, BoundExpression
        /// Checks if expression directly or indirectly represents a value with its own home. In
        /// such cases it is possible to get a reference without loading into a temporary.
        /// </summary>
-        private bool HasHome(BoundExpression expression, bool needWriteable)
+        private bool HasHome(BoundExpression expression, AddressKind addressKind)


addressKind here refers to the destination, so maybe call it destinationAddressKind or requiredAddressKind? #WontFix

I would like to rename this "HasHome" to something like "CanGetDirectReference" or similar (in a separate clean-up change). I started doing that, but reverted since that does not need to be in 15.5 and just makes the PR larger than it needs to be.
Then "requiredAddressKind" would be Ok.

In reply to: 146960213 [](ancestors = 146960213)

agocke · 2017-10-25T19:22:40Z

src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs

@@ -437,7 +444,14 @@ private bool HasHome(BoundFieldAccess fieldAccess, bool needWriteable)
                return false;
            }

-            if (!needWriteable && !EnablePEVerifyCompat())
+            // when reference is strictly required, consider fields as addressable


I don't think this comment says what you mean. Even if we copy into a temp, you're going to get an address back. Really, this is a statement about not being allowed to copy. #Resolved

Yeah. The idea is that direct reference is strongly desired.

In reply to: 146961206 [](ancestors = 146961206)

agocke · 2017-10-25T19:23:58Z

src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs

+            //NOTE: we are not propagating AddressKind.Constrained here.
+            //      the reason is that while Constrained permits calls, it does not permit 
+            //      taking field addresses, so we have to turn Constrained into writeable.
+            var tempOpt = EmitReceiverRef(fieldAccess.ReceiverOpt, addressKind == AddressKind.Constrained? AddressKind.Writeable: addressKind);


Strange formatting around the ?: operator. #Resolved

agocke · 2017-10-25T19:26:03Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

+                }
+                else
+                {
+                    // vararg case


Consider asserting that the last parameter is a vararg here. #Resolved

agocke · 2017-10-25T19:28:33Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

-                LocalDefinition temp = EmitAddress(assignmentOperator.Right, AddressKind.ReadOnly);
+                // NOTE: passing "ReadOnlyStrict" here. 
+                //       we should not get an address of a copy if at all possible
+                LocalDefinition temp = EmitAddress(assignmentOperator.Right, local.RefKind == RefKind.RefReadOnly ? AddressKind.ReadOnlyStrict: AddressKind.Writeable);


Missing a space before the : #Resolved

agocke · 2017-10-25T19:30:05Z

src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs

@@ -724,7 +724,9 @@ private void EmitReturnStatement(BoundReturnStatement boundReturnStatement)
            }
            else
            {
-                this.EmitAddress(expressionOpt, this._method.RefKind == RefKind.RefReadOnly? AddressKind.ReadOnly: AddressKind.Writeable);
+                // NOTE: passing "ReadOnlyStrict" here. 
+                //       we should not get an address of a copy if at all possible


I would make a stronger statement: we cannot ever return the address of a copy. #Resolved

cston · 2017-10-25T19:37:06Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs

+  IL_000a:  ldind.i4
+  IL_000b:  call       ""void System.Console.WriteLine(int)""
+  IL_0010:  ret
+}");


Main is identical in both cases. Should there be a difference in M(in int)? (We're not checking the IL for that method.) #ByDesign

That is the point of the test - main passes a readonly field as an "in" parameter. The only way to do it correctly is unverifiable, therefore the presence of the compat flag should not change IL of Main. - if we see different IL, we would have a bug (that was a case before this fix).

M is not very interesting here. Unlike with fields, verifier does not care if parameter is "in".

In reply to: 146964668 [](ancestors = 146964668)

cston · 2017-10-25T19:39:49Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs

+            CompileAndVerify(comp, verify: false, expectedOutput: @"47");
+
+            comp = CreateCompilationWithMscorlib46(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, options: TestOptions.UnsafeReleaseExe, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature());
+            CompileAndVerify(comp, verify: false, expectedOutput: @"47");


Consider verifying IL for the affected methods. #Resolved

cston · 2017-10-25T19:40:43Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs

+
+";
+
+            var comp = CreateCompilationWithMscorlib46(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, options: TestOptions.UnsafeReleaseExe);


Are the references necessary? #WontFix

I just copied from other test. They will not make it a better or worse, so I will let them be.

In reply to: 146965555 [](ancestors = 146965555)

agocke

I see the ref readonly local tests now. LGTM

jcouv · 2017-10-30T18:59:46Z

What's the status on this PR? Are we waiting for shiproom approval?

jcouv · 2017-11-03T17:01:26Z

Merging asap per pre-approval in https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_queries?id=514815

jcouv · 2017-11-03T17:50:41Z

test perf_correctness_prtest please

natidea · 2017-11-03T18:09:24Z

Approved to merge via https://devdiv.visualstudio.com/DevDiv/_workitems/edit/514815

VSadov · 2017-11-03T19:20:45Z

Thanks!!!

* dotnet/master: (36 commits) Remove usage of IOperation feature flag in newly added unit tests More fixes for IOperation tree. (dotnet#23028) Address PR feedback and remove unused resource and comment out unused error code Remove IOperation feature flag and internal registration APIs for operations Add unit test for VB Address review feedback and add more unit tests Fix unit tests Fix build errors from merge resolution Revert unintentional newline deletion Address recent feedback and do not generate an IParenthesizedOperation for C#. This change is now just a test-only change that verifies the current behavior. Address PR feedback and use singleton for null instance Address PR feedback Address PR feedback Do not invoke GetStandaloneNode helper in GetOperationWorker Fix InvalidCastException in CSharpOperationFactory for invalid nested member initializer (dotnet#22983) Use ICollection<string>.Contains to avoid Enumerator allocation Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. (dotnet#22933) Enable peverify-compat mode for langver < 7.2 (dotnet#22772) Properly detect root of a tree in Operation.SearchParentOperation (dotnet#22974) Fix IArgument and IArrayInitializer to have null types ...

* dotnet/master: (28 commits) Fix the operation verifier code now that getoperationinternal no longer exists. Remove usage of IOperation feature flag in newly added unit tests More fixes for IOperation tree. (dotnet#23028) Address PR feedback and remove unused resource and comment out unused error code Remove IOperation feature flag and internal registration APIs for operations Add unit test for VB Address review feedback and add more unit tests Fix unit tests Fix build errors from merge resolution Revert unintentional newline deletion Address recent feedback and do not generate an IParenthesizedOperation for C#. This change is now just a test-only change that verifies the current behavior. Address PR feedback and use singleton for null instance Address PR feedback Address PR feedback Do not invoke GetStandaloneNode helper in GetOperationWorker Fix InvalidCastException in CSharpOperationFactory for invalid nested member initializer (dotnet#22983) Use ICollection<string>.Contains to avoid Enumerator allocation Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. (dotnet#22933) Enable peverify-compat mode for langver < 7.2 (dotnet#22772) Fix IArgument and IArrayInitializer to have null types ...

dnfclas added the cla-already-signed label Oct 19, 2017

VSadov force-pushed the compatFixes branch from f617300 to e7ac95a Compare October 19, 2017 21:51

VSadov added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 19, 2017

VSadov mentioned this pull request Oct 19, 2017

Enable peverify-compat mode when SecurityTransparent or APTCA assembly attributes are present #22782

Closed

VSadov changed the title ~~Emit correct code when demands of new features interfere with peverify-compat.~~ Enable peverify-compat mode for langver < 7.2 Oct 19, 2017

VSadov added Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Oct 20, 2017

VSadov added this to the 15.5 milestone Oct 20, 2017

VSadov force-pushed the compatFixes branch 2 times, most recently from 2cc7cd1 to d089bf9 Compare October 20, 2017 01:17

VSadov added 2 commits October 19, 2017 18:46

Emit correct code when demands of new features interfere with peverif…

acfc47c

…y-compat.

Enable peverify compat mode in langver < 7.2

8cd464a

VSadov force-pushed the compatFixes branch from d089bf9 to 8cd464a Compare October 20, 2017 01:47

agocke reviewed Oct 25, 2017

View reviewed changes

cston reviewed Oct 25, 2017

View reviewed changes

cston approved these changes Oct 25, 2017

View reviewed changes

jaredpar approved these changes Oct 25, 2017

View reviewed changes

agocke approved these changes Oct 25, 2017

View reviewed changes

PR feedback

4059d28

natidea added the Approved to merge label Nov 3, 2017

jcouv merged commit ab63bff into dotnet:master Nov 3, 2017

VSadov deleted the compatFixes branch November 3, 2017 19:20

VSadov mentioned this pull request Nov 8, 2017

VS 15.5 Preview: VerificationException in otherwise valid function #22707

Closed

This was referenced Nov 9, 2017

Consider switching to peverify-compat automatically when APTCA or similar attributes are used #22712

Closed

ref readonly returns should not return references to temps when used with PEVerifyCompat flag #22721

Closed

jcouv mentioned this pull request Apr 17, 2018

C#7.2 produces not verifiable il code when direct accessing field of readonly struct field #26196

Closed

svick mentioned this pull request Apr 4, 2019

Consider expanding JIT Asm support ashmind/SharpLab#384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable peverify-compat mode for langver < 7.2 #22772

Enable peverify-compat mode for langver < 7.2 #22772

VSadov commented Oct 19, 2017 •

edited

Loading

MeiChin-Tsai commented Oct 25, 2017

agocke left a comment

agocke Oct 25, 2017 •

edited by VSadov

Loading

VSadov Oct 25, 2017

agocke Oct 25, 2017 •

edited by VSadov

Loading

VSadov Oct 25, 2017

agocke Oct 25, 2017 •

edited by VSadov

Loading

VSadov Oct 25, 2017

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

cston Oct 25, 2017 •

edited by VSadov

Loading

VSadov Oct 25, 2017

cston Oct 25, 2017 •

edited by VSadov

Loading

cston Oct 25, 2017 •

edited by VSadov

Loading

VSadov Oct 25, 2017

agocke left a comment

jcouv commented Oct 30, 2017

jcouv commented Nov 3, 2017 •

edited

Loading

jcouv commented Nov 3, 2017

natidea commented Nov 3, 2017

VSadov commented Nov 3, 2017

		@@ -139,7 +139,7 @@ private bool IsDebugPlus()

		private bool EnablePEVerifyCompat()


		";

		var comp = CreateCompilationWithMscorlib46(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef }, options: TestOptions.UnsafeReleaseExe);

Enable peverify-compat mode for langver < 7.2 #22772

Enable peverify-compat mode for langver < 7.2 #22772

Conversation

VSadov commented Oct 19, 2017 • edited Loading

MeiChin-Tsai commented Oct 25, 2017

agocke left a comment

Choose a reason for hiding this comment

agocke Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Oct 25, 2017

Choose a reason for hiding this comment

agocke Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Oct 25, 2017

Choose a reason for hiding this comment

agocke Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Oct 25, 2017

Choose a reason for hiding this comment

agocke Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

agocke Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

agocke Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

agocke Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

cston Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Oct 25, 2017

Choose a reason for hiding this comment

cston Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

cston Oct 25, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Oct 25, 2017

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

jcouv commented Oct 30, 2017

jcouv commented Nov 3, 2017 • edited Loading

jcouv commented Nov 3, 2017

natidea commented Nov 3, 2017

VSadov commented Nov 3, 2017

VSadov commented Oct 19, 2017 •

edited

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

agocke Oct 25, 2017 •

edited by VSadov

Loading

cston Oct 25, 2017 •

edited by VSadov

Loading

cston Oct 25, 2017 •

edited by VSadov

Loading

cston Oct 25, 2017 •

edited by VSadov

Loading

jcouv commented Nov 3, 2017 •

edited

Loading