Debugger[CDB]: Refactor breakpoint handling.

Add a command to list breakpoints enabling id access.
Implemented breakpoint handling similar to gdb using breakpoint
ids (no longer delete and re-set all breakpoints on a change).
Save the module that is reported back in the session so that
it can be re-used for the next start. Keep a per-debugger-session
cache of fileName->Module for adding breakpoints to accelerate
setting breakpoints in the same file.
Polish the breakpoint tooltip.

src/plugins/debugger/cdb/cdbengine.cpp

@@ -380,7 +380,6 @@ CdbEngine::CdbEngine(const DebuggerStartParameters &sp,
     m_accessible(false),
     m_specialStopMode(NoSpecialStop),
     m_nextCommandToken(0),
-    m_nextBreakpointNumber(1),
     m_currentBuiltinCommandIndex(-1),
     m_extensionCommandPrefixBA("!"QT_CREATOR_CDB_EXT"."),
     m_operateByInstructionPending(true),
@@ -1022,7 +1021,7 @@ void CdbEngine::executeRunToLine(const QString &fileName, int lineNumber)
     BreakpointParameters bp(BreakpointByFileAndLine);
     bp.fileName = fileName;
     bp.lineNumber = lineNumber;
-    postCommand(cdbAddBreakpointCommand(bp, true), 0);
+    postCommand(cdbAddBreakpointCommand(bp, BreakpointId(-1), true), 0);
     continueInferior();
 }
 
@@ -1032,7 +1031,7 @@ void CdbEngine::executeRunToFunction(const QString &functionName)
     BreakpointParameters bp(BreakpointByFunction);
     bp.functionName = functionName;
 
-    postCommand(cdbAddBreakpointCommand(bp, true), 0);
+    postCommand(cdbAddBreakpointCommand(bp, BreakpointId(-1), true), 0);
     continueInferior();
 }
 
