Improve mapping of breakpoints to disassembly & source

* We breakpoint directly on source/disassembly lines rather than trying to map
  to instructions that may get stepped over and skipped.

qrenderdoc/Windows/ShaderViewer.cpp

@@ -2008,8 +2008,11 @@ bool ShaderViewer::step(bool forward, StepMode mode)
       else
         applyBackwardsChange();
 
+      LineColumnInfo curLine = m_Trace->lineInfo[GetCurrentState().nextInstruction];
+
       // break out if we hit a breakpoint, no matter what
-      if(m_Breakpoints.contains((int)GetCurrentState().nextInstruction))
+      if(m_Breakpoints.contains({-1, curLine.disassemblyLine}) ||
+         m_Breakpoints.contains({curLine.fileIndex, curLine.lineStart}))
         break;
 
       // if we've reached the limit, break
@@ -2021,7 +2024,6 @@ bool ShaderViewer::step(bool forward, StepMode mode)
         break;
 
       // keep going if we're still on the same source line as we started
-      LineColumnInfo curLine = m_Trace->lineInfo[GetCurrentState().nextInstruction];
       if(curLine.SourceEqual(oldLine))
         continue;
 
@@ -2121,8 +2123,10 @@ bool ShaderViewer::step(bool forward, StepMode mode)
 
           applyBackwardsChange();
 
+          LineColumnInfo curLine = m_Trace->lineInfo[GetCurrentState().nextInstruction];
+
           // still need to check for instruction-level breakpoints
-          if(m_Breakpoints.contains((int)GetCurrentState().nextInstruction))
+          if(m_Breakpoints.contains({-1, curLine.disassemblyLine}))
             break;
         }
       }
@@ -2133,8 +2137,10 @@ bool ShaderViewer::step(bool forward, StepMode mode)
         {
           applyBackwardsChange();
 
+          LineColumnInfo curLine = m_Trace->lineInfo[GetCurrentState().nextInstruction];
+
           // still need to check for instruction-level breakpoints
-          if(m_Breakpoints.contains((int)GetCurrentState().nextInstruction))
+          if(m_Breakpoints.contains({-1, curLine.disassemblyLine}))
             break;
         }
       }
@@ -2162,52 +2168,21 @@ void ShaderViewer::runToCursor(bool forward)
   if(!m_Trace || m_States.empty())
     return;
 
-  ScintillaEdit *cur = currentScintilla();
+  // don't update the UI or remove any breakpoints
+  m_TempBreakpoint = true;
+  QSet<QPair<int, uint32_t>> oldBPs = m_Breakpoints;
 
-  if(cur != m_DisassemblyView)
-  {
-    int scintillaIndex = m_FileScintillas.indexOf(cur);
+  // add a temporary breakpoint on the current instruction
+  ToggleBreakpointOnInstruction(-1);
 
-    if(scintillaIndex < 0)
-      return;
+  // run in the direction
+  runTo(~0U, forward);
 
-    sptr_t i = cur->lineFromPosition(cur->currentPos()) + 1;
-
-    LineColumnInfo sourceLine;
-    sourceLine.fileIndex = scintillaIndex;
-    sourceLine.lineStart = sourceLine.lineEnd = i;
-
-    auto it = m_Location2Inst.lowerBound(sourceLine);
-
-    // find the next source location that has an instruction mapped. If there was an exact match
-    // this won't loop, if the lower bound was earlier we'll step at most once to get to the next
-    // line past it.
-    if(it != m_Location2Inst.end() && (sptr_t)it.key().lineEnd < i)
-      it++;
-
-    if(it != m_Location2Inst.end())
-    {
-      runTo(it.value(), forward, ShaderEvents::NoEvent);
-      return;
-    }
-
-    // if we didn't find one, just run
-    runTo(~0U, forward);
-  }
-  else
-  {
-    sptr_t i = m_DisassemblyView->lineFromPosition(m_DisassemblyView->currentPos());
-
-    for(; i < m_DisassemblyView->lineCount(); i++)
-    {
-      int line = instructionForDisassemblyLine(i);
-      if(line >= 0)
-      {
-        runTo((uint32_t)line, forward);
-        break;
-      }
-    }
-  }
+  // turn off temp breakpoint state
+  m_TempBreakpoint = false;
+  // restore the set of breakpoints to what it was (this handles the case of doing 'run to cursor'
+  // on a line that already has a breakpoint)
+  m_Breakpoints = oldBPs;
 }
 
 int ShaderViewer::instructionForDisassemblyLine(sptr_t line)
