[ILVerify] Implement verification of return instruction (#4650)

* Implement verification of return instruction. Added PermanentHome flag to StackValue.

* Fixed test cases not returning correctly.

* Reverted load static field changes.

* Added trackObjCtor flag and added relevant references to PEVerify code blocks.

* Added test cases for return instruction.

8 files changed, 285 insertions, 76 deletions

diff --git a/src/ILVerify/src/ILImporter.StackValue.cs b/src/ILVerify/src/ILImporter.StackValue.cs
index 62693b2e2..fe1148832 100644
--- a/src/ILVerify/src/ILImporter.StackValue.cs
+++ b/src/ILVerify/src/ILImporter.StackValue.cs
@@ -16,7 +16,8 @@ namespace Internal.IL
         public enum StackValueFlags
         {
             None = 0,
-            ReadOnly = 1 << 1
+            ReadOnly = 1 << 1,
+            PermanentHome = 1 << 2
         }
         private StackValueFlags Flags;
 
@@ -37,11 +38,21 @@ namespace Internal.IL
             Flags |= StackValueFlags.ReadOnly;
         }
 
+        public void SetIsPermanentHome()
+        {
+            Flags |= StackValueFlags.PermanentHome;
+        }
+
         public bool IsReadOnly
         {
             get { return (Flags & StackValueFlags.ReadOnly) == StackValueFlags.ReadOnly; }
         }
 
+        public bool IsPermanentHome
+        {
+            get { return (Flags & StackValueFlags.PermanentHome) == StackValueFlags.PermanentHome; }
+        }
+
         public bool IsNullReference
         {
             get { return Kind == StackValueKind.ObjRef && Type == null; }
@@ -83,9 +94,11 @@ namespace Internal.IL
             return new StackValue(StackValueKind.ValueType, type);
         }
 
-        static public StackValue CreateByRef(TypeDesc type, bool readOnly = false)
+        static public StackValue CreateByRef(TypeDesc type, bool readOnly = false, bool permanentHome = false)
         {
-            return new StackValue(StackValueKind.ByRef, type, null, readOnly ? StackValueFlags.ReadOnly : StackValueFlags.None);
+            return new StackValue(StackValueKind.ByRef, type, null,
+                (readOnly ? StackValueFlags.ReadOnly : StackValueFlags.None) | 
+                (permanentHome ? StackValueFlags.PermanentHome : StackValueFlags.None));
         }
 
         static public StackValue CreateMethod(MethodDesc method)
diff --git a/src/ILVerify/src/ILImporter.Verify.cs b/src/ILVerify/src/ILImporter.Verify.cs
index fd8d8e3e1..f720f6b4d 100644
--- a/src/ILVerify/src/ILImporter.Verify.cs
+++ b/src/ILVerify/src/ILImporter.Verify.cs
@@ -57,6 +57,8 @@ namespace Internal.IL
         StackValue[] _stack = s_emptyStack;
         int _stackTop = 0;
 
+        bool _trackObjCtorState;
+
         class ExceptionRegion
         {
             public ILExceptionRegion ILRegion;
@@ -165,6 +167,8 @@ namespace Internal.IL
 
             _maxStack = _methodIL.MaxStack;
 
+            _trackObjCtorState = !_methodSignature.IsStatic && _method.IsConstructor && !method.OwningType.IsValueType;
+
             _ilBytes = _methodIL.GetILBytes();
             _locals = _methodIL.GetLocals();
 
@@ -687,6 +691,7 @@ namespace Internal.IL
         void EndImportingInstruction()
         {
             CheckPendingPrefix(_pendingPrefix);
+            ClearPendingPrefix(_pendingPrefix); // Make sure prefix is cleared
         }
 
         void StartImportingBasicBlock(BasicBlock basicBlock)
@@ -797,6 +802,14 @@ namespace Internal.IL
             if (!argument)
                 Check(_initLocals, VerifierError.InitLocals);
 
+#if false
+            if (argument)
+            {
+                if (m_verTrackObjCtorInitState && !vstate->isThisInitialized() && x.IsThisPtr())
+                    x.SetUninitialisedObjRef();
+            }
+#endif
+
             Push(StackValue.CreateFromType(varType));
         }
 
@@ -806,6 +819,14 @@ namespace Internal.IL
 
             var value = Pop();
 
