Fix to assert 'ret->gtGetOp1() == nullptr' failure in morph.cpp

Investigation reveals that the assumption of GT_RETURN stmnt for methods returning struct through hidden RetBuf argument will be of the form GT_RETURN(TYP_VOID, op1=nullptr, op2=nullptr) is incorrect when running under profiler.

Profiler Leave callback requires that methods with struct return, return RetBuf in RAX. For this reason when required to generate Profiler callbacks, importer generates GT_RETURN(BY_REF, addr of retBuf) - see impReturnInstruction()

There is also another issue that fgAddInternal() which adds oneReturn block doesn't take into account of running under profiler and hence doesn't create genReturnLocal var and as a result oneReturn block doesn't return address of retBufArg. When running under profiler what should happen for a method returning struct via hidden RetBuf is as follows:

Say BB01 is any block ending in GT_RETURN(TYP_BYREF, addr of RetBuf). fgAddInternal() will decide to use oneReturn block since running under profiler. Since the method returns addr of RetBuf it should create a genReturnLocal of type TYP_BYREF. fgMorphBlocks() on finding genReturnLocal is valid, will replace GT_RETURN with as assignment of "genReturnLocal = add of RetBuf" and make BB01 to branch to oneReturn block.

Since fgAddInternal() doesn't take into account of running under profiler, it doesn't create genReturnLocal for methods having hidden RetBuf arg. Hence fgMorphBlocks() will find genReturnLocal is not valid and asserts that GT_RETURN must be of TYP_VOID with its op1=op2=nullptr, which is not the case and hence the assert.

Note that this assert will repro only when running under profiler because importer generates GT_RETURN(TYP_BYREF, addr of RetBuf) for methods with hidden RetBuf arg only when required to generate profiler callbacks.

Fix: Take into account of whether asked to generate Profiler callbacks create genReturnLocal.
Also took this opportunity cleaned up unnecessary code that was executing for unix and added new abstractions to check whether the method being compiled has ret val.

Note in case of unix there is no need to call struct classification logic to normalize struct return type returned in a single register because it is already done in lvaInitTypeRef() and assigned to compRetNativeType.

diff --git a/src/jit/compiler.h b/src/jit/compiler.h
index 9e4640f..2d17cb3 100644 (file)
--- a/src/jit/compiler.h
+++ b/src/jit/compiler.h
@@ -8068,6 +8068,56 @@ public :
     }
     info;
 
+    // Returns true if the method being compiled returns a non-void and non-struct value.
+    // Note that lvaInitTypeRef() normalizes compRetNativeType for struct returns in a 
+    // single register as per target arch ABI (e.g on Amd64 Windows structs of size 1, 2,
+    // 4 or 8 gets normalized to TYP_BYTE/TYP_SHORT/TYP_INT/TYP_LONG; On Arm Hfa structs).
+    // Methods returning such structs are considered to return non-struct return value and
+    // this method returns true in that case.
+    bool                compMethodReturnsNativeScalarType() 
+    {
+        return (info.compRetType != TYP_VOID) && !varTypeIsStruct(info.compRetNativeType);
+    }
+
+    // Returns true if the method being compiled returns RetBuf addr as its return value
+    bool                compMethodReturnsRetBufAddr() 
+    {
+        // Profiler Leave calllback expects the address of retbuf as return value for 
+        // methods with hidden RetBuf argument.  impReturnInstruction() when profiler
+        // callbacks are needed creates GT_RETURN(TYP_BYREF, op1 = Addr of RetBuf) for
+        // methods with hidden RetBufArg.
+        //
+        // TODO-AMD64-Unix - As per this ABI, addr of RetBuf needs to be returned by
+        // methods with hidden RetBufArg.  Right now we are special casing GT_RETURN
+        // of TYP_VOID in codegenxarch.cpp to generate "mov rax, addr of RetBuf".
+        // Instead we should consider explicitly materializing GT_RETURN of TYP_BYREF
+        // return addr of RetBuf in IR.
+        return compIsProfilerHookNeeded() && (info.compRetBuffArg != BAD_VAR_NUM);
+    }
+
+    // Returns true if the method returns a value in more than one return register
+    // TODO-ARM-Bug: Deal with multi-register genReturnLocaled structs?
+    // TODO-ARM64: Does this apply for ARM64 too?
+    bool                compMethodReturnsMultiRegRetType() 
+    {       
+#if FEATURE_MULTIREG_RET
+#ifdef FEATURE_UNIX_AMD64_STRUCT_PASSING
+        // Methods returning a struct in two registers is considered having a return value of TYP_STRUCT.
+        // Such method's compRetNativeType is TYP_STRUCT without a hidden RetBufArg
+        return varTypeIsStruct(info.compRetNativeType) && (info.compRetBuffArg == BAD_VAR_NUM);
+#endif 
+#endif
+        return false;
+    }
+
+    // Returns true if the method being compiled returns a value
+    bool                compMethodHasRetVal()
+    {
+        return compMethodReturnsNativeScalarType() ||
+               compMethodReturnsRetBufAddr() ||
+               compMethodReturnsMultiRegRetType();
+    }
+
 #if defined(DEBUG)
 
     void                compDispLocalVars();

diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index a731443..b321aa1 100644 (file)
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -8159,71 +8159,40 @@ void                Compiler::fgAddInternal()
         //
         // We don't have a oneReturn block for this method
         //
-        genReturnBB = NULL;
+        genReturnBB = nullptr;
     }
 
     // If there is a return value, then create a temp for it.  Real returns will store the value in there and
-    // it'll be reloaded by the single return.
-    // TODO-ARM-Bug: Deal with multi-register genReturnLocaled structs?
-    // TODO-ARM64: Does this apply for ARM64 too?
-#if defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
-    // Create a local temp to store the return if the return type is not void and the
-    // native return type is not a struct or the native return type is a struct that is returned
-    // in registers (no RetBuffArg argument.)
-    // If we fold all returns into a single return statement, create a temp for struct type variables as well.
-    if (genReturnBB && ((info.compRetType != TYP_VOID && !varTypeIsStruct(info.compRetNativeType)) ||
-        (varTypeIsStruct(info.compRetNativeType) && info.compRetBuffArg == BAD_VAR_NUM)))
-#else // !defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
-    if (genReturnBB && (info.compRetType != TYP_VOID) && !varTypeIsStruct(info.compRetNativeType))
-#endif // !defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
+    // it'll be reloaded by the single return.                                    
+    if (genReturnBB && compMethodHasRetVal())
     {
         genReturnLocal = lvaGrabTemp(true DEBUGARG("Single return block return value"));
-#if defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
-        var_types retLocalType = TYP_STRUCT;
-        if (info.compRetNativeType == TYP_STRUCT)
+
+        if (compMethodReturnsNativeScalarType())
         {
-            // If the native ret type is a struct, make sure the right 
-            // normalized type is assigned to the local variable.
-            SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR structDesc;
-            assert(info.compMethodInfo->args.retTypeClass != nullptr);
-            eeGetSystemVAmd64PassStructInRegisterDescriptor(info.compMethodInfo->args.retTypeClass, &structDesc);
-            if (structDesc.passedInRegisters && structDesc.eightByteCount <= 1)
-            {
-                retLocalType = lvaTable[genReturnLocal].lvType = getEightByteType(structDesc, 0);
-            }
-            else
-            {
-                lvaTable[genReturnLocal].lvType = TYP_STRUCT;
-            }
+            lvaTable[genReturnLocal].lvType = genActualType(info.compRetNativeType);
         }
-        else
-#endif // defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
+        else if (compMethodReturnsRetBufAddr())
         {
-            lvaTable[genReturnLocal].lvType = genActualType(info.compRetNativeType);
+            lvaTable[genReturnLocal].lvType = TYP_BYREF;
         }
-        
-        if (varTypeIsFloating(lvaTable[genReturnLocal].lvType))
+        else if (compMethodReturnsMultiRegRetType())
         {
-            this->compFloatingPointUsed = true;
+            lvaTable[genReturnLocal].lvType = TYP_STRUCT;
+            lvaSetStruct(genReturnLocal, info.compMethodInfo->args.retTypeClass, true);
+#if FEATURE_MULTIREG_ARGS_OR_RET
+            lvaTable[genReturnLocal].lvIsMultiRegArgOrRet = true;
+#endif
         }
-
-#if defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
-        // Handle a struct return type for System V Amd64 systems.
-        if (varTypeIsStruct(info.compRetNativeType))
+        else
         {
-            // Handle the normalized return type.
-            if (varTypeIsStruct(retLocalType))
-            {
-                lvaSetStruct(genReturnLocal, info.compMethodInfo->args.retTypeClass, true);
-            }
-            else
-            {
-                lvaTable[genReturnLocal].lvVerTypeInfo = typeInfo(TI_STRUCT, info.compMethodInfo->args.retTypeClass);
-            }
+            assert(!"unreached");
+        }
 
-            lvaTable[genReturnLocal].lvIsMultiRegArgOrRet = true;
+        if (varTypeIsFloating(lvaTable[genReturnLocal].lvType))
+        {
+            this->compFloatingPointUsed = true;
         }
-#endif // defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
 
         if (!varTypeIsFloating(info.compRetType))
             lvaTable[genReturnLocal].setPrefReg(REG_INTRET, this);
@@ -8498,20 +8467,17 @@ void                Compiler::fgAddInternal()
         //
 
         // spill any value that is currently in the oneReturnStmtNode
-        if (oneReturnStmtNode != NULL)
+        if (oneReturnStmtNode != nullptr)
         {
             fgInsertStmtAtEnd(genReturnBB, oneReturnStmtNode);
-            oneReturnStmtNode = NULL;
+            oneReturnStmtNode = nullptr;
         }
 
         //make sure to reload the return value as part of the return (it is saved by the "real return").
         if (genReturnLocal != BAD_VAR_NUM)
         {
-#if defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
-            noway_assert(info.compRetType != TYP_VOID);
-#else // !defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
-            noway_assert(info.compRetType != TYP_VOID && info.compRetNativeType != TYP_STRUCT);
-#endif // !defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
+            noway_assert(compMethodHasRetVal());
+
             GenTreePtr retTemp = gtNewLclvNode(genReturnLocal, lvaTable[genReturnLocal].TypeGet());
 
             //make sure copy prop ignores this node (make sure it always does a reload from the temp).

diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index a4ab013..6ba1f01 100644 (file)
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -14131,7 +14131,7 @@ void                Compiler::fgMorphBlocks()
 
         if (block->bbJumpKind == BBJ_RETURN)
         {
-             if ((genReturnBB != NULL)  &&
+             if ((genReturnBB != nullptr)  &&
                  (genReturnBB != block) &&
                  ((block->bbFlags & BBF_HAS_JMP) == 0))
              {
@@ -14166,11 +14166,7 @@ void                Compiler::fgMorphBlocks()
                 //replace the GT_RETURN node to be a GT_ASG that stores the return value into genReturnLocal.
                 if (genReturnLocal != BAD_VAR_NUM)
                 {
-#if defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
-                    noway_assert(info.compRetType != TYP_VOID);
-#else 
-                    noway_assert(info.compRetType != TYP_VOID && info.compRetNativeType != TYP_STRUCT);
-#endif
+                    noway_assert(compMethodHasRetVal());
 
                     // GT_RETURN must have non-null operand as the method is returning the value assigned to genReturnLocal
                     noway_assert(ret->gtGetOp1() != nullptr);