Introduces a peverify-compat feature flag #21269

VSadov · 2017-08-02T22:03:29Z

In some scenarios we have a choice of emitting faster code that is typesafe, but not formally verifiable vs. less efficient but verifiable code.

The most common occurrence is around accessing readonly fields of structs. If the intent is only to read, then we could just get a field reference and read to it.
However verification rules do not make distinction when reference is used only to read and require that intermediate copy of the field value be made.

The flag allows suppressing peverify violations, where possible, typically at the cost of introducing intermediate copying.

- Made readonly fields homeless outside the constructor when flag is set.

tannergooding · 2017-08-02T22:18:01Z

@VSadov, so the default will be faster unverifiable code, but there will be an option to enable the slower verifiable code, correct?

Is this a case where we could get peverify updated to have knowledge about 'readonly' structs/fields?

VSadov · 2017-08-02T23:33:32Z

@tannergooding - peverify as a tool is a bit of deadend at the moment.
It has too many assumptions about desktop .NET and dependencies/coupling with JIT internal implementation of the rules. Not all JITs do it the same way, so behavior may vary and difficult to change without changing JIT. Some JITs do not want to be in verification business at all...

The most likely vehicle for smarter rules is this, once it is more mature: https://github.com/dotnet/corert/tree/master/src/ILVerify

VSadov · 2017-08-02T23:38:16Z

test windows_release_vs-integration_prtest please

These still fail once in a while in our branch. Likely fixed in master.

VSadov · 2017-08-04T15:35:58Z

@dotnet/roslyn-compiler - please review

jaredpar · 2017-08-04T16:34:03Z

@tannergooding

so the default will be faster unverifiable code, but there will be an option to enable the slower verifiable code, correct?

As @VSadov noted, the problem is that peverify is not evolving at the moment. The code we generate is still type safe and can be verified as such according to verification rules we've sketched out. Need a new tool to catch up with our code :)

tannergooding · 2017-08-04T16:37:34Z

@jaredpar, so this is essentially a safety fallback for users who still rely on peverify (or other outdated verification tools)?

jcouv · 2017-08-04T16:39:57Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -163,6 +163,8 @@ internal override CommonAnonymousTypeManager CommonAnonymousTypeManager
        /// </summary>
        internal bool FeatureStrictEnabled => Feature("strict") != null;

+        internal bool FeaturePEVerifyCompatEnabled => Feature("peverify-compat") != null;


Please add xml doc as above. #Resolved

jcouv · 2017-08-04T16:41:12Z

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

+            CompileAndVerify(comp, verify: false, expectedOutput: @"
+3
+42
+2


The output is different from above. That seems surprising. #WontFix

❓ I understand printing 2 here, but why 3 when peverify-compat is off? #WontFix

Oh, I get it now. Passes a references to f instead of evaluating it. #WontFix

I'm also concerned about this case. Is there a way to describe the cases which have observable changes and exclude them from peverify-compat? #WontFix

The output is different because the flag currently deoptimizes overal treatment of readonly fields as lvalues in readonly context. Thus it affects scenarios where removing aliasing is observable. Generally those are scenarios involving readonly ref

This makes me think that perhaps we should not deoptimize those.
It could be hard, however, to distinguish situations where new features are involved since that could be very indirect.
#WontFix

Basically, in the simplest case, - if you pass a readonly field to an in parameter do you want:
1 - never pass it by reference (defeats the purpose of the in, definitely no )
2 - always pass it by reference (triggers peverify complains)
3 - change the behavior based on the compat flag (some contorted code may see the difference)

We have #3 right now.
#WontFix

jaredpar · 2017-08-04T17:08:12Z

@tannergooding

so this is essentially a safety fallback for users who still rely on peverify (or other outdated verification tools)?

I wouldn't use the word safety here because of the nature of the problem. Nothing is unsafe here, peverify is just not keeping up with the language.

I prefer thinking of it as a release valve for customers who are impacted by this change. Our expectation is that this customer doesn't exist but given this is going into a point release of the language want to have a backup plan.

#WontFix

jaredpar · 2017-08-04T17:16:26Z

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

@@ -137,6 +137,11 @@ private bool IsDebugPlus()
            return _module.Compilation.Options.DebugPlusMode;
        }

+        private bool IsPEVerifyCompatible()


Name sits wrong with me. Think it would read better as matching the option name. Say: EnablePEVerifyCompat #Resolved

I do not have much preferences here.

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

jaredpar · 2017-08-04T17:17:23Z

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

-  IL_003c:  ldflda     ""MyManagedStruct.Nested.Nested1 MyManagedStruct.Nested.n""
+  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
 }
 ");
        }



Let's add a test trait for PEVerifyCompat mode and mark the new tests as such. That will make it easy to remove the feature later on if we make that decision. #Resolved

compiling with WithPEVerifyCompatFeature is a fairly robust indication, but can add a trait too.

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

jcouv · 2017-08-04T18:07:55Z

From discussion with Vlad, it is surprising that an "optimization" or "de-optimization" would have observable effects. The new language semantics need to be documented.

~~From our discussion, it sounds like the flag should have no observable effect on old code. So I'd recommend one of two approaches:~~

~~the feature flag should only remove the optimization on "old" code, where it is not observable. "New" code is not PE-verifiable anyways.~~
~~the feature flag should produce a warning if used in compilation with "new" code (7.2, ref readonly, or anything that makes the optimization observable)~~