+#if false
+            if (argument)
+            {
+                if (m_verTrackObjCtorInitState && !vstate->isThisInitialized() )
+                    Verify(!m_paramVerifyMap[num].IsThisPtr(), MVER_E_THIS_UNINIT_STORE); //"storing to uninit this ptr"
+            }
+#endif
+
             CheckIsAssignable(value, StackValue.CreateFromType(varType));
         }
 
@@ -816,6 +837,14 @@ namespace Internal.IL
             if (!argument)
                 Check(_initLocals, VerifierError.InitLocals);
 
+#if false
+            if (argument)
+            {
+                if (m_verTrackObjCtorInitState && !vstate->isThisInitialized() )
+                    Verify(!tiRetVal.IsThisPtr(), MVER_E_THIS_UNINIT_STORE);
+            }
+#endif
+
             Push(StackValue.CreateByRef(varType));
         }
 
@@ -1106,13 +1135,11 @@ namespace Internal.IL
                     vstate->readonlyPrefix = false;
                     tiRetVal.SetIsReadonlyByRef();
                 }
-
-                if (tiRetVal.IsByRef())
-                {                
-                    tiRetVal.SetIsPermanentHomeByRef();
-                }
 #endif
 
+                if (returnValue.Kind == StackValueKind.ByRef)
+                    returnValue.SetIsPermanentHome();
+
                 Push(returnValue);
             }
         }
@@ -1196,68 +1223,40 @@ namespace Internal.IL
 
         void ImportReturn()
         {
-
-
 #if false
-    // 'this' must be init before return
-    if (m_verTrackObjCtorInitState)
-        Verify(vstate->isThisPublishable(), MVER_E_THIS_UNINIT_RET);
-
-    // review : already in verifyreturnflow
-    if (region)
-    {
-        switch (RgnGetRegionType(region))
-        {
-            case ReaderBaseNS::RGN_FILTER:
-                BADCODE(MVER_E_RET_FROM_FIL);
-                break;
-            case ReaderBaseNS::RGN_TRY:
-                BADCODE(MVER_E_RET_FROM_TRY);
-                break;
-            case ReaderBaseNS::RGN_FAULT:
-            case ReaderBaseNS::RGN_FINALLY:
-            case ReaderBaseNS::RGN_MEXCEPT:
-            case ReaderBaseNS::RGN_MCATCH:
-                BADCODE(MVER_E_RET_FROM_HND);
-                break;
-            case ReaderBaseNS::RGN_ROOT:
-                break;
-            default:
-                VASSERT(UNREACHED);
-                break;
-        }
-    }
+            // 'this' must be init before return
+            if (_trackObjCtorState)
+                Verify(vstate->isThisPublishable(), MVER_E_THIS_UNINIT_RET);
+#endif
 
-    m_jitInfo->getMethodSig(getCurrentMethodHandle(), &sig);
+            // Check current region type
+            Check(_currentBasicBlock.FilterIndex == null, VerifierError.ReturnFromFilter);
+            Check(_currentBasicBlock.TryIndex == null, VerifierError.ReturnFromTry);
+            Check(_currentBasicBlock.HandlerIndex == null, VerifierError.ReturnFromHandler);
 
-    // Now get the return type and convert it to our format.
-    corType = sig.retType;
+            var declaredReturnType = _method.Signature.ReturnType;
 
-    expectedStack = 0;
-    if (corType != CORINFO_TYPE_VOID)
-    {
-        Verify(vstate->stackLevel() > 0, MVER_E_RET_MISSING);
-        Verify(vstate->stackLevel() == 1, MVER_E_RET_EMPTY);
+            if (declaredReturnType.IsVoid)
+            {
+                Debug.Assert(_stackTop >= 0);
 
-        vertype tiVal = vstate->impStackTop(0);
-        vertype tiDeclared = verMakeTypeInfo(corType, sig.retTypeClass);
+                if (_stackTop > 0)
+                    VerificationError(VerifierError.ReturnVoid, _stack[_stackTop - 1]);
+            }
+            else
+            {
+                if (_stackTop <= 0)
+                    VerificationError(VerifierError.ReturnMissing);
+                else
+                {
+                    Check(_stackTop == 1, VerifierError.ReturnEmpty);
 
-        VerifyCompatibleWith(tiVal, tiDeclared.NormaliseForStack());
-        Verify((!verIsByRefLike(tiDeclared)) || verIsSafeToReturnByRef(tiVal), MVER_E_RET_PTR_TO_STACK);
-        expectedStack=1;
-    }
-    else
-    {
-        Verify(vstate->stackLevel() == 0, MVER_E_RET_VOID);
-    }
+                    var actualReturnType = Pop();
+                    CheckIsAssignable(actualReturnType, StackValue.CreateFromType(declaredReturnType));
 
-    if (expectedStack == 1)
-        vstate->pop();
-    else
-        VASSERT(!expectedStack);
-#endif
-
-            // TODO
+                    Check((!declaredReturnType.IsByRef && !declaredReturnType.IsByRefLike) || actualReturnType.IsPermanentHome, VerifierError.ReturnPtrToStack);
+                }
+            }
         }
 
         void ImportFallthrough(BasicBlock next)
