Simplify our breakpoint handling

Only store a Hash of break points in the debugger,
instead of the involved logic that currently adds and
removes break points. Add the current line number to
the Debug statements in the interpreter, and pass them
on to the debugger for checking whether we should really
break.

This adds a slight additional overhead to running inside the
debugger, but greatly simplifies the logic and doesn't require
modifying the bytecode anymore. This in turn opens up the
possibility to run the debugger on JIT generated code later on.

Change-Id: If2a3ae8f8d08b69a3a704cbbe0a84000f917a32e
Reviewed-by: Simon Hausmann <simon.hausmann@digia.com>

5 files changed, 71 insertions, 271 deletions

diff --git a/src/qml/compiler/qv4instr_moth_p.h b/src/qml/compiler/qv4instr_moth_p.h
index c424407e2c..4f8173fb80 100644
--- a/src/qml/compiler/qv4instr_moth_p.h
+++ b/src/qml/compiler/qv4instr_moth_p.h
@@ -236,7 +236,7 @@ union Instr
     };
     struct instr_debug {
         MOTH_INSTR_HEADER
-        quint32 breakPoint;
+        qint32 lineNumber;
     };
     struct instr_loadRuntimeString {
         MOTH_INSTR_HEADER
diff --git a/src/qml/compiler/qv4isel_moth.cpp b/src/qml/compiler/qv4isel_moth.cpp
index e26a190774..7f2af0829c 100644
--- a/src/qml/compiler/qv4isel_moth.cpp
+++ b/src/qml/compiler/qv4isel_moth.cpp
@@ -390,11 +390,6 @@ void InstructionSelection::run(int functionIndex)
 
     uint currentLine = -1;
     for (int i = 0, ei = _function->basicBlocks.size(); i != ei; ++i) {
-        if (irModule->debugMode) {
-            Instruction::Debug debug;
-            debug.breakPoint = 0;
-            addInstruction(debug);
-        }
         _block = _function->basicBlocks[i];
         _nextBlock = (i < ei - 1) ? _function->basicBlocks[i + 1] : 0;
         _addrs.insert(_block, _codeNext - _codeStart);
@@ -417,15 +412,20 @@ void InstructionSelection::run(int functionIndex)
             if (s->location.isValid()) {
                 lineNumberMappings << _codeNext - _codeStart << s->location.startLine;
                 if (irModule->debugMode && s->location.startLine != currentLine) {
+                    currentLine = s->location.startLine;
                     Instruction::Debug debug;
-                    debug.breakPoint = 0;
+                    debug.lineNumber = currentLine;
                     addInstruction(debug);
-                    currentLine = s->location.startLine;
                 }
             }
 
             s->accept(this);
         }
+        if (irModule->debugMode) {
+            Instruction::Debug debug;
+            debug.lineNumber = currentLine;
+            addInstruction(debug);
+        }
     }
 
     jsGenerator->registerLineNumberMapping(_function, lineNumberMappings);
@@ -1489,8 +1489,5 @@ void CompilationUnit::linkBackendToEngine(QV4::ExecutionEngine *engine)
         QV4::Function *runtimeFunction = new QV4::Function(engine, this, compiledFunction, &VME::exec);
         runtimeFunction->codeData = reinterpret_cast<const uchar *>(codeRefs.at(i).constData());
         runtimeFunctions[i] = runtimeFunction;
-
-        if (QV4::Debugging::Debugger *debugger = engine->debugger)
-            debugger->setPendingBreakpoints(runtimeFunction);
     }
 }
diff --git a/src/qml/jsruntime/qv4debugging.cpp b/src/qml/jsruntime/qv4debugging.cpp
index dade627206..7d19c7804e 100644
--- a/src/qml/jsruntime/qv4debugging.cpp
+++ b/src/qml/jsruntime/qv4debugging.cpp
@@ -115,14 +115,13 @@ Debugger::Debugger(QV4::ExecutionEngine *engine)
     : m_engine(engine)
     , m_agent(0)
     , m_state(Running)
-    , m_pauseRequested(false)
-    , m_gatherSources(0)
-    , m_havePendingBreakPoints(false)
-    , m_currentInstructionPointer(0)
     , m_stepping(NotStepping)