@@ -1607,8 +1606,15 @@ unsigned CdbEngine::examineStopReason(const GdbMi &stopReason,
     }
     const int threadId = stopReason.findChild("threadId").data().toInt();
     if (reason == "breakpoint") {
-        const int number = stopReason.findChild("breakpointId").data().toInt();
-        const BreakpointId id = breakHandler()->findBreakpointByNumber(number);
+        // Note: Internal breakpoints (run to line) are reported with id=0.
+        BreakpointId id = 0;
+        int number = 0;
+        const GdbMi breakpointIdG = stopReason.findChild("breakpointId");
+        if (breakpointIdG.isValid()) {
+            id = breakpointIdG.data().toULongLong();
+            if (id)
+                number = breakHandler()->response(id).number;
+        }
         if (id && breakHandler()->type(id) == Watchpoint) {
             *message = msgWatchpointTriggered(id, number, breakHandler()->address(id), QString::number(threadId));
         } else {
@@ -1750,6 +1756,8 @@ void CdbEngine::handleSessionIdle(const QByteArray &messageBA)
             showMessage(QString::fromAscii(stopReason.findChild("threaderror").data()), LogError);
         }
         // Fire off remaining commands asynchronously
+        if (!m_pendingBreakpointMap.isEmpty())
+            postCommandSequence(CommandListBreakPoints);
         if (debuggerCore()->isDockVisible(QLatin1String(Constants::DOCKWIDGET_REGISTER)))
             postCommandSequence(CommandListRegisters);
         if (debuggerCore()->isDockVisible(QLatin1String(Constants::DOCKWIDGET_MODULES)))
@@ -2069,33 +2077,6 @@ static QByteArray multiBreakpointCommand(const char *cmdC, const Breakpoints &bp
 }
 #endif
 
-// Figure out what kind of changes are required to synchronize
-enum BreakPointSyncType {
-    BreakpointsUnchanged, BreakpointsAdded, BreakpointsRemovedChanged
-};
-
-static inline BreakPointSyncType breakPointSyncType(const BreakHandler *handler,
-                                                    const BreakpointIds ids)
-{
-    bool added = false;
-    foreach (BreakpointId id, ids) {
-        const BreakpointState state = handler->state(id);
-        if (debugBreakpoints > 1)
-            qDebug("    Checking on breakpoint %llu, state %d\n", id, state);
-        switch (state) {
-        case BreakpointInsertRequested:
-            added = true;
-            break;
-        case BreakpointChangeRequested:
-        case BreakpointRemoveRequested:
-            return BreakpointsRemovedChanged;
-        default:
-            break;
-        }
-    }
-    return added ? BreakpointsAdded : BreakpointsUnchanged;
-}
-
 bool CdbEngine::stateAcceptsBreakpointChanges() const
 {
     switch (state()) {
@@ -2124,13 +2105,33 @@ void CdbEngine::attemptBreakpointSynchronization()
         if (acceptsBreakpoint(id))
             handler->setEngine(id, this);
 
-    // Find out if there is a need to synchronize again
+    // Quick check: is there a need to change something? - Populate module cache
+    bool changed = false;
     const BreakpointIds ids = handler->engineBreakpointIds(this);
-    const BreakPointSyncType syncType = breakPointSyncType(handler, ids);
+    foreach (BreakpointId id, ids) {
+        switch (handler->state(id)) {
+        case BreakpointInsertRequested:
+        case BreakpointRemoveRequested:
+        case BreakpointChangeRequested:
+            changed = true;
+            break;
+        case BreakpointInserted: {
+            // Collect the new modules matching the files.
+            // In the future, that information should be obtained from the build system.
+            const BreakpointParameters &data = handler->breakpointData(id);
+            if (data.type == BreakpointByFileAndLine && !data.module.isEmpty())
+                m_fileNameModuleHash.insert(data.fileName, data.module);
+        }
+        break;
+        default:
+            break;
+        }
+    }
+
     if (debugBreakpoints)
-        qDebug("attemptBreakpointSynchronizationI %dms accessible=%d, %s %d breakpoints, syncType=%d",
-               elapsedLogTime(), m_accessible, stateName(state()), ids.size(), syncType);
-    if (syncType == BreakpointsUnchanged)
+        qDebug("attemptBreakpointSynchronizationI %dms accessible=%d, %s %d breakpoints, changed=%d",
+               elapsedLogTime(), m_accessible, stateName(state()), ids.size(), changed);
+    if (!changed)
         return;
 
     if (!m_accessible) {
@@ -2139,55 +2140,64 @@ void CdbEngine::attemptBreakpointSynchronization()
             doInterruptInferior(SpecialStopSynchronizeBreakpoints);
         return;
     }
-
-    // If there are changes/removals, delete all breakpoints and re-insert
-    // all enabled breakpoints. This is the simplest
-    // way to apply changes since CDB ids shift when removing breakpoints and there is no
-    // easy way to re-match them.
-
-    if (syncType == BreakpointsRemovedChanged) { // Need to clear out all?
-        postCommand("bc *", 0);
-        m_nextBreakpointNumber = 0;
-    }
-
+    // Add/Change breakpoints and store pending ones in map, since
+    // Breakhandler::setResponse() on pending breakpoints clears the pending flag.
+    // handleBreakPoints will the complete that information and set it on the break handler.
+    bool addedChanged = false;
     foreach (BreakpointId id, ids) {
+        BreakpointParameters parameters = handler->breakpointData(id);
         BreakpointResponse response;
-        const BreakpointParameters &p = handler->breakpointData(id);
-        response.fromParameters(p);
+        response.fromParameters(parameters);
+        // If we encountered that file and have a module for it: Add it.
+        if (parameters.type == BreakpointByFileAndLine && parameters.module.isEmpty()) {
+            const QHash<QString, QString>::const_iterator it = m_fileNameModuleHash.constFind(parameters.fileName);
+            if (it != m_fileNameModuleHash.constEnd())
+                parameters.module = it.value();
+        }
         switch (handler->state(id)) {
         case BreakpointInsertRequested:
-            response.number = m_nextBreakpointNumber++;
-            postCommand(cdbAddBreakpointCommand(p, false, response.number), 0);
+            postCommand(cdbAddBreakpointCommand(parameters, id, false), 0);
+            if (!parameters.enabled)
+                postCommand("bd " + QByteArray::number(id), 0);
             handler->notifyBreakpointInsertProceeding(id);
             handler->notifyBreakpointInsertOk(id);
-            handler->setResponse(id, response);
+            m_pendingBreakpointMap.insert(id, response);
+            addedChanged = true;
+            if (debugBreakpoints)
+                qDebug("Adding %llu %s\n", id, qPrintable(response.toString()));
             break;
         case BreakpointChangeRequested:
-            // Skip disabled breakpoints, else add.
             handler->notifyBreakpointChangeProceeding(id);
-            if (p.enabled) {
-                response.number = m_nextBreakpointNumber++;
-                postCommand(cdbAddBreakpointCommand(p, false, response.number), 0);
-                handler->notifyBreakpointChangeOk(id);
+            if (parameters.enabled != handler->response(id).enabled) {
+                // Change enabled/disabled breakpoints without triggering update.
+                postCommand((parameters.enabled ? "be " : "bd ") + QByteArray::number(id), 0);
+                response.pending = false;
+                response.enabled = parameters.enabled;
                 handler->setResponse(id, response);
+            } else {
+                // Delete and re-add, triggering update
+                addedChanged = true;
+                postCommand("bc " + QByteArray::number(id), 0);
+                postCommand(cdbAddBreakpointCommand(parameters, id, false), 0);
+                m_pendingBreakpointMap.insert(id, response);
             }
+            handler->notifyBreakpointChangeOk(id);
+            if (debugBreakpoints)
+                qDebug("Changing %llu %s\n", id, qPrintable(response.toString()));
             break;
         case BreakpointRemoveRequested:
+            postCommand("bc " + QByteArray::number(id), 0);
             handler->notifyBreakpointRemoveProceeding(id);
             handler->notifyBreakpointRemoveOk(id);
-            break;
-        case BreakpointInserted:
-            // Existing breakpoints were deleted due to change/removal, re-set
-            if (syncType == BreakpointsRemovedChanged) {
-                response.number = m_nextBreakpointNumber++;;
-                postCommand(cdbAddBreakpointCommand(p, false, response.number), 0);
-                handler->setResponse(id, response);
-            }
+            m_pendingBreakpointMap.remove(id);
             break;
         default:
             break;
         }
     }
+    // List breakpoints and send responses
+    if (addedChanged)
+        postCommandSequence(CommandListBreakPoints);
 }
 
 QString CdbEngine::normalizeFileName(const QString &f)
@@ -2308,6 +2318,11 @@ void CdbEngine::postCommandSequence(unsigned mask)
         postExtensionCommand("modules", QByteArray(), 0, &CdbEngine::handleModules, mask & ~CommandListModules);
         return;
     }
+    if (mask & CommandListBreakPoints) {
+        postExtensionCommand("breakpoints", QByteArray("-v"), 0,
+                             &CdbEngine::handleBreakPoints, mask & ~CommandListBreakPoints);
+        return;
+    }
 }
 
 void CdbEngine::handleWidgetAt(const CdbExtensionCommandPtr &reply)
@@ -2342,6 +2357,81 @@ void CdbEngine::handleWidgetAt(const CdbExtensionCommandPtr &reply)
     m_watchPointX = m_watchPointY = 0;
 }
 
