Annotate handle indirections with GTF_IND_INVARIANT and add validation to fgDebugCheckFlags

src/coreclr/src/jit/compiler.cpp

@@ -8890,14 +8890,14 @@ void cTreeFlags(Compiler* comp, GenTree* tree)
                         chars += printf("[ICON_STR_HDL]");
                         break;
 
-                    case GTF_ICON_PSTR_HDL:
+                    case GTF_ICON_CONST_PTR:
 
-                        chars += printf("[ICON_PSTR_HDL]");
+                        chars += printf("[ICON_CONST_PTR]");
                         break;
 
-                    case GTF_ICON_PTR_HDL:
+                    case GTF_ICON_GLOBAL_PTR:
 
-                        chars += printf("[ICON_PTR_HDL]");
+                        chars += printf("[ICON_GLOBAL_PTR]");
                         break;
 
                     case GTF_ICON_VARG_HDL:

src/coreclr/src/jit/compiler.h

@@ -5269,6 +5269,7 @@ class Compiler
     void fgDebugCheckNodesUniqueness();
 
     void fgDebugCheckFlags(GenTree* tree);
+    void fgDebugCheckDispFlags(GenTree* tree, unsigned dispFlags, unsigned debugFlags);
     void fgDebugCheckFlagsHelper(GenTree* tree, unsigned treeFlags, unsigned chkFlags);
     void fgDebugCheckTryFinallyExits();
 #endif

src/coreclr/src/jit/compiler.hpp

@@ -1148,7 +1148,7 @@ inline GenTreeCall* Compiler::gtNewRuntimeLookupHelperCallNode(CORINFO_RUNTIME_L
                                                                GenTree*                ctxTree,
                                                                void*                   compileTimeHandle)
 {
-    GenTree* argNode = gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_TOKEN_HDL, compileTimeHandle);
+    GenTree* argNode = gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_GLOBAL_PTR, compileTimeHandle);
     GenTreeCall::Use* helperArgs = gtNewCallArgs(ctxTree, argNode);
 
     return gtNewHelperCallNode(pRuntimeLookup->helper, TYP_I_IMPL, helperArgs);

src/coreclr/src/jit/emitarm64.cpp