+    , m_pauseRequested(false)
+    , m_haveBreakPoints(false)
     , m_stopForStepping(false)
-    , m_returnedValue(Primitive::undefinedValue())
     , m_breakOnThrow(false)
+    , m_returnedValue(Primitive::undefinedValue())
+    , m_gatherSources(0)
     , m_runningJob(0)
 {
     qMetaTypeId<Debugger*>();
@@ -181,12 +180,6 @@ void Debugger::resume(Speed speed)
     if (!m_returnedValue.isUndefined())
         m_returnedValue = Encode::undefined();
 
-    clearTemporaryBreakPoints();
-    if (speed == StepOver)
-        setTemporaryBreakPointOnNextLine();
-    if (speed == StepOut)
-        m_temporaryBreakPoints = TemporaryBreakPoint(getFunction(), m_engine->currentContext());
-
     m_stepping = speed;
     m_runningCondition.wakeAll();
 }
@@ -194,20 +187,15 @@ void Debugger::resume(Speed speed)
 void Debugger::addBreakPoint(const QString &fileName, int lineNumber, const QString &condition)
 {
     QMutexLocker locker(&m_lock);
-    if (!m_pendingBreakPointsToRemove.remove(fileName, lineNumber))
-        m_pendingBreakPointsToAdd.add(fileName, lineNumber);
-    m_havePendingBreakPoints = !m_pendingBreakPointsToAdd.isEmpty() || !m_pendingBreakPointsToRemove.isEmpty();
-    if (!condition.isEmpty())
-        m_breakPointConditions.add(fileName, lineNumber, condition);
+    m_breakPoints.insert(DebuggerBreakPoint(fileName, lineNumber), condition);
+    m_haveBreakPoints = true;
 }
 
 void Debugger::removeBreakPoint(const QString &fileName, int lineNumber)
 {
     QMutexLocker locker(&m_lock);
-    if (!m_pendingBreakPointsToAdd.remove(fileName, lineNumber))
-        m_pendingBreakPointsToRemove.add(fileName, lineNumber);
-    m_havePendingBreakPoints = !m_pendingBreakPointsToAdd.isEmpty() || !m_pendingBreakPointsToRemove.isEmpty();
-    m_breakPointConditions.remove(fileName, lineNumber);
+    m_breakPoints.remove(DebuggerBreakPoint(fileName, lineNumber));
+    m_haveBreakPoints = !m_breakPoints.isEmpty();
 }
 
 void Debugger::setBreakOnThrow(bool onoff)
@@ -217,27 +205,17 @@ void Debugger::setBreakOnThrow(bool onoff)
     m_breakOnThrow = onoff;
 }
 
-Debugger::ExecutionState Debugger::currentExecutionState(const uchar *code) const
+Debugger::ExecutionState Debugger::currentExecutionState(int lineNumber) const
 {
-    if (!code)
-        code = m_currentInstructionPointer;
     // ### Locking
     ExecutionState state;
-
     state.function = getFunction();
     state.fileName = state.function->sourceFile();
-
-    qptrdiff relativeProgramCounter = code - state.function->codeData;
-    state.lineNumber = state.function->lineNumberForProgramCounter(relativeProgramCounter);
+    state.lineNumber = lineNumber;
 
     return state;
 }
 
-void Debugger::setPendingBreakpoints(Function *function)
-{
-    m_pendingBreakPointsToAddToFutureCode.applyToFunction(function, /*removeBreakPoints*/ false);
-}
-
 QVector<StackFrame> Debugger::stackTrace(int frameLimit) const
 {
     return m_engine->stackTrace(frameLimit);
@@ -467,23 +445,16 @@ QVector<ExecutionContext::ContextType> Debugger::getScopeTypes(int frame) const
     return types;
 }
 