+static inline void formatCdbBreakPointResponse(BreakpointId id, const BreakpointResponse &r,
+                                                  QTextStream &str)
+{
+    str << "Obtained breakpoint " << id << " (#" << r.number << ')';
+    if (r.pending) {
+        str << ", pending";
+    } else {
+        str.setIntegerBase(16);
+        str << ", at 0x" << r.address;
+        str.setIntegerBase(10);
+    }
+    if (!r.enabled)
+        str << ", disabled";
+    if (!r.module.isEmpty())
+        str << ", module: '" << r.module << '\'';
+    str << '\n';
+}
+
+void CdbEngine::handleBreakPoints(const CdbExtensionCommandPtr &reply)
+{
+    if (debugBreakpoints)
+        qDebug("CdbEngine::handleBreakPoints: sucess=%d: %s", reply->success, reply->reply.constData());
+    if (!reply->success) {
+        showMessage(QString::fromAscii(reply->errorMessage), LogError);
+        return;
+    }
+    GdbMi value;
+    value.fromString(reply->reply);
+    if (value.type() != GdbMi::List) {
+        showMessage(QString::fromAscii("Unabled to parse breakpoints reply"), LogError);
+        return;
+    }
+    handleBreakPoints(value);
+}
+
+void CdbEngine::handleBreakPoints(const GdbMi &value)
+{
+    // Report all obtained parameters back. Note that not all parameters are reported
+    // back, so, match by id and complete
+    if (debugBreakpoints)
+        qDebug("\nCdbEngine::handleBreakPoints with %d", value.childCount());
+    QString message;
+    QTextStream str(&message);
+    BreakHandler *handler = breakHandler();
+    foreach (const GdbMi &breakPointG, value.children()) {
+        BreakpointResponse reportedResponse;
+        const BreakpointId id = parseBreakPoint(breakPointG, &reportedResponse);
+        if (debugBreakpoints)
+            qDebug("  Parsed %llu: pending=%d %s\n", id, reportedResponse.pending,
+                   qPrintable(reportedResponse.toString()));
+
+        if (!reportedResponse.pending) {
+            const PendingBreakPointMap::iterator it = m_pendingBreakpointMap.find(id);
+            if (it != m_pendingBreakpointMap.end()) {
+                // Complete the response and set on handler.
+                BreakpointResponse &currentResponse = it.value();
+                currentResponse.number = reportedResponse.number;
+                currentResponse.address = reportedResponse.address;
+                currentResponse.module = reportedResponse.module;
+                currentResponse.pending = reportedResponse.pending;
+                currentResponse.enabled = reportedResponse.enabled;
+                formatCdbBreakPointResponse(id, currentResponse, str);
+                handler->setResponse(id, currentResponse);
+                m_pendingBreakpointMap.erase(it);
+            }
+        } // not pending reported
+    } // foreach
+    if (m_pendingBreakpointMap.empty()) {
+        str << QLatin1String("All breakpoints have been resolved.\n");
+    } else {
+        str << QString::fromLatin1("%1 breakpoint(s) pending...\n").arg(m_pendingBreakpointMap.size());
+    }
+    showMessage(message, LogMisc);
+}
+
 void CdbEngine::watchPoint(const QPoint &p)
 {
     m_watchPointX = p.x();

src/plugins/debugger/cdb/cdbengine.h

@@ -35,6 +35,7 @@
 #define DEBUGGER_CDBENGINE_H
 
 #include "debuggerengine.h"
+#include "breakpoint.h"
 
 #include <QtCore/QSharedPointer>
 #include <QtCore/QProcess>
@@ -68,7 +69,8 @@ public:
         CommandListStack = 0x1,
         CommandListThreads = 0x2,
         CommandListRegisters = 0x4,
-        CommandListModules = 0x8
+        CommandListModules = 0x8,
+        CommandListBreakPoints = 0x10
     };
 
     typedef QSharedPointer<CdbBuiltinCommand> CdbBuiltinCommandPtr;