From further discussion, this observable change is an acceptable break (very corner case). #Resolved

VSadov · 2017-08-04T19:49:18Z

Discussed further. - The goal of the flag is a last resort way to suppress peverify errors. It would not be generally used or recommended as a long term solution.
We are ok with suboptimal codegen and deviation from spec in unusual corner cases.

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

VSadov · 2017-08-04T22:53:56Z

any more suggestion on this one?

VSadov · 2017-08-07T18:43:44Z

@dotnet/roslyn-compiler - PING

sharwell · 2017-08-07T18:50:46Z

@VSadov When this flag is documented, consider adding a recommendation similar to the following:

⚠️ In certain edge cases, this flag will produce code which deviates from the C# language specification due to limitations in the PEVerify implementation. For application consistency, users are encouraged to use the output from this feature only for passing to PEVerify, and using a second build where this feature is not enabled at execution time.

jcouv

LGTM

VSadov added 3 commits August 1, 2017 15:01

- Introduced the flag.

ac2adb1

- Made readonly fields homeless outside the constructor when flag is set.

one more case for ?.

6b3776b

Another case for nested structs

bc819fe

dnfclas added the cla-already-signed label Aug 2, 2017

VSadov added the Area-Compilers label Aug 2, 2017

remove commented out code used for testing.

11fc778

jcouv reviewed Aug 4, 2017

View reviewed changes

jaredpar reviewed Aug 4, 2017

View reviewed changes

VSadov added 2 commits August 4, 2017 12:49

PR feedback

b05c21a

missing using

243afaf

jaredpar approved these changes Aug 7, 2017

View reviewed changes

jcouv approved these changes Aug 7, 2017

View reviewed changes

VSadov merged commit d3ea5d2 into dotnet:features/readonly-ref Aug 8, 2017

jcouv mentioned this pull request Oct 16, 2017

VS 15.5 Preview: VerificationException in otherwise valid function #22707

Closed

jcouv added the Documentation label Oct 16, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces a peverify-compat feature flag #21269

Introduces a peverify-compat feature flag #21269

VSadov commented Aug 2, 2017

tannergooding commented Aug 2, 2017

VSadov commented Aug 2, 2017

VSadov commented Aug 2, 2017 •

edited

Loading

VSadov commented Aug 4, 2017

jaredpar commented Aug 4, 2017

tannergooding commented Aug 4, 2017

jcouv Aug 4, 2017 •

edited by VSadov

Loading

jcouv Aug 4, 2017 •

edited by VSadov

Loading

sharwell Aug 4, 2017 •

edited by VSadov

Loading

sharwell Aug 4, 2017 •

edited by VSadov

Loading

sharwell Aug 4, 2017 •

edited by VSadov

Loading

VSadov Aug 4, 2017 •

edited

Loading

VSadov Aug 4, 2017 •

edited

Loading

jaredpar commented Aug 4, 2017 •

edited by VSadov

Loading

jaredpar Aug 4, 2017 •

edited by VSadov

Loading

VSadov Aug 4, 2017

jaredpar Aug 4, 2017 •

edited by VSadov

Loading

VSadov Aug 4, 2017

jcouv commented Aug 4, 2017 •

edited by VSadov

Loading

VSadov commented Aug 4, 2017

VSadov commented Aug 4, 2017

VSadov commented Aug 7, 2017

sharwell commented Aug 7, 2017

jcouv left a comment

Introduces a peverify-compat feature flag #21269

Introduces a peverify-compat feature flag #21269

Conversation

VSadov commented Aug 2, 2017

tannergooding commented Aug 2, 2017

VSadov commented Aug 2, 2017

VSadov commented Aug 2, 2017 • edited Loading

VSadov commented Aug 4, 2017

jaredpar commented Aug 4, 2017

tannergooding commented Aug 4, 2017

jcouv Aug 4, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

jcouv Aug 4, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

sharwell Aug 4, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

sharwell Aug 4, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

sharwell Aug 4, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

VSadov Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

jaredpar commented Aug 4, 2017 • edited by VSadov Loading

jaredpar Aug 4, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Aug 4, 2017

Choose a reason for hiding this comment

jaredpar Aug 4, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Aug 4, 2017

Choose a reason for hiding this comment

jcouv commented Aug 4, 2017 • edited by VSadov Loading

VSadov commented Aug 4, 2017

VSadov commented Aug 4, 2017

VSadov commented Aug 7, 2017

sharwell commented Aug 7, 2017

jcouv left a comment

Choose a reason for hiding this comment

VSadov commented Aug 2, 2017 •

edited

Loading

jcouv Aug 4, 2017 •

edited by VSadov

Loading

jcouv Aug 4, 2017 •

edited by VSadov

Loading

sharwell Aug 4, 2017 •

edited by VSadov

Loading

sharwell Aug 4, 2017 •

edited by VSadov

Loading

sharwell Aug 4, 2017 •

edited by VSadov

Loading

VSadov Aug 4, 2017 •

edited

Loading

VSadov Aug 4, 2017 •

edited

Loading

jaredpar commented Aug 4, 2017 •

edited by VSadov

Loading

jaredpar Aug 4, 2017 •

edited by VSadov

Loading

jaredpar Aug 4, 2017 •

edited by VSadov

Loading

jcouv commented Aug 4, 2017 •

edited by VSadov

Loading