[Lldb-commits] [PATCH] Report breakpoint/watchpoint hits during single stepping.

[Tamas Berghammer]

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2253
@@ -2252,2 +2252,3 @@
 
     case 0:
+    case TRAP_TRACE:  // We receive this on single stepping.
----------------
Can you add a comment why we have this case here? (I don't know. If you don't know it either then just leave it as it is now)

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2263-2264
@@ -2294,4 +2262,4 @@
             {
-                if (log)
-                    log->Printf ("NativeProcessLinux::%s() pid = %" PRIu64 " fixup: %s", __FUNCTION__, pid, error.AsCString ());
+                MonitorWatchpoint(pid, thread_sp);
+                break;
             }
----------------
Please pass down the watchpoint index to MonitorWatchpoint and then to SetStoppedByWatchpoint so it don't have to look it up again.

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2267-2276
@@ -2305,27 +2266,12 @@
 
-        // We need to tell all other running threads before we notify the delegate about this stop.
-        CallAfterRunningThreadsStop (pid,
-                                     [=](lldb::tid_t deferred_notification_tid)
-                                     {
-                                         SetCurrentThreadID (deferred_notification_tid);
-                                         // Tell the process we have a stop (from software breakpoint).
-                                         SetState (StateType::eStateStopped, true);
-                                     });
-        break;
-
-    case TRAP_HWBKPT:
-        if (log)
-            log->Printf ("NativeProcessLinux::%s() received watchpoint event, pid = %" PRIu64, __FUNCTION__, pid);
-
-        // This thread is currently stopped.
-        NotifyThreadStop (pid);
-
-        // Mark the thread as stopped at watchpoint.
-        // The address is at (lldb::addr_t)info->si_addr if we need it.
-        if (thread_sp)
-            std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedByWatchpoint ();
-        else
-        {
-            if (log)
-                log->Printf ("NativeProcessLinux::%s() pid %" PRIu64 " tid %" PRIu64 ": warning, cannot process hardware breakpoint since no thread metadata", __FUNCTION__, GetID (), pid);
+            // TODO: detect conditional breakpoint
+            // If a breakpoint was hit, report it
+            //addr_t pc = thread_sp->GetRegisterContext()->GetPC(LLDB_INVALID_ADDRESS);
+            //NativeBreakpointSP bp_sp;
+            //m_breakpoint_list.GetBreakpoint(pc, bp_sp);
+            //if (bp_sp && bp_sp->IsEnabled())
+            //{
+            //    MonitorBreakpoint(pid, thread_sp);
+            //    break;
+            //}
         }
----------------
AFAIK it is impossible for LLGS to detect if a conditional breakpoint was hit or not because it don't know anything about the condition. I suggest to remove this comment and just create a bug for it (with possibly a test case) because this part of the code will never fix this issue. I think the proper solution will require lldb to do the check if a breakpoint was hit or not when it receive a stop by trace packet.

We should also consider what is the expected behavior when a watchpoint and a breakpoint is hit with the same instruction. I don't really care about what we display to the user but both the hit count of the breakpoint and the hit count of the watchpoint should be incremented. This behavior can be implemented also if you move the breakpoint detection into lldb.

================
Comment at: source/Plugins/Process/Linux/NativeThreadLinux.cpp:400-402
@@ -400,3 +399,5 @@
     m_stop_description.clear ();
-    for (uint32_t wp_index = 0; wp_index < num_hw_watchpoints; ++wp_index)
+    uint32_t wp_index;
+    Error error = reg_ctx->GetWatchpointHitIndex(wp_index);
+    if (wp_index != LLDB_INVALID_INDEX32)
     {
----------------
Please add an assert and set stop reason to some meaningful default value (e.g.: Invalid) if wp_index == LLDB_INVALID_INDEX32 as this case shouldn't happen

http://reviews.llvm.org/D8404

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/