@@ -1478,10 +1477,13 @@ namespace Internal.IL
         void ImportAddressOfField(int token, bool isStatic)
         {
             var field = ResolveFieldToken(token);
+            bool isPermanentHome = false;
 
             if (isStatic)
             {
                 Check(field.IsStatic, VerifierError.ExpectedStaticField);
+
+                isPermanentHome = true;
             }
             else
             {
@@ -1498,9 +1500,11 @@ namespace Internal.IL
                     StackValue.CreateByRef(owningType) : StackValue.CreateObjRef(owningType);
 
                 CheckIsAssignable(actualThis, declaredThis);
+
+                isPermanentHome = actualThis.Kind == StackValueKind.ObjRef || actualThis.IsPermanentHome;
             }
 
-            Push(StackValue.CreateByRef(field.FieldType));
+            Push(StackValue.CreateByRef(field.FieldType, false, isPermanentHome));
         }
 
         void ImportStoreField(int token, bool isStatic)
@@ -1593,6 +1597,18 @@ namespace Internal.IL
             var value = Pop();
 
             CheckIsObjRef(value);
+
+#if false
+            if (m_verTrackObjCtorInitState && !vstate->isThisInitialized())
+                Verify(!tiRetVal.IsThisPtr(), MVER_E_STACK_UNINIT);
+
+            while (vstate->stackLevel() > 0)
+            {
+                // vstate->pop();
+                // throw is not a return so we don't need to be initialized
+                vstate->popPossiblyUninit();
+            }
+#endif
         }
 
         void ImportLoadString(int token)
@@ -1762,7 +1778,8 @@ namespace Internal.IL
                 CheckIsPointerElementCompatibleWith(actualElementType, elementType);
             }
 
-            Push(StackValue.CreateByRef(elementType, HasPendingPrefix(Prefix.ReadOnly)));
+            // an array interior pointer is always on the heap, hence permanentHome = true
+            Push(StackValue.CreateByRef(elementType, HasPendingPrefix(Prefix.ReadOnly), true));
             ClearPendingPrefix(Prefix.ReadOnly);
         }
 
