Improve optimized debugging experience by generating multiple native<…

…->IL mappings

Currently we have some pretty convoluted logic that tries to ensure we
only generate multiple IP mappings mapping to the same native offset in
rare cases. Otherwise we try to remove one of the mappings so that we
end up with only one IL offset for each native offset.

In optimized code it is common that we have multiple IL offsets mapping
to the same native code address, for example when a statement does not
end up generating any native code (e.g. GT_ASG between two locals in the
same register). The loss of one of the mappings leads to a poor
debugging experience in those cases, where breakpoints fail to bind
completely depending on which statement is breakpointed.

This changes the debug mappings logic so that we only discard mappings
for the same native offset when one of the mappings does not map to any
IL offset, or when both mappings map to literally the same IL offset.

This should not affect code when compiled without optimizations: there
we emit nops when we encounter subsequent IL offsets without any
intermediate generated native code.

src/coreclr/debug/ee/debugger.h

@@ -1583,14 +1583,7 @@ class DebuggerJitInfo
                                    DebuggerILToNativeMap **end);
     NativeOffset MapILOffsetToNative(ILOffset ilOffset);
 
-    // MapSpecialToNative maps a CordDebugMappingResult to a native
-    //      offset so that we can get the address of the prolog & epilog. which
-    //      determines which epilog or prolog, if there's more than one.
-    SIZE_T MapSpecialToNative(CorDebugMappingResult mapping,
-                              SIZE_T which,
-                              BOOL *pfAccurate);
 #if defined(FEATURE_EH_FUNCLETS)
-    void   MapSpecialToNative(int funcletIndex, DWORD* pPrologEndOffset, DWORD* pEpilogStartOffset);
     SIZE_T MapILOffsetToNativeForSetIP(SIZE_T offsetILTo, int funcletIndexFrom, EHRangeTree* pEHRT, BOOL* pExact);
 #endif // FEATURE_EH_FUNCLETS
 

src/coreclr/debug/ee/functioninfo.cpp

@@ -523,71 +523,6 @@ void DebuggerJitInfo::ILToNativeOffsetIterator::Next()
 #endif // !FEATURE_EH_FUNCLETS
 }
 
-
-
-// SIZE_T DebuggerJitInfo::MapSpecialToNative():  Maps something like
-//      a prolog to a native offset.
-// CordDebugMappingResult mapping:  Mapping type to be looking for.
-// SIZE_T which:  Which one.  <TODO>For now, set to zero.  <@todo Later, we'll
-//      change this to some value that we get back from MapNativeToILOffset
-//      to indicate which of the (possibly multiple epilogs) that may
-//      be present.</TODO>
-
-SIZE_T DebuggerJitInfo::MapSpecialToNative(CorDebugMappingResult mapping,
-                                           SIZE_T which,
-                                           BOOL *pfAccurate)
-{
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-        PRECONDITION(NULL != pfAccurate);
-    }
-    CONTRACTL_END;
-
-    LOG((LF_CORDB, LL_INFO10000, "DJI::MSTN map:0x%x which:0x%x\n", mapping, which));
-
-    bool fFound;
-    SIZE_T  cFound = 0;
-
-    DebuggerILToNativeMap *m = GetSequenceMap();
-    DebuggerILToNativeMap *mEnd = m + GetSequenceMapCount();
-    if (m)
-    {
-        while(m < mEnd)
-        {
-            _ASSERTE(m>=GetSequenceMap());
-
-            fFound = false;
-
-            if (DbgIsSpecialILOffset(m->ilOffset))
-                cFound++;
-
-            if (cFound == which)
-            {
-                _ASSERTE( (mapping == MAPPING_PROLOG &&
-                    m->ilOffset == (ULONG) ICorDebugInfo::PROLOG) ||
-                          (mapping == MAPPING_EPILOG &&
-                    m->ilOffset == (ULONG) ICorDebugInfo::EPILOG) ||
-                          ((mapping == MAPPING_NO_INFO || mapping == MAPPING_UNMAPPED_ADDRESS) &&
-                    m->ilOffset == (ULONG) ICorDebugInfo::NO_MAPPING)
-                        );
-
-                (*pfAccurate) = TRUE;
-                LOG((LF_CORDB, LL_INFO10000, "DJI::MSTN found mapping to nat:0x%x\n",
-                    m->nativeStartOffset));
-                return m->nativeStartOffset;
-            }
-            m++;
-        }
-    }
-
-    LOG((LF_CORDB, LL_INFO10000, "DJI::MSTN No mapping found :(\n"));
-    (*pfAccurate) = FALSE;
-
-    return 0;
-}
-
 #if defined(FEATURE_EH_FUNCLETS)
 //
 // DebuggerJitInfo::MapILOffsetToNativeForSetIP()

src/coreclr/jit/codegencommon.cpp

@@ -10644,49 +10644,25 @@ void CodeGen::genIPmappingGen()
             continue;
         }
 
-        // Both have mappings.
-        // If previous is the prolog, keep both if this one is at IL offset 0.
-        // (TODO: Why? Debugger has no problem breaking on the prolog mapping
-        // it seems.)
-        if ((prev->ipmdKind == IPmappingDscKind::Prolog) && (it->ipmdKind == IPmappingDscKind::Normal) &&
-            (it->ipmdLoc.GetOffset() == 0))
+        // If these are literally the same mappings, then remove one.
+        if ((it->ipmdKind == prev->ipmdKind) && ((it->ipmdKind != IPmappingDscKind::Normal) || (it->ipmdLoc.GetOffset() == prev->ipmdLoc.GetOffset())))
         {
-            ++it;
-            continue;
-        }
-
-        // For the special case of an IL instruction with no body followed by
-        // the epilog (say ret void immediately preceding the method end), we
-        // leave both entries in, so that we'll stop at the (empty) ret
-        // statement if the user tries to put a breakpoint there, and then have
-        // the option of seeing the epilog or not based on SetUnmappedStopMask
-        // for the stepper.
-        if (it->ipmdKind == IPmappingDscKind::Epilog)
-        {
-            ++it;
-            continue;
-        }
+            // If these are normal mappings at the same offset then combine
+            // location flags.
+            if (it->ipmdKind == IPmappingDscKind::Normal)
+            {
+                prev->ipmdLoc =
+                    ILLocation(
+                        prev->ipmdLoc.GetOffset(),
+                        prev->ipmdLoc.IsStackEmpty() | it->ipmdLoc.IsStackEmpty(),
+                        prev->ipmdLoc.IsCall() | it->ipmdLoc.IsCall());
+            }
 
-        // For managed return values we store all calls. Keep both in this case
-        // too.
-        if (((prev->ipmdKind == IPmappingDscKind::Normal) && (prev->ipmdLoc.IsCall())) ||
-            ((it->ipmdKind == IPmappingDscKind::Normal) && (it->ipmdLoc.IsCall())))
-        {
-            ++it;
+            it = compiler->genIPmappings.erase(it);
             continue;
         }
 
-        // Otherwise report the higher offset unless the previous mapping is a
-        // label.
-        if (prev->ipmdIsLabel)
-        {
-            it = compiler->genIPmappings.erase(it);
-        }
-        else
-        {
-            compiler->genIPmappings.erase(prev);
-            ++it;
-        }
+        ++it;
     }
 
     // Tell them how many mapping records we've got