@@ -159,6 +161,8 @@ private slots:
     void consoleStubExited();
 
 private:
+    typedef QHash<BreakpointId, BreakpointResponse> PendingBreakPointMap;
+
     enum SpecialStopMode
     {
         NoSpecialStop,
@@ -205,6 +209,8 @@ private:
     void handleModules(const CdbExtensionCommandPtr &reply);
     void handleMemory(const CdbExtensionCommandPtr &);
     void handleWidgetAt(const CdbExtensionCommandPtr &);
+    void handleBreakPoints(const CdbExtensionCommandPtr &);
+    void handleBreakPoints(const GdbMi &value);
 
     QString normalizeFileName(const QString &f);
     void updateLocalVariable(const QByteArray &iname);
@@ -227,7 +233,6 @@ private:
     bool m_accessible;
     SpecialStopMode m_specialStopMode;
     int m_nextCommandToken;
-    int m_nextBreakpointNumber;
     QList<CdbBuiltinCommandPtr> m_builtinCommandQueue;
     int m_currentBuiltinCommandIndex; // Current command whose output is recorded.
     QList<CdbExtensionCommandPtr> m_extensionCommandQueue;
@@ -244,6 +249,8 @@ private:
     unsigned m_wX86BreakpointCount;
     int m_watchPointX;
     int m_watchPointY;
+    PendingBreakPointMap m_pendingBreakpointMap;
+    QHash<QString, QString> m_fileNameModuleHash;
 };
 
 } // namespace Internal

src/plugins/debugger/cdb/cdbparsehelpers.cpp