-void Debugger::maybeBreakAtInstruction(const uchar *code, bool breakPointHit)
+void Debugger::maybeBreakAtInstruction(int lineNumber)
 {
     if (m_runningJob) // do not re-enter when we're doing a job for the debugger.
         return;
 
     QMutexLocker locker(&m_lock);
-    m_currentInstructionPointer = code;
-
-    ExecutionState state = currentExecutionState();
+    if (m_breakPoints.isEmpty())
+        return;
 
-    // Do debugger internal work
-    if (m_havePendingBreakPoints) {
-        if (breakPointHit)
-            breakPointHit = !m_pendingBreakPointsToRemove.contains(state.fileName, state.lineNumber);
-
-        applyPendingBreakPoints();
-    }
+    ExecutionState state = currentExecutionState(lineNumber);
 
     if (m_gatherSources) {
         m_gatherSources->run();
@@ -491,23 +462,23 @@ void Debugger::maybeBreakAtInstruction(const uchar *code, bool breakPointHit)
         m_gatherSources = 0;
     }
 
-    if (m_stopForStepping) {
-        clearTemporaryBreakPoints();
-        m_stopForStepping = false;
-        m_pauseRequested = false;
+    switch (m_stepping) {
+    case StepOver:
+    case StepIn:
         pauseAndWait(Step);
-    } else if (m_pauseRequested) { // Serve debugging requests from the agent
-        m_pauseRequested = false;
-        pauseAndWait(PauseRequest);
-    } else if (breakPointHit) {
-        if (m_stepping == StepOver && m_temporaryBreakPoints.context == m_engine->currentContext())
+        break;
+    case StepOut:
+    case NotStepping:
+        if (m_stopForStepping) { // Serve debugging requests from the agent
+            m_stopForStepping = false;
             pauseAndWait(Step);
-        else if (reallyHitTheBreakPoint(state.fileName, state.lineNumber))
+        } else if (m_pauseRequested) { // Serve debugging requests from the agent
+            m_pauseRequested = false;
+            pauseAndWait(PauseRequest);
+        } else if (reallyHitTheBreakPoint(state.fileName, state.lineNumber)) {
             pauseAndWait(BreakPoint);
+        }
     }
-
-    if (!m_pendingBreakPointsToAdd.isEmpty() || !m_pendingBreakPointsToRemove.isEmpty())
-        applyPendingBreakPoints();
 }
 
 void Debugger::enteringFunction()
@@ -517,7 +488,6 @@ void Debugger::enteringFunction()
     if (m_stepping == StepIn) {
         m_stepping = NotStepping;
         m_stopForStepping = true;
-        m_pauseRequested = true;
     }
 }
 
@@ -527,12 +497,9 @@ void Debugger::leavingFunction(const ReturnedValue &retVal)
 
     QMutexLocker locker(&m_lock);
 
-    if ((m_stepping == StepOut || m_stepping == StepOver)
-            && temporaryBreakPointInFunction(m_engine->currentContext())) {
-        clearTemporaryBreakPoints();
+    if (m_stepping == StepOut || m_stepping == StepOver) {
         m_stepping = NotStepping;
         m_stopForStepping = true;
-        m_pauseRequested = true;
         m_returnedValue = retVal;
     }
 }
@@ -546,7 +513,6 @@ void Debugger::aboutToThrow()
         return;
 
     QMutexLocker locker(&m_lock);
-    clearTemporaryBreakPoints();
     pauseAndWait(Throwing);
 }
 
@@ -584,84 +550,12 @@ void Debugger::pauseAndWait(PauseReason reason)
     m_state = Running;
 }
 