@@ -12309,7 +12309,15 @@ void emitter::emitDispIns(
                     {
                         targetName = "SetGlobalSecurityCookie";
                     }
-                    else if ((idFlags == GTF_ICON_STR_HDL) || (idFlags == GTF_ICON_PSTR_HDL))
+                    else if (idFlags == GTF_ICON_CONST_PTR)
+                    {
+                        targetName = "const ptr";
+                    }
+                    else if (idFlags == GTF_ICON_GLOBAL_PTR)
+                    {
+                        targetName = "global ptr";
+                    }
+                    else if (idFlags == GTF_ICON_STR_HDL)
                     {
                         stringLiteral = emitComp->eeGetCPString(targetHandle);
                         // Note that eGetCPString isn't currently implemented on Linux/ARM

src/coreclr/src/jit/flowgraph.cpp

@@ -4371,7 +4371,7 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
         {
             // Use a double indirection
             GenTree* addr =
-                gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pAddrOfCaptureThreadGlobal, GTF_ICON_PTR_HDL, true);
+                gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pAddrOfCaptureThreadGlobal, GTF_ICON_CONST_PTR, true);
 
             value = gtNewOperNode(GT_IND, TYP_INT, addr);
             // This indirection won't cause an exception.
@@ -4380,7 +4380,7 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
         else
         {
             // Use a single indirection
-            value = gtNewIndOfIconHandleNode(TYP_INT, (size_t)addrTrap, GTF_ICON_PTR_HDL, false);
+            value = gtNewIndOfIconHandleNode(TYP_INT, (size_t)addrTrap, GTF_ICON_GLOBAL_PTR, false);
         }
 
         // Treat the reading of g_TrapReturningThreads as volatile.
@@ -9525,7 +9525,8 @@ void Compiler::fgAddInternal()
 
     if (dbgHandle || pDbgHandle)
     {
-        GenTree* embNode        = gtNewIconEmbHndNode(dbgHandle, pDbgHandle, GTF_ICON_TOKEN_HDL, info.compMethodHnd);
+        // Test the JustMyCode VM global state variable
+        GenTree* embNode        = gtNewIconEmbHndNode(dbgHandle, pDbgHandle, GTF_ICON_GLOBAL_PTR, info.compMethodHnd);
         GenTree* guardCheckVal  = gtNewOperNode(GT_IND, TYP_INT, embNode);
         GenTree* guardCheckCond = gtNewOperNode(GT_EQ, TYP_INT, guardCheckVal, gtNewZeroConNode(TYP_INT));
 
@@ -21854,6 +21855,77 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
             case GT_ADDR:
                 assert(!op1->CanCSE());
 
+            case GT_IND:
+                // Do we have a constant integer address as op1?
+                //
+                if (op1->OperGet() == GT_CNS_INT)
+                {
+                    // Is this constant a handle of some kind?
+                    //
+                    unsigned handleKind = (op1->gtFlags & GTF_ICON_HDL_MASK);
+                    if (handleKind != 0)
+                    {
+                        // Is the GTF_IND_INVARIANT flag set or unset?
+                        //
+                        bool invariantFlag = (tree->gtFlags & GTF_IND_INVARIANT) != 0;
+                        if (invariantFlag)
+                        {
+                            // Record the state of the GTF_IND_INVARIANT flags into 'chkFlags'
+                            chkFlags |= GTF_IND_INVARIANT;
+                        }
+
+                        // Is the GTF_IND_NONFAULTING flag set or unset?
+                        //
+                        bool nonFaultingFlag = (tree->gtFlags & GTF_IND_NONFAULTING) != 0;
+                        if (nonFaultingFlag)
+                        {
+                            // Record the state of the GTF_IND_NONFAULTING flags into 'chkFlags'
+                            chkFlags |= GTF_IND_NONFAULTING;
+                        }
+                        assert(nonFaultingFlag); // Currently this should always be set for all handle kinds
+
+                        // Some of these aren't handles to invariant data...
+                        //
+                        if ((handleKind == GTF_ICON_STATIC_HDL) || // Pointer to a mutable class Static variable
+                            (handleKind == GTF_ICON_BBC_PTR) ||    // Pointer to a mutable basic block count value
+                            (handleKind == GTF_ICON_GLOBAL_PTR))   // Pointer to mutable data from the VM state
+
+                        {
+                            // We expect the Invariant flag to be unset for this handleKind
+                            // If it is set then we will assert with "unexpected GTF_IND_INVARIANT flag set ...
+                            //
+                            if (handleKind == GTF_ICON_STATIC_HDL)
+                            {
+                                // We expect the GTF_GLOB_REF flag to be set for this handleKind
+                                // If it is not set then we will assert with "Missing flags on tree"
+                                //
+                                treeFlags |= GTF_GLOB_REF;
+                            }
+                        }
+                        else // All the other handle indirections are considered invariant
+                        {
+                            // We expect the Invariant flag to be set for this handleKind
+                            // If it is not set then we will assert with "Missing flags on tree"
+                            //
+                            treeFlags |= GTF_IND_INVARIANT;
+                        }
+
+                        // We currently expect all handle kinds to be nonFaulting
+                        //
+                        treeFlags |= GTF_IND_NONFAULTING;
+
+                        // Matrix for GTF_IND_INVARIANT (treeFlags and chkFlags)
+                        //
+                        //                    chkFlags INVARIANT value
+                        //                       0                 1
+                        //                 +--------------+----------------+
+                        //  treeFlags   0  |    OK        |  Missing Flag  |
+                        //  INVARIANT      +--------------+----------------+
+                        //  value:      1  |  Extra Flag  |       OK       |
+                        //                 +--------------+----------------+
+                    }
+                }
+
             default:
                 break;
         }
@@ -22102,6 +22174,27 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
     fgDebugCheckFlagsHelper(tree, treeFlags, chkFlags);
 }
 