@@ -2268,6 +2243,7 @@ void ShaderViewer::runTo(const rdcarray<uint32_t> &runToInstructions, bool forwa
     return;
 
   bool firstStep = true;
+  LineColumnInfo oldLine = m_Trace->lineInfo[GetCurrentState().nextInstruction];
 
   // this is effectively infinite as we break out before moving to next/previous state if that would
   // be first/last
@@ -2282,10 +2258,20 @@ void ShaderViewer::runTo(const rdcarray<uint32_t> &runToInstructions, bool forwa
       break;
 
     // or breakpoint
-    if(!firstStep && m_Breakpoints.contains((int)GetCurrentState().nextInstruction))
+    LineColumnInfo curLine = m_Trace->lineInfo[GetCurrentState().nextInstruction];
+    if(!firstStep && (m_Breakpoints.contains({-1, curLine.disassemblyLine}) ||
+                      m_Breakpoints.contains({curLine.fileIndex, curLine.lineStart})))
       break;
 
-    firstStep = false;
+    if(isSourceDebugging())
+    {
+      if(!curLine.SourceEqual(oldLine))
+        firstStep = false;
+    }
+    else
+    {
+      firstStep = false;
+    }
 
     if(forward)
     {
@@ -2344,7 +2330,9 @@ void ShaderViewer::runToResourceAccess(bool forward, VarType type, const Bindpoi
       break;
 
     // or breakpoint
-    if(m_Breakpoints.contains((int)GetCurrentState().nextInstruction))
+    LineColumnInfo curLine = m_Trace->lineInfo[GetCurrentState().nextInstruction];
+    if(m_Breakpoints.contains({-1, curLine.disassemblyLine}) ||
+       m_Breakpoints.contains({curLine.fileIndex, curLine.lineStart}))
       break;
   }
 
@@ -4936,9 +4924,16 @@ void ShaderViewer::ToggleBreakpointOnInstruction(int32_t instruction)
   if(!m_Trace || m_States.empty())
     return;
 
-  sptr_t instLine = -1;
+  QPair<int, uint32_t> sourceBreakpoint = {-1, 0};
+  QList<QPair<int, uint32_t>> disasmBreakpoints;
 
-  if(instruction == -1)
+  if(instruction >= 0)
+  {
+    LineColumnInfo &instLine = m_Trace->lineInfo[instruction];
+    sourceBreakpoint = {instLine.fileIndex, instLine.lineStart};
+    disasmBreakpoints.push_back({-1, instLine.disassemblyLine});
+  }
+  else
   {
     ScintillaEdit *cur = currentScintilla();
 
@@ -4957,27 +4952,6 @@ void ShaderViewer::ToggleBreakpointOnInstruction(int32_t instruction)
       sourceLine.fileIndex = scintillaIndex;
       sourceLine.lineStart = sourceLine.lineEnd = i;
 
-      // first see if any of the existing breakpoints map to this source location. They may come
-      // from a different 'non-canonical' instruction which is still on the same line. If the user
-      // is toggling on that source location, they expect to disable the breakpoint on that
-      // instruction even if it's not the instruction where a breakpoint will be *added* if they add
-      // one there.
-      bool exactMatch = false;
-      QList<int> bps = m_Breakpoints;
-      for(int inst : bps)
-      {
-        LineColumnInfo instSourceLine = m_Trace->lineInfo[inst];
-        // ignore columns
-        instSourceLine.colStart = instSourceLine.colEnd = 0;
-        if(instSourceLine.SourceEqual(sourceLine))
-        {
-          ToggleBreakpointOnInstruction(inst);
-          exactMatch = true;
-        }
-      }
-      if(exactMatch)
-        return;
-
       auto it = m_Location2Inst.lowerBound(sourceLine);
 
       // find the next source location that has an instruction mapped. If there was an exact match
@@ -4989,88 +4963,128 @@ void ShaderViewer::ToggleBreakpointOnInstruction(int32_t instruction)
       // set a breakpoint on all instructions that match this line
       if(it != m_Location2Inst.end())
       {
+        sourceBreakpoint = {scintillaIndex, (uint32_t)i};
         for(uint32_t inst : it.value())
-          ToggleBreakpointOnInstruction((int)inst);
+          disasmBreakpoints.push_back({-1, m_Trace->lineInfo[inst].disassemblyLine});
+      }
+      else
+      {
         return;
       }
     }
     else
     {
-      instLine = m_DisassemblyView->lineFromPosition(m_DisassemblyView->currentPos());
+      int32_t disassemblyLine = m_DisassemblyView->lineFromPosition(m_DisassemblyView->currentPos());
 
-      for(; instLine < m_DisassemblyView->lineCount(); instLine++)
+      for(; disassemblyLine < m_DisassemblyView->lineCount(); disassemblyLine++)
       {
-        instruction = instructionForDisassemblyLine(instLine);
-
-        if(instruction >= 0)
+        if(instructionForDisassemblyLine(disassemblyLine) >= 0)
           break;
       }
+
+      if(disassemblyLine < m_DisassemblyView->lineCount())
+        disasmBreakpoints.push_back({-1, (uint32_t)disassemblyLine + 1});
     }
   }
 
-  if(instruction < 0 || instruction >= m_Trace->lineInfo.count())
-    return;
-
-  if(instLine == -1)
+  // if we have a source breakpoint, treat that as 'canonical' whether or not the corresponding
+  // disasm ones are set
+  if(sourceBreakpoint.first >= 0)
   {
-    if(instruction < m_Trace->lineInfo.count())
-      instLine = m_Trace->lineInfo[instruction].disassemblyLine - 1;
-  }
-
-  if(m_Breakpoints.contains(instruction))
-  {
-    if(instLine >= 0)
+    if(m_TempBreakpoint)
     {
-      m_DisassemblyView->markerDelete(instLine, BREAKPOINT_MARKER);
-      m_DisassemblyView->markerDelete(instLine, BREAKPOINT_MARKER + 1);
+      m_Breakpoints.insert(sourceBreakpoint);
+      for(QPair<int, uint32_t> &d : disasmBreakpoints)
+        m_Breakpoints.insert(d);
+    }
+    else if(m_Breakpoints.contains(sourceBreakpoint))
+    {
+      m_Breakpoints.remove(sourceBreakpoint);
+      m_FileScintillas[sourceBreakpoint.first]->markerDelete(sourceBreakpoint.second - 1,
+                                                             BREAKPOINT_MARKER);
+      m_FileScintillas[sourceBreakpoint.first]->markerDelete(sourceBreakpoint.second - 1,
+                                                             BREAKPOINT_MARKER + 1);
 
-      const LineColumnInfo &lineInfo = m_Trace->lineInfo[instruction];
-
-      if(lineInfo.fileIndex >= 0 && lineInfo.fileIndex < m_FileScintillas.count())
+      for(QPair<int, uint32_t> &d : disasmBreakpoints)
       {
-        for(sptr_t line = lineInfo.lineStart; line <= (sptr_t)lineInfo.lineEnd; line++)
-        {
-          ScintillaEdit *s = m_FileScintillas[lineInfo.fileIndex];
-          if(s)
-          {
-            m_FileScintillas[lineInfo.fileIndex]->markerDelete(line - 1, BREAKPOINT_MARKER);
-            m_FileScintillas[lineInfo.fileIndex]->markerDelete(line - 1, BREAKPOINT_MARKER + 1);
-          }
-        }
+        m_Breakpoints.remove(d);
+        m_DisassemblyView->markerDelete(d.second - 1, BREAKPOINT_MARKER);
+        m_DisassemblyView->markerDelete(d.second - 1, BREAKPOINT_MARKER + 1);
+      }
+    }
+    else
+    {
+      m_Breakpoints.insert(sourceBreakpoint);
+      m_FileScintillas[sourceBreakpoint.first]->markerAdd(sourceBreakpoint.second - 1,
+                                                          BREAKPOINT_MARKER);
+      m_FileScintillas[sourceBreakpoint.first]->markerAdd(sourceBreakpoint.second - 1,
+                                                          BREAKPOINT_MARKER + 1);
+
+      for(QPair<int, uint32_t> &d : disasmBreakpoints)
+      {
+        m_Breakpoints.insert(d);
+        m_DisassemblyView->markerAdd(d.second - 1, BREAKPOINT_MARKER);
+        m_DisassemblyView->markerAdd(d.second - 1, BREAKPOINT_MARKER + 1);
       }
     }
-    m_Breakpoints.removeOne(instruction);
   }
   else
   {
-    if(instLine >= 0)
+    if(disasmBreakpoints.empty())
+      return;
+
+    QPair<int, uint32_t> &bp = disasmBreakpoints[0];
+
+    if(m_TempBreakpoint)
     {
-      m_DisassemblyView->markerAdd(instLine, BREAKPOINT_MARKER);
-      m_DisassemblyView->markerAdd(instLine, BREAKPOINT_MARKER + 1);
-
-      const LineColumnInfo &lineInfo = m_Trace->lineInfo[instruction];
-
-      if(lineInfo.fileIndex >= 0 && lineInfo.fileIndex < m_FileScintillas.count())
-      {
-        for(sptr_t line = lineInfo.lineStart; line <= (sptr_t)lineInfo.lineEnd; line++)
-        {
-          ScintillaEdit *s = m_FileScintillas[lineInfo.fileIndex];
-          if(s)
-          {
-            m_FileScintillas[lineInfo.fileIndex]->markerAdd(line - 1, BREAKPOINT_MARKER);
-            m_FileScintillas[lineInfo.fileIndex]->markerAdd(line - 1, BREAKPOINT_MARKER + 1);
-          }
-        }
-      }
-      m_Breakpoints.push_back(instruction);
+      m_Breakpoints.insert(bp);
+    }
+    else if(m_Breakpoints.contains(bp))
+    {
+      m_Breakpoints.remove(bp);
+      m_DisassemblyView->markerDelete(bp.second - 1, BREAKPOINT_MARKER);
+      m_DisassemblyView->markerDelete(bp.second - 1, BREAKPOINT_MARKER + 1);
+    }
+    else
+    {
+      m_Breakpoints.insert(bp);
+      m_DisassemblyView->markerAdd(bp.second - 1, BREAKPOINT_MARKER);
+      m_DisassemblyView->markerAdd(bp.second - 1, BREAKPOINT_MARKER + 1);
     }
   }
 }
 
 void ShaderViewer::ToggleBreakpointOnDisassemblyLine(int32_t disassemblyLine)
 {
-  // instructionForDisassemblyLine expects scintilla 0-based line numbers
-  ToggleBreakpointOnInstruction(instructionForDisassemblyLine(disassemblyLine - 1));
+  if(!m_Trace || m_States.empty())
+    return;
+
+  // move forward to the next actual mapped line
+  for(; disassemblyLine < m_DisassemblyView->lineCount(); disassemblyLine++)
+  {
+    if(instructionForDisassemblyLine(disassemblyLine - 1) >= 0)
+      break;
+  }
+
+  if(disassemblyLine >= m_DisassemblyView->lineCount())
+    return;
+
+  if(m_TempBreakpoint)
+  {
+    m_Breakpoints.insert({-1, (uint32_t)disassemblyLine});
+  }
+  else if(m_Breakpoints.contains({-1, (uint32_t)disassemblyLine}))
+  {
+    m_Breakpoints.remove({-1, (uint32_t)disassemblyLine});
+    m_DisassemblyView->markerDelete(disassemblyLine - 1, BREAKPOINT_MARKER);
+    m_DisassemblyView->markerDelete(disassemblyLine - 1, BREAKPOINT_MARKER + 1);
+  }
+  else
+  {
+    m_Breakpoints.insert({-1, (uint32_t)disassemblyLine});
+    m_DisassemblyView->markerAdd(disassemblyLine - 1, BREAKPOINT_MARKER);
+    m_DisassemblyView->markerAdd(disassemblyLine - 1, BREAKPOINT_MARKER + 1);
+  }
 }
 
 void ShaderViewer::RunForward()

qrenderdoc/Windows/ShaderViewer.h

@@ -26,6 +26,7 @@
 
 #include <QFrame>
 #include <QSemaphore>
+#include <QSet>
 #include <QStyledItemDelegate>
 #include "Code/Interface/QRDInterface.h"
 
@@ -314,7 +315,8 @@ private:
 
   rdcarray<BoundResourceArray> m_ReadOnlyResources;
   rdcarray<BoundResourceArray> m_ReadWriteResources;
-  QList<int> m_Breakpoints;
+  QSet<QPair<int, uint32_t>> m_Breakpoints;
+  bool m_TempBreakpoint = false;
 
   QList<QPair<ScintillaEdit *, int>> m_FindAllResults;