@@ -1814,7 +1831,7 @@ namespace Internal.IL
         {
             var type = ResolveTypeToken(token);
 
-            var value = Pop();
+            CheckIsObjRef(Pop());
 
             if (opCode == ILOpcode.unbox_any)
             {
@@ -1824,7 +1841,8 @@ namespace Internal.IL
             {
                 Check(type.IsValueType, VerifierError.ValueTypeExpected);
 
-                Push(StackValue.CreateByRef(type ,true));
+                // We always come from an ObjRef, hence this is permanentHome
+                Push(StackValue.CreateByRef(type, true, true));
             }
         }
 
diff --git a/src/ILVerify/src/Resources/Strings.resx b/src/ILVerify/src/Resources/Strings.resx
index 640f58fce..f3f5ce7dc 100644
--- a/src/ILVerify/src/Resources/Strings.resx
+++ b/src/ILVerify/src/Resources/Strings.resx
@@ -222,6 +222,27 @@
   <data name="Rethrow" xml:space="preserve">
     <value>Rethrow from outside a catch handler.</value>
   </data>
+  <data name="ReturnEmpty" xml:space="preserve">
+    <value>Stack must contain only the return value.</value>
+  </data>
+  <data name="ReturnFromFilter" xml:space="preserve">
+    <value>Return out of exception filter block.</value>
+  </data>
+  <data name="ReturnFromHandler" xml:space="preserve">
+    <value>Return out of exception handler block.</value>
+  </data>
+  <data name="ReturnFromTry" xml:space="preserve">
+    <value>Return out of try block.</value>
+  </data>
+  <data name="ReturnMissing" xml:space="preserve">
+    <value>Return value missing on the stack.</value>
+  </data>
+  <data name="ReturnPtrToStack" xml:space="preserve">
+    <value>Return type is ByRef, TypedReference, ArgHandle, or ArgIterator.</value>
+  </data>
+  <data name="ReturnVoid" xml:space="preserve">
+    <value>Stack must be empty on return from a void function.</value>
+  </data>
   <data name="StackByRef" xml:space="preserve">
     <value>Expected ByRef on the stack.</value>
   </data>
diff --git a/src/ILVerify/src/VerifierError.cs b/src/ILVerify/src/VerifierError.cs
index ca26c5af4..d76ae4aa8 100644
--- a/src/ILVerify/src/VerifierError.cs
+++ b/src/ILVerify/src/VerifierError.cs
@@ -70,9 +70,9 @@ namespace ILVerify
         BranchOutOfHandler,     //"Branch out of exception handler block."
         BranchOutOfFilter,      //"Branch out of exception filter block."
         //E_BR_OUTOF_FIN       "Branch out of finally block."
-        //E_RET_FROM_TRY       "Return out of try block."
-        //E_RET_FROM_HND       "Return out of exception handler block."
-        //E_RET_FROM_FIL       "Return out of exception filter block."
+        ReturnFromTry,          //"Return out of try block."
+        ReturnFromHandler,      //"Return out of exception handler block."
+        ReturnFromFilter,       //"Return out of exception filter block."
         //E_BAD_JMP_TARGET     "jmp / exception into the middle of an instruction."
         //E_PATH_LOC           "Non-compatible types depending on path."
         //E_PATH_THIS          "Init state for this differs depending on path."
@@ -118,10 +118,10 @@ namespace ILVerify
         //  E_TOKEN_TYPE_SIG     "Expected signature token."
         Unverifiable,                   // Instruction can not be verified.
         StringOperand,                  // Operand does not point to a valid string ref.
-        //E_RET_PTR_TO_STACK   "Return type is ByRef, TypedReference, ArgHandle, or ArgIterator."
-        //E_RET_VOID           "Stack must be empty on return from a void function."
-        //E_RET_MISSING        "Return value missing on the stack."
-        //E_RET_EMPTY          "Stack must contain only the return value."
+        ReturnPtrToStack,               // Return type is ByRef, TypedReference, ArgHandle, or ArgIterator.
+        ReturnVoid,                     // Stack must be empty on return from a void function.
+        ReturnMissing,                  // Return value missing on the stack.
+        ReturnEmpty,                    // Stack must contain only the return value.
         //E_RET_UNINIT         "Return uninitialized data."
         //E_ARRAY_ACCESS       "Illegal array access."
         //E_ARRAY_V_STORE      "Store non Object type into Object array."
diff --git a/src/ILVerify/tests/ILTests/CastingTests.il b/src/ILVerify/tests/ILTests/CastingTests.il
index 769022b73..bddd3bcd3 100644
--- a/src/ILVerify/tests/ILTests/CastingTests.il
+++ b/src/ILVerify/tests/ILTests/CastingTests.il
@@ -193,10 +193,10 @@
         .locals init (
             class [System.Runtime]System.Func`1<!T> V_0
         )
-        
-        ldarg.0
+
         ldloc.0
         callvirt  instance !0 class [System.Runtime]System.Func`1<!T>::Invoke()
+        pop
         ret
     }
 
diff --git a/src/ILVerify/tests/ILTests/LoadStoreIndirectTest.il b/src/ILVerify/tests/ILTests/LoadStoreIndirectTest.il
index eb7343a54..95cc79c3f 100644
--- a/src/ILVerify/tests/ILTests/LoadStoreIndirectTest.il
+++ b/src/ILVerify/tests/ILTests/LoadStoreIndirectTest.il
@@ -17,6 +17,7 @@
     {
         ldarg.0
         ldind.ref
+        pop
         ret
     }
 
@@ -24,6 +25,7 @@
     {
         ldarg.0
         ldind.i4
+        pop
         ret
     }
 
@@ -31,6 +33,7 @@
     {
         ldarg.0
         ldind.i4
+        pop
         ret
     }
 
@@ -38,6 +41,7 @@
     {
         ldarg.0
         ldind.ref
+        pop
         ret
     }
 
diff --git a/src/ILVerify/tests/ILTests/PrefixTests.il b/src/ILVerify/tests/ILTests/PrefixTests.il
index 801d4df78..5ba5e3546 100644
--- a/src/ILVerify/tests/ILTests/PrefixTests.il
+++ b/src/ILVerify/tests/ILTests/PrefixTests.il
@@ -27,6 +27,7 @@
         readonly. 
         ldelema    [System.Runtime]System.Int32
         call       instance string [System.Runtime]System.Int32::ToString()
+        pop
         ret
     }
 