-void Debugger::setTemporaryBreakPointOnNextLine()
-{
-    ExecutionState state = currentExecutionState();
-    Function *function = state.function;
-    if (!function)
-        return;
-
-    QList<qptrdiff> pcs = function->programCountersForAllLines();
-    if (pcs.isEmpty())
-        return;
-
-    m_temporaryBreakPoints = TemporaryBreakPoint(function, m_engine->currentContext());
-    m_temporaryBreakPoints.codeOffsets.reserve(pcs.size());
-    for (QList<qptrdiff>::const_iterator i = pcs.begin(), ei = pcs.end(); i != ei; ++i) {
-        // note: we do set a breakpoint on the current line, because there could be a loop where
-        // a step-over would be jump back to the first instruction making up the current line.
-        qptrdiff offset = *i;
-
-        if (hasBreakOnInstruction(function, offset))
-            continue; // do not set a temporary breakpoint if there already is a breakpoint set by the user
-
-        setBreakOnInstruction(function, offset, true);
-        m_temporaryBreakPoints.codeOffsets.append(offset);
-    }
-}
-
-void Debugger::clearTemporaryBreakPoints()
-{
-    if (m_temporaryBreakPoints.function) {
-        foreach (quintptr offset, m_temporaryBreakPoints.codeOffsets)
-            setBreakOnInstruction(m_temporaryBreakPoints.function, offset, false);
-        m_temporaryBreakPoints = TemporaryBreakPoint();
-    }
-}
-
-bool Debugger::temporaryBreakPointInFunction(ExecutionContext *context) const
-{
-    return m_temporaryBreakPoints.function == getFunction()
-            && m_temporaryBreakPoints.context == context;
-}
-
-void Debugger::applyPendingBreakPoints()
-{
-    foreach (QV4::CompiledData::CompilationUnit *unit, m_engine->compilationUnits) {
-        foreach (Function *function, unit->runtimeFunctions) {
-            m_pendingBreakPointsToAdd.applyToFunction(function, /*removeBreakPoints*/false);
-            m_pendingBreakPointsToRemove.applyToFunction(function, /*removeBreakPoints*/true);
-        }
-    }
-
-    for (BreakPoints::ConstIterator it = m_pendingBreakPointsToAdd.constBegin(),
-         end = m_pendingBreakPointsToAdd.constEnd(); it != end; ++it) {
-        foreach (int lineNumber, it.value())
-            m_pendingBreakPointsToAddToFutureCode.add(it.key(), lineNumber);
-    }
-
-    m_pendingBreakPointsToAdd.clear();
-    m_pendingBreakPointsToRemove.clear();
-    m_havePendingBreakPoints = false;
-}
-
-void Debugger::setBreakOnInstruction(Function *function, qptrdiff codeOffset, bool onoff)
-{
-    uchar *codePtr = const_cast<uchar *>(function->codeData) + codeOffset;
-    Moth::Instr::instr_debug *debug = reinterpret_cast<Moth::Instr::instr_debug *>(codePtr);
-    debug->breakPoint = onoff;
-}
-
-bool Debugger::hasBreakOnInstruction(Function *function, qptrdiff codeOffset)
-{
-    uchar *codePtr = const_cast<uchar *>(function->codeData) + codeOffset;
-    Moth::Instr::instr_debug *debug = reinterpret_cast<Moth::Instr::instr_debug *>(codePtr);
-    return debug->breakPoint;
-}
-
 bool Debugger::reallyHitTheBreakPoint(const QString &filename, int linenr)
 {
-    QString condition = m_breakPointConditions.condition(filename, linenr);
+    BreakPoints::iterator it = m_breakPoints.find(DebuggerBreakPoint(filename, linenr));
+    if (it == m_breakPoints.end())
+        return false;
+    QString condition = it.value();
     if (condition.isEmpty())
         return true;
 
@@ -798,69 +692,6 @@ DebuggerAgent::~DebuggerAgent()
     Q_ASSERT(m_debuggers.isEmpty());
 }
 