@@ -54,7 +54,9 @@ namespace Debugger {
 namespace Internal {
 
 // Convert breakpoint in CDB syntax.
-QByteArray cdbAddBreakpointCommand(const BreakpointParameters &bpIn, bool oneshot, int id)
+QByteArray cdbAddBreakpointCommand(const BreakpointParameters &bpIn,
+                                   BreakpointId id /* = BreakpointId(-1) */,
+                                   bool oneshot)
 {
 #ifdef Q_OS_WIN
     const BreakpointParameters bp = fixWinMSVCBreakpoint(bpIn);
@@ -68,8 +70,10 @@ QByteArray cdbAddBreakpointCommand(const BreakpointParameters &bpIn, bool onesho
     if (bp.threadSpec >= 0)
         str << '~' << bp.threadSpec << ' ';
 
-    str << (bp.type == Watchpoint ? "ba" : "bp");
-    if (id >= 0)
+    // Currently use 'bu' so that the offset expression (including file name)
+    // is kept when reporting back breakpoints (which is otherwise discarded when resolving).
+    str << (bp.type == Watchpoint ? "ba" : "bu");
+    if (id != BreakpointId(-1))
         str << id;
     str << ' ';
     if (oneshot)
@@ -100,7 +104,7 @@ QByteArray cdbAddBreakpointCommand(const BreakpointParameters &bpIn, bool onesho
         break;
     }
     if (bp.ignoreCount)
-        str << ' ' << bp.ignoreCount;
+        str << ' ' << (bp.ignoreCount + 1);
     // Condition currently unsupported.
     return rc;
 }
@@ -181,6 +185,66 @@ static inline bool parseThread(QByteArray line, ThreadData *thread, bool *curren
     return true;
 }
 
+// Helper to retrieve an int child from GDBMI
+static inline bool gdbmiChildToInt(const GdbMi &parent, const char *childName, int *target)
+{
+    const GdbMi childBA = parent.findChild(childName);
+    if (childBA.isValid()) {
+        bool ok;
+        const int v = childBA.data().toInt(&ok);
+        if (ok) {
+            *target = v;
+            return  true;
+        }
+    }
+    return false;
+}
+
+// Helper to retrieve an bool child from GDBMI
+static inline bool gdbmiChildToBool(const GdbMi &parent, const char *childName, bool *target)
+{
+    const GdbMi childBA = parent.findChild(childName);
+    if (childBA.isValid()) {
+        *target = childBA.data() == "true";
+        return true;
+    }
+    return false;
+}
+
+// Parse extension command listing breakpoints.
+// Note that not all fields are returned, since file, line, function are encoded
+// in the expression (that is in addition deleted on resolving for a bp-type breakpoint).
+BreakpointId parseBreakPoint(const GdbMi &gdbmi, BreakpointResponse *r,
+                             QString *expression /*  = 0 */)
+{
+    BreakpointId id = BreakpointId(-1);
+    gdbmiChildToInt(gdbmi, "number", &(r->number));
+    gdbmiChildToBool(gdbmi, "enabled", &(r->enabled));
+    gdbmiChildToBool(gdbmi, "deferred", &(r->pending));
+    const GdbMi idG = gdbmi.findChild("id");
+    if (idG.isValid()) { // Might not be valid if there is not id
+        bool ok;
+        const BreakpointId cid = idG.data().toULongLong(&ok);
+        if (ok)
+            id = cid;
+    }
+    const GdbMi moduleG = gdbmi.findChild("module");
+    if (moduleG.isValid())
+        r->module = QString::fromLocal8Bit(moduleG.data());
+    if (expression) {
+        const GdbMi expressionG = gdbmi.findChild("expression");
+        if (expressionG.isValid())
+            *expression = QString::fromLocal8Bit(expressionG.data());
+    }
+    const GdbMi addressG = gdbmi.findChild("address");
+    if (addressG.isValid())
+        r->address = addressG.data().toULongLong(0, 0);
+    if (gdbmiChildToInt(gdbmi, "passcount", &(r->ignoreCount)))
+        r->ignoreCount--;
+    gdbmiChildToInt(gdbmi, "thread", &(r->threadSpec));
+    return id;
+}
+
 QString debugByteArray(const QByteArray &a)
 {
     QString rc;

src/plugins/debugger/cdb/cdbparsehelpers.h

@@ -34,6 +34,8 @@
 #ifndef CDBPARSEHELPERS_H
 #define CDBPARSEHELPERS_H
 
+#include "breakpoint.h"
+
 #include <QtCore/QtGlobal>
 #include <QtCore/QList>
 #include <QtCore/QVector>
@@ -53,7 +55,11 @@ class Register;
 class GdbMi;
 
 // Convert breakpoint in CDB syntax.
-QByteArray cdbAddBreakpointCommand(const BreakpointParameters &d, bool oneshot = false, int id = -1);
+QByteArray cdbAddBreakpointCommand(const BreakpointParameters &d, BreakpointId id = BreakpointId(-1), bool oneshot = false);
+// Parse extension command listing breakpoints.
+// Note that not all fields are returned, since file, line, function are encoded
+// in the expression (that is in addition deleted on resolving for a bp-type breakpoint).
+BreakpointId parseBreakPoint(const GdbMi &gdbmi, BreakpointResponse *r, QString *expression = 0);
 
 // Convert a CDB integer value: '00000000`0012a290' -> '12a290', '0n10' ->'10'
 QByteArray fixCdbIntegerValue(QByteArray t, bool stripLeadingZeros = false, int *basePtr = 0);