+//------------------------------------------------------------------------------
+// fgDebugCheckDispFlags:
+//    Wrapper function that displays two GTF_IND_ flags
+//      and then calls ftDispFlags to display the rest.
+//
+// Arguments:
+//    tree       - Tree whose flags are being checked
+//    dispFlags  - the first argument for gtDispFlags
+//                 ands hold GTF_IND_INVARIANT and GTF_IND_NONFLUALTING
+//    debugFlags - the second argument to gtDispFlags
+//
+void Compiler::fgDebugCheckDispFlags(GenTree* tree, unsigned dispFlags, unsigned debugFlags)
+{
+    if (tree->OperGet() == GT_IND)
+    {
+        printf("%c", (dispFlags & GTF_IND_INVARIANT) ? '#' : '-');
+        printf("%c", (dispFlags & GTF_IND_NONFAULTING) ? 'n' : '-');
+    }
+    GenTree::gtDispFlags(dispFlags, debugFlags);
+}
+
 //------------------------------------------------------------------------------
 // fgDebugCheckFlagsHelper : Check if all bits that are set in chkFlags are also set in treeFlags.
 //
@@ -22120,34 +22213,37 @@ void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, unsigned treeFlags, unsign
     {
         // Print the tree so we can see it in the log.
         printf("Missing flags on tree [%06d]: ", dspTreeID(tree));
-        GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE);
+        Compiler::fgDebugCheckDispFlags(tree, chkFlags & ~treeFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
 
         noway_assert(!"Missing flags on tree");
 
         // Print the tree again so we can see it right after we hook up the debugger.
         printf("Missing flags on tree [%06d]: ", dspTreeID(tree));
-        GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE);
+        Compiler::fgDebugCheckDispFlags(tree, chkFlags & ~treeFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
     }
     else if (treeFlags & ~chkFlags)
     {
-        // TODO: We are currently only checking extra GTF_EXCEPT, GTF_ASG, and GTF_CALL flags.
-        if ((treeFlags & ~chkFlags & ~GTF_GLOB_REF & ~GTF_ORDER_SIDEEFF) != 0)
+        // We can't/don't consider these flags (GTF_GLOB_REF or GTF_ORDER_SIDEEFF) as being "extra" flags
+        //
+        unsigned flagsToCheck = ~GTF_GLOB_REF & ~GTF_ORDER_SIDEEFF;
+
+        if ((treeFlags & ~chkFlags & flagsToCheck) != 0)
         {
             // Print the tree so we can see it in the log.
-            printf("Extra flags on parent tree [%X]: ", tree);
-            GenTree::gtDispFlags(treeFlags & ~chkFlags, GTF_DEBUG_NONE);
+            printf("Extra flags on tree [%06d]: ", dspTreeID(tree));
+            Compiler::fgDebugCheckDispFlags(tree, treeFlags & ~chkFlags, GTF_DEBUG_NONE);
             printf("\n");
             gtDispTree(tree);
 
             noway_assert(!"Extra flags on tree");
 
             // Print the tree again so we can see it right after we hook up the debugger.
-            printf("Extra flags on parent tree [%X]: ", tree);
-            GenTree::gtDispFlags(treeFlags & ~chkFlags, GTF_DEBUG_NONE);
+            printf("Extra flags on tree [%06d]: ", dspTreeID(tree));
+            Compiler::fgDebugCheckDispFlags(tree, treeFlags & ~chkFlags, GTF_DEBUG_NONE);
             printf("\n");
             gtDispTree(tree);
         }

src/coreclr/src/jit/gentree.cpp

@@ -5987,18 +5987,25 @@ GenTree* Compiler::gtNewIndOfIconHandleNode(var_types indType, size_t addr, unsi
     //
     indNode->gtFlags |= GTF_IND_NONFAULTING;
 
-    // String Literal handles are indirections that return a TYP_REF.
-    // They are pointers into the GC heap and they are not invariant
-    // as the address is a reportable GC-root and as such it can be
-    // modified during a GC collection
+    // String Literal handles are indirections that return a TYP_REF, and
+    // these are pointers into the GC heap.  We don't currently have any
+    // TYP_BYREF pointers, but if we did they also must be pointers into the GC heap.
     //
-    if (indType == TYP_REF)
+    // Also every GTF_ICON_STATIC_HDL also must be a pointer into the GC heap
+    // we will set GTF_GLOB_REF for these kinds of references.
+    //
+    if ((varTypeIsGC(indType)) || (iconFlags == GTF_ICON_STATIC_HDL))
     {
-        // This indirection points into the gloabal heap
+        // This indirection also points into the gloabal heap
         indNode->gtFlags |= GTF_GLOB_REF;
     }
+
     if (isInvariant)
     {
+        assert(iconFlags != GTF_ICON_STATIC_HDL); // Pointer to a mutable class Static variable
+        assert(iconFlags != GTF_ICON_BBC_PTR);    // Pointer to a mutable basic block count value
+        assert(iconFlags != GTF_ICON_GLOBAL_PTR); // Pointer to mutable data from the VM state
+
         // This indirection also is invariant.
         indNode->gtFlags |= GTF_IND_INVARIANT;
     }
@@ -6044,12 +6051,9 @@ GenTree* Compiler::gtNewIconEmbHndNode(void* value, void* pValue, unsigned iconF
 
         // This indirection won't cause an exception.
         handleNode->gtFlags |= GTF_IND_NONFAULTING;
-#if 0
-        // It should also be invariant, but marking it as such leads to bad diffs.
 
         // This indirection also is invariant.
         handleNode->gtFlags |= GTF_IND_INVARIANT;
-#endif
     }
 
     iconNode->AsIntCon()->gtCompileTimeHandle = (size_t)compileTimeHandle;
@@ -6064,7 +6068,14 @@ GenTree* Compiler::gtNewStringLiteralNode(InfoAccessType iat, void* pValue)
 
     switch (iat)
     {
-        case IAT_VALUE: // constructStringLiteral in CoreRT case can return IAT_VALUE
+        case IAT_VALUE:
+            // For CoreRT only - Constant object can be a frozen string.
+            if (!IsTargetAbi(CORINFO_CORERT_ABI))
+            {
+                // Non CoreRT - This case is illegal, creating a TYP_REF from an INT_CNS
+                noway_assert(!"unreachable IAT_VALUE case in gtNewStringLiteralNode");
+            }
+
             tree         = gtNewIconEmbHndNode(pValue, nullptr, GTF_ICON_STR_HDL, nullptr);
             tree->gtType = TYP_REF;
 #ifdef DEBUG
@@ -6083,7 +6094,7 @@ GenTree* Compiler::gtNewStringLiteralNode(InfoAccessType iat, void* pValue)
 
         case IAT_PPVALUE: // The value needs to be accessed via a double indirection
             // Create the first indirection
-            tree = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pValue, GTF_ICON_PSTR_HDL, true);
+            tree = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pValue, GTF_ICON_CONST_PTR, true);
 #ifdef DEBUG
             tree->gtGetOp1()->AsIntCon()->gtTargetHandle = (size_t)pValue;
 #endif
@@ -10304,6 +10315,31 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, __in __in_z _
                 printf((tree->gtFlags & GTF_JCMP_EQ) ? "EQ" : "NE");
                 goto DASH;
 
+            case GT_CNS_INT:
+                if (tree->IsIconHandle())
+                {
+                    if ((tree->gtFlags & GTF_ICON_INITCLASS) != 0)
+                    {
+                        printf("I"); // Static Field handle with INITCLASS requirement
+                        --msgLength;
+                        break;
+                    }
+                    else if ((tree->gtFlags & GTF_ICON_FIELD_OFF) != 0)
+                    {
+                        printf("O");
+                        --msgLength;
+                        break;
+                    }
+                    else
+                    {
+                        // Some other handle
+                        printf("H");
+                        --msgLength;
+                        break;
+                    }
+                }
+                goto DASH;
+
             default:
             DASH:
                 printf("-");
@@ -10869,13 +10905,14 @@ void Compiler::gtDispConst(GenTree* tree)
             if (tree->IsIconHandle(GTF_ICON_STR_HDL))
             {
                 const WCHAR* str = eeGetCPString(tree->AsIntCon()->gtIconVal);
-                if (str != nullptr)
+                // If *str points to a '\0' then don't print the string's values
+                if ((str != nullptr) && (*str != '\0'))
                 {
                     printf(" 0x%X \"%S\"", dspPtr(tree->AsIntCon()->gtIconVal), str);
                 }
-                else
+                else // We can't print the value of the string
                 {
-                    // Note that eGetCPString isn't currently implemented on Linux/ARM
+                    // Note that eeGetCPString isn't currently implemented on Linux/ARM
                     // and instead always returns nullptr
                     printf(" 0x%X [ICON_STR_HDL]", dspPtr(tree->AsIntCon()->gtIconVal));
                 }
@@ -10941,11 +10978,11 @@ void Compiler::gtDispConst(GenTree* tree)
                         case GTF_ICON_STR_HDL:
                             unreached(); // This case is handled above
                             break;
-                        case GTF_ICON_PSTR_HDL:
-                            printf(" pstr");
+                        case GTF_ICON_CONST_PTR:
+                            printf(" const ptr");
                             break;
-                        case GTF_ICON_PTR_HDL:
-                            printf(" ptr");
+                        case GTF_ICON_GLOBAL_PTR:
+                            printf(" global ptr");
                             break;
                         case GTF_ICON_VARG_HDL:
                             printf(" vararg");

src/coreclr/src/jit/gentree.h

@@ -904,11 +904,11 @@ struct GenTree
 #define GTF_ICON_FIELD_HDL          0x40000000 // GT_CNS_INT -- constant is a field handle
 #define GTF_ICON_STATIC_HDL         0x50000000 // GT_CNS_INT -- constant is a handle to static data
 #define GTF_ICON_STR_HDL            0x60000000 // GT_CNS_INT -- constant is a string handle
-#define GTF_ICON_PSTR_HDL           0x70000000 // GT_CNS_INT -- constant is a ptr to a string handle
-#define GTF_ICON_PTR_HDL            0x80000000 // GT_CNS_INT -- constant is a ldptr handle
+#define GTF_ICON_CONST_PTR          0x70000000 // GT_CNS_INT -- constant is a pointer to immutable data, (e.g. IAT_PPVALUE)
+#define GTF_ICON_GLOBAL_PTR         0x80000000 // GT_CNS_INT -- constant is a pointer to mutable data (e.g. from the VM state)
 #define GTF_ICON_VARG_HDL           0x90000000 // GT_CNS_INT -- constant is a var arg cookie handle
 #define GTF_ICON_PINVKI_HDL         0xA0000000 // GT_CNS_INT -- constant is a pinvoke calli handle
-#define GTF_ICON_TOKEN_HDL          0xB0000000 // GT_CNS_INT -- constant is a token handle
+#define GTF_ICON_TOKEN_HDL          0xB0000000 // GT_CNS_INT -- constant is a token handle (other than class, method or field)
 #define GTF_ICON_TLS_HDL            0xC0000000 // GT_CNS_INT -- constant is a TLS ref with offset
 #define GTF_ICON_FTN_ADDR           0xD0000000 // GT_CNS_INT -- constant is a function address
 #define GTF_ICON_CIDMID_HDL         0xE0000000 // GT_CNS_INT -- constant is a class ID or a module ID