-void Debugger::BreakPoints::add(const QString &fileName, int lineNumber)
-{
-    QList<int> &lines = (*this)[fileName];
-    if (!lines.contains(lineNumber)) {
-        lines.append(lineNumber);
-        std::sort(lines.begin(), lines.end());
-    }
-}
-
-bool Debugger::BreakPoints::remove(const QString &fileName, int lineNumber)
-{
-    Iterator breakPoints = find(fileName);
-    if (breakPoints == constEnd())
-        return false;
-    return breakPoints->removeAll(lineNumber) > 0;
-}
-
-bool Debugger::BreakPoints::contains(const QString &fileName, int lineNumber) const
-{
-    ConstIterator breakPoints = find(fileName);
-    if (breakPoints == constEnd())
-        return false;
-    return breakPoints->contains(lineNumber);
-}
-
-void Debugger::BreakPoints::applyToFunction(Function *function, bool removeBreakPoints)
-{
-    Iterator breakPointsForFile = begin();
-
-    while (breakPointsForFile != end()) {
-        if (!function->sourceFile().endsWith(breakPointsForFile.key())) {
-            ++breakPointsForFile;
-            continue;
-        }
-
-        QList<int>::Iterator breakPoint = breakPointsForFile->begin();
-        while (breakPoint != breakPointsForFile->end()) {
-            bool breakPointFound = false;
-            const quint32 *lineNumberMappings = function->compiledFunction->lineNumberMapping();
-            for (quint32 i = 0; i < function->compiledFunction->nLineNumberMappingEntries; ++i) {
-                const int codeOffset = lineNumberMappings[i * 2];
-                const int lineNumber = lineNumberMappings[i * 2 + 1];
-                if (lineNumber == *breakPoint) {
-                    setBreakOnInstruction(function, codeOffset, !removeBreakPoints);
-                    // Continue setting the next break point.
-                    breakPointFound = true;
-                    break;
-                }
-            }
-            if (breakPointFound)
-                breakPoint = breakPointsForFile->erase(breakPoint);
-            else
-                ++breakPoint;
-        }
-
-        if (breakPointsForFile->isEmpty())
-            breakPointsForFile = erase(breakPointsForFile);
-        else
-            ++breakPointsForFile;
-    }
-}
-
-
 Debugger::Collector::~Collector()
 {
 }
