From 20f42feab40f6385e36e6d76a8d91e53fd22a231 Mon Sep 17 00:00:00 2001 From: baldurk Date: Wed, 22 Jun 2022 13:47:24 +0100 Subject: [PATCH] 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 | 266 +++++++++++++++------------- qrenderdoc/Windows/ShaderViewer.h | 4 +- 2 files changed, 143 insertions(+), 127 deletions(-) diff --git a/qrenderdoc/Windows/ShaderViewer.cpp b/qrenderdoc/Windows/ShaderViewer.cpp index dd2cd96eb..dab0a6357 100644 --- a/qrenderdoc/Windows/ShaderViewer.cpp +++ b/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> 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 &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 &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 sourceBreakpoint = {-1, 0}; + QList> 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 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 &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 &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 &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 &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() diff --git a/qrenderdoc/Windows/ShaderViewer.h b/qrenderdoc/Windows/ShaderViewer.h index f0b09ffcf..52f605c30 100644 --- a/qrenderdoc/Windows/ShaderViewer.h +++ b/qrenderdoc/Windows/ShaderViewer.h @@ -26,6 +26,7 @@ #include #include +#include #include #include "Code/Interface/QRDInterface.h" @@ -314,7 +315,8 @@ private: rdcarray m_ReadOnlyResources; rdcarray m_ReadWriteResources; - QList m_Breakpoints; + QSet> m_Breakpoints; + bool m_TempBreakpoint = false; QList> m_FindAllResults;