@@ -43,6 +44,7 @@
         
         readonly. 
         ldelem    [System.Runtime]System.Int32
+        pop
         ret
     }
 
diff --git a/src/ILVerify/tests/ILTests/ReturnTests.il b/src/ILVerify/tests/ILTests/ReturnTests.il
new file mode 100644
index 000000000..600f5b877
--- /dev/null
+++ b/src/ILVerify/tests/ILTests/ReturnTests.il
@@ -0,0 +1,151 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+.assembly extern System.Runtime
+{
+}
+
+.assembly ReturnTests
+{
+}
+
+.class public sequential ansi sealed beforefieldinit Struct
+        extends [System.Runtime]System.ValueType
+{
+    .field public int32 instanceField
+}
+
+.class public auto ansi beforefieldinit ReturnTestsType
+        extends [System.Runtime]System.Object
+{
+    .field private int32 instanceField
+    .field private static int32 staticField
+
+    .field private valuetype Struct instanceStruct
+
+    .method public hidebysig instance void Return.Void_Valid() cil managed
+    {
+        ldarg.0
+        pop
+        ret
+    }
+
+    .method public hidebysig instance void Return.VoidNonEmptyStack_Invalid_ReturnVoid() cil managed
+    {
+        ldarg.0
+        ret
+    }
+
+    .method public hidebysig instance int32 Return.Int32_Valid() cil managed
+    {
+        ldc.i4.0
+        ret
+    }
+
+    .method public hidebysig instance int32 Return.Int32EmptyStack_Invalid_ReturnMissing() cil managed
+    {
+        ret
+    }
+
+    .method public hidebysig instance int32 Return.Int32StackLeft_Invalid_ReturnEmpty() cil managed
+    {
+        ldc.i4.0
+        ldc.i4.0
+        ret
+    }
+
+    .method public hidebysig instance void Return.FromTry_Invalid_ReturnFromTry() cil managed
+    {
+        .try
+        {
+            ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s     lbl_ret
+        }
+
+    lbl_ret:
+        ret
+    }
+
+    .method public hidebysig instance void Return.FromCatch_Invalid_ReturnFromHandler() cil managed
+    {
+        .try
+        {
+            leave.s     lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            ret
+        }
+
+    lbl_ret:
+        ret
+    }
+
+    .method public hidebysig instance void Return.FromFilter_Invalid_ReturnFromFilter() cil managed
+    {
+        .try
+        {
+            leave.s     lbl_ret
+        }
+        filter
+        {
+            pop
+            ret
+
+            endfilter
+        }
+        {
+            pop
+            leave.s     lbl_ret
+        }
+
+    lbl_ret:
+        ret
+    }
+
+    .method public hidebysig instance int32 Return.ObjectForInt32_Invalid_StackUnexpected(object o) cil managed
+    {
+        ldarg.1
+        ret
+    }
+
+    .method public hidebysig instance int32& Return.LocalArgByRef_Invalid_ReturnPtrToStack(int32 i) cil managed
+    {
+        ldarga.s    i
+        ret
+    }
+
+    .method public hidebysig instance int32& Return.InstanceFieldByRef_Valid(int32 i) cil managed
+    {
+        ldarg.0
+        ldflda      int32 ReturnTestsType::instanceField
+        ret
+    }
+
+    .method public hidebysig instance int32& Return.StaticFieldByRef_Valid(int32 i) cil managed
+    {
+        ldsflda      int32 ReturnTestsType::staticField
+        ret
+    }
+
+    .method public hidebysig instance int32& Return.LocalStructFieldByRef_Invalid_ReturnPtrToStack(valuetype Struct s) cil managed
+    {
+        ldarg.1
+        ldflda      int32 Struct::instanceField
+        ret
+    }
+
+    .method public hidebysig instance int32& Return.FieldStructFieldByRef_Valid() cil managed
+    {
+        ldarg.0
+        ldflda      valuetype Struct ReturnTestsType::instanceStruct
+        ldflda      int32 Struct::instanceField
+        ret
+    }
+}