diff --git a/src/qml/jsruntime/qv4debugging_p.h b/src/qml/jsruntime/qv4debugging_p.h
index 0e19c51935..aec2cc64ae 100644
--- a/src/qml/jsruntime/qv4debugging_p.h
+++ b/src/qml/jsruntime/qv4debugging_p.h
@@ -69,6 +69,25 @@ enum PauseReason {
 
 class DebuggerAgent;
 
+struct DebuggerBreakPoint {
+    DebuggerBreakPoint(QString fileName, int line)
+        : fileName(fileName), lineNumber(line)
+    {}
+    QString fileName;
+    int lineNumber;
+};
+inline uint qHash(const DebuggerBreakPoint &b, uint seed = 0) Q_DECL_NOTHROW
+{
+    return qHash(b.fileName, seed) ^ b.lineNumber;
+}
+inline bool operator==(const DebuggerBreakPoint &a, const DebuggerBreakPoint &b)
+{
+    return a.lineNumber == b.lineNumber && a.fileName == b.fileName;
+}
+
+typedef QHash<DebuggerBreakPoint, QString> BreakPoints;
+
+
 class Q_QML_EXPORT Debugger
 {
 public:
@@ -114,9 +133,9 @@ public:
 
     enum Speed {
         FullThrottle = 0,
-        StepIn,
         StepOut,
         StepOver,
+        StepIn,
 
         NotStepping = FullThrottle
     };
@@ -150,12 +169,11 @@ public:
         Function *function;
     };
 
-    ExecutionState currentExecutionState(const uchar *code = 0) const;
+    ExecutionState currentExecutionState(int lineNumber) const;
 
     bool pauseAtNextOpportunity() const {
-        return m_pauseRequested || m_havePendingBreakPoints || m_gatherSources;
+        return m_pauseRequested || m_haveBreakPoints || m_gatherSources || m_stepping >= StepOver;
     }
-    void setPendingBreakpoints(Function *function);
 
     QVector<StackFrame> stackTrace(int frameLimit = -1) const;
     void collectArgumentsInContext(Collector *collector, int frameNr = 0, int scopeNr = 0);
@@ -166,7 +184,7 @@ public:
     QVector<ExecutionContext::ContextType> getScopeTypes(int frame = 0) const;
 
 public: // compile-time interface
-    void maybeBreakAtInstruction(const uchar *code, bool breakPointHit);
+    void maybeBreakAtInstruction(int line);
 
 public: // execution hooks
     void enteringFunction();
@@ -178,77 +196,31 @@ private:
 
     // requires lock to be held
     void pauseAndWait(PauseReason reason);
-    // requires lock to be held
-    void setTemporaryBreakPointOnNextLine();
-    // requires lock to be held
-    void clearTemporaryBreakPoints();
-    // requires lock to be held
-    bool temporaryBreakPointInFunction(ExecutionContext *context) const;
 
-    void applyPendingBreakPoints();
-    static void setBreakOnInstruction(Function *function, qptrdiff codeOffset, bool onoff);
-    static bool hasBreakOnInstruction(Function *function, qptrdiff codeOffset);
     bool reallyHitTheBreakPoint(const QString &filename, int linenr);
 
     void runInEngine(Job *job);
     void runInEngine_havingLock(Debugger::Job *job);
 
 private:
-    struct BreakPoints : public QHash<QString, QList<int> >
-    {
-        void add(const QString &fileName, int lineNumber);
-        bool remove(const QString &fileName, int lineNumber);
-        bool contains(const QString &fileName, int lineNumber) const;
-        void applyToFunction(Function *function, bool removeBreakPoints);
-    };
 
     QV4::ExecutionEngine *m_engine;
     DebuggerAgent *m_agent;
     QMutex m_lock;
     QWaitCondition m_runningCondition;
     State m_state;
-    bool m_pauseRequested;
-    Job *m_gatherSources;
-    bool m_havePendingBreakPoints;
-    BreakPoints m_pendingBreakPointsToAdd;
-    BreakPoints m_pendingBreakPointsToAddToFutureCode;
-    BreakPoints m_pendingBreakPointsToRemove;
-    const uchar *m_currentInstructionPointer;
     Speed m_stepping;
+    bool m_pauseRequested;
+    bool m_haveBreakPoints;
     bool m_stopForStepping;
-    QV4::PersistentValue m_returnedValue;
-
-    struct TemporaryBreakPoint {
-        Function *function;
-        QVector<qptrdiff> codeOffsets;
-        ExecutionContext *context;
-        TemporaryBreakPoint(): function(0), context(0) {}
-        TemporaryBreakPoint(Function *function, ExecutionContext *context)
-            : function(function)
-            , context(context)
-        {}
-    } m_temporaryBreakPoints;
-
     bool m_breakOnThrow;
 
+    BreakPoints m_breakPoints;
+    QV4::PersistentValue m_returnedValue;
+
+    Job *m_gatherSources;
     Job *m_runningJob;
     QWaitCondition m_jobIsRunning;
-
-    struct BreakPointConditions: public QHash<QString, QString>
-    {
-        static QString genKey(const QString &fileName, int lineNumber)
-        {
-            return fileName + QLatin1Char(':') + QString::number(lineNumber);
-        }
-
-        QString condition(const QString &fileName, int lineNumber)
-        { return value(genKey(fileName, lineNumber)); }
-        void add(const QString &fileName, int lineNumber, const QString &condition)
-        { insert(genKey(fileName, lineNumber), condition); }
-        void remove(const QString &fileName, int lineNumber)
-        { take(genKey(fileName, lineNumber)); }
-    };
-    BreakPointConditions m_breakPointConditions;
 };
 
 class Q_QML_EXPORT DebuggerAgent : public QObject
diff --git a/src/qml/jsruntime/qv4vme_moth.cpp b/src/qml/jsruntime/qv4vme_moth.cpp
index a657b34be5..fbf32e106a 100644
--- a/src/qml/jsruntime/qv4vme_moth.cpp
+++ b/src/qml/jsruntime/qv4vme_moth.cpp
@@ -662,8 +662,8 @@ QV4::ReturnedValue VME::run(QV4::ExecutionContext *context, const uchar *code
 
     MOTH_BEGIN_INSTR(Debug)
         QV4::Debugging::Debugger *debugger = context->engine->debugger;
-        if (debugger && (instr.breakPoint || debugger->pauseAtNextOpportunity()))
-            debugger->maybeBreakAtInstruction(code, instr.breakPoint);
+        if (debugger && debugger->pauseAtNextOpportunity())
+            debugger->maybeBreakAtInstruction(instr.lineNumber);
     MOTH_END_INSTR(Debug)
 
     MOTH_BEGIN_INSTR(LoadThis)