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

[Chaoren Lin]

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2254-2278
@@ -2253,53 +2253,27 @@
     case 0:
     case TRAP_TRACE:
         // We receive this on single stepping.
-        if (log)
-            log->Printf ("NativeProcessLinux::%s() received trace event, pid = %" PRIu64 " (single stepping)", __FUNCTION__, pid);
-
         if (thread_sp)
         {
-            std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedByTrace ();
-        }
-
-        // This thread is currently stopped.
-        NotifyThreadStop (pid);
-
-        // Here we don't have to request the rest of the threads to stop or request a deferred stop.
-        // This would have already happened at the time the Resume() with step operation was signaled.
-        // At this point, we just need to say we stopped, and the deferred notifcation will fire off
-        // once all running threads have checked in as stopped.
-        SetCurrentThreadID (pid);
-        // Tell the process we have a stop (from software breakpoint).
-        CallAfterRunningThreadsStop (pid,
-                                     [=] (lldb::tid_t signaling_tid)
-                                     {
-                                         SetState (StateType::eStateStopped, true);
-                                     });
-        break;
-
-    case SI_KERNEL:
-    case TRAP_BRKPT:
-        if (log)
-            log->Printf ("NativeProcessLinux::%s() received breakpoint event, pid = %" PRIu64, __FUNCTION__, pid);
-
-        // This thread is currently stopped.
-        NotifyThreadStop (pid);
+            // If a watchpoint was hit, report that instead
+            uint32_t wp_index;
+            thread_sp->GetRegisterContext()->GetWatchpointHitIndex(wp_index);
+            if (wp_index != LLDB_INVALID_INDEX32)
+            {
+                MonitorWatchpoint(pid, thread_sp);
+                break;
+            }
 
-        // Mark the thread as stopped at breakpoint.
-        if (thread_sp)
-        {
-            std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedByBreakpoint ();
-            Error error = FixupBreakpointPCAsNeeded (thread_sp);
-            if (error.Fail ())
+            // If a breakpoint was hit, report that instead
+            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())
             {
-                if (log)
-                    log->Printf ("NativeProcessLinux::%s() pid = %" PRIu64 " fixup: %s", __FUNCTION__, pid, error.AsCString ());
+                MonitorBreakpoint(pid, thread_sp);
+                break;
             }
         }
-        else
-        {
-            if (log)
-                log->Printf ("NativeProcessLinux::%s()  pid = %" PRIu64 ": warning, cannot process software breakpoint since no thread metadata", __FUNCTION__, pid);
-        }
-
+        MonitorTrace(pid, thread_sp);
+        break;
 
----------------
tberghammer wrote:
> The logic for TRAP_TRACE is very similar to the one in NativeThreadLinux::SetStoppedByWatchpoint. Can you merge to common part of the two?
Yeah, I think we can remove SetStoppedByTrace() in NativeThreadLinux::SetStoppedByWatchpoint if we just combine TRAP_TRACE and TRAP_HWBKPT.

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2268-2275
@@ -2294,5 +2267,10 @@
+            // If a breakpoint was hit, report that instead
+            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())
             {
-                if (log)
-                    log->Printf ("NativeProcessLinux::%s() pid = %" PRIu64 " fixup: %s", __FUNCTION__, pid, error.AsCString ());
+                MonitorBreakpoint(pid, thread_sp);
+                break;
             }
         }
----------------
tberghammer wrote:
> What happens if it is a conditional breakpoint where the break condition isn't met? I haven't checked it but I guess the thread will be continued even though it should be in stopped by trace state.
:( Yeah, that does happen. Commenting that out for now. I'll revisit once I figure out how to check the condition there. And add a test case in lang/c/stepping/TestStepAndBreakpoints.py.

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:1085
@@ +1084,3 @@
+    wp_index = LLDB_INVALID_INDEX32;
+    return Error();
+}
----------------
ovyalov wrote:
> Do we need to return failed error here?
I think whether a watchpoint hit is expected or not depends on the calling context. It's conceivable that the caller expects the possiblilty that no watchpoint has been hit. In that case, the caller will need to do some extra checking to determine if it needs to propagate the error.

http://reviews.llvm.org/D8404

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