Report stopped by trace if none of the watchpoint was hit

[tberghammer]

Summary

Report stopped by trace if none of the watchpoint was hit

Some linux kernel reports a watchpoint hit after single stepping even when no watchpoint was hit. This CL looks for a watchpoint which was hit and reports a stop by trace if it haven't found any.

[jingham]

How will you handle the case where the instruction we are stepping over triggers a real watchpoint?

[vharron]

How will you handle the case where the instruction we are stepping over triggers a real watchpoint?

Thanks for the suggestion:

"Create a test that verifies correct behavior when a watchpoint is hit during a step"
http://llvm.org/bugs/show_bug.cgi?id=22814

[jingham]

I didn't understand your reply. Adding a test case won't make this code work correctly.

This change should not go in till you figure out how to identify the watchpoint stop reason which corresponds to the return from the single step and only return true in that case.

[vharron]

Adding a test case won't make this code work correctly.

Sorry, that was confusing. I don't have a great solution at this point. The kernel is what the kernel is and it's on shipped Android systems, so we have to live with it. Hopefully that bug will eventually drive a kernel patch.

This change should not go in till you figure out how to identify the watchpoint stop reason which corresponds to the return from the single step and only return true in that case.

I see your point. Tamas, I've been assuming that this is related to your work on aarch64. Is that correct? If so, do you want to try to patch the kernel, ask Linaro for help or punt this whole problem to them?

[jingham]

Do you get any data back from the watchpoint hit? For instance, if the trace is implemented by a "pc != curr_pc" type watch, then you should be able to get the watched address from the watchpoint hit event. In that case, you can check whether the watched address was the current PC, and only report this as a trace stop when that is true.

Another way to do this, if you are using llgs, is to have THAT guy convert the watchpoint hit into a trace event. llgs is closer to the metal, and might have a better chance to figure out what just went on.

[jasonmolenda]

I'm no expert at this layer but you might want to have your kernel people look at the "ARMv8 Debug Architecture" manual from ARM, specifically section 4.2.1, "Effect of taking debug exception to an EL using AArch64 on system registers" (or the AArch32 section below that). It should be possible for the kernel to distinguish between a single step with the MDSCR_EL1 SS bit and a watchpoint debug event.

[tberghammer]

Thanks for noticing this bug and for all of the suggestions. I created a new fix what address the actual root cause of the problem with reporting the right stop reason from lldb-server.

For ARMv8 the watchpoint handling is not implemented yet, but it should be possible to implement the IsWatchpointHit function.

[jingham]

The approach looks right. I'll let somebody who works on the NativeThread side weigh in on whether the implementation is correct.

[tberghammer]

Friendly ping

[vharron]

source/Plugins/Process/Linux/NativeThreadLinux.cpp

406 ↗

(On Diff #21343)

Which kernels is it known to happen on/not happen on?

[chaoren]

LGTM

Some nits below:

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h

49 ↗

(On Diff #21343)

Is this necessary?

source/Plugins/Process/Linux/NativeThreadLinux.cpp

405 ↗

(On Diff #21343)

What Vince asked (which kernels). Also nit: grammar.

[tberghammer]

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h

49 ↗

(On Diff #21343)

Which part?

The override keyword is optional but gives us some more compile time check.

[tberghammer]

source/Plugins/Process/Linux/NativeThreadLinux.cpp

406 ↗

(On Diff #21343)

Added kernel description to comment (android-arm64 platfrom-21) and fixed grammar

[labath]

It could be that the kernel reports a watchpoint hit on single stepping because on arm single stepping is actually implemented using hardware breakpoints. This is just a guess, but in any case it might be interesting to create a test case, which what happens with single stepping after we have set the maximum number of watchpoints possible. Does single stepping still work? Do some of our watchpoints get overwritten, etc.

[jasonmolenda]

On armv7 architecture systems, there are 4 hardware breakpoint registers (DBGBVR) and 4 hardware watchpoint registers (DBGWVR). The breakpoint registers track values in the pc register. You can also say "stop when the PC is not equal to this value" which is how single instruction stepping is done on armv7.

On armv8 architecture systems, AArch32 or AArch64, there is a single instruction step bit (MDSCR_EL1, SS bit) to accomplish this more simply. There are still the dual sets of hardware breakpoint and hardware watchpoint registers -- but the hardware breakpoint registers don't have the "stop when the PC is not equal to this value" capability.

The kernel should have no problem distinguishing between an MDSCR_EL1 SS bit stop event and a watchpoint exception (on armv8 systems). On an armv7 system, it's possible to distinguish between a breakpoint and watchpoint event. And exhausting the watchpoint register file won't prevent stepping from working.

fwiw, lldb today doesn't use the hardware breakpoints, except for single stepping on armv7 systems. breakpoint set does have the --hardware option to request that the remote gdb stub use a hardware breakpoint if it has that capability. But lldb doesn't do that on its own today.

[labath]

Thank you, that was very enlightening. :)

lldb/trunk/include/lldb/Host/common/NativeRegisterContext.h

public:
     SetHardwareWatchpoint (lldb::addr_t addr, size_t size, uint32_t watch_flags);
 
     virtual bool
     ClearHardwareWatchpoint (uint32_t hw_index);
 
     virtual Error
     ClearAllHardwareWatchpoints ();
 
+    virtual Error
+    IsWatchpointHit (uint8_t wp_index);
+
+    virtual Error
+    IsWatchpointVacant (uint32_t wp_index);
+
+    virtual lldb::addr_t
+    GetWatchpointAddress (uint32_t wp_index);
+
     virtual bool
     HardwareSingleStep (bool enable);
 
     virtual Error
     ReadRegisterValueFromMemory (const lldb_private::RegisterInfo *reg_info, lldb::addr_t src_addr, lldb::addr_t src_len, RegisterValue &reg_value);
 
     virtual Error
     WriteRegisterValueToMemory (const lldb_private::RegisterInfo *reg_info, lldb::addr_t dst_addr, lldb::addr_t dst_len, const RegisterValue &reg_value);

lldb/trunk/source/Host/common/NativeRegisterContext.cpp

 }
 
 Error
 NativeRegisterContext::ClearAllHardwareWatchpoints ()
 {
     return Error ("not implemented");
 }
 
+Error
+NativeRegisterContext::IsWatchpointHit (uint8_t wp_index)
+{
+    return Error ("not implemented");
+}
+
+Error
+NativeRegisterContext::IsWatchpointVacant (uint32_t wp_index)
+{
+    return Error ("not implemented");
+}
+
+lldb::addr_t
+NativeRegisterContext::GetWatchpointAddress (uint32_t wp_index)
+{
+    return LLDB_INVALID_ADDRESS;
+}
+
 bool
 NativeRegisterContext::HardwareSingleStep (bool enable)
 {
     return false;
 }
 
 Error
 NativeRegisterContext::ReadRegisterValueFromMemory (

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h

public:
 
         Error
         ReadAllRegisterValues (lldb::DataBufferSP &data_sp) override;
 
         Error
         WriteAllRegisterValues (const lldb::DataBufferSP &data_sp) override;
 
         Error
-        IsWatchpointHit(uint8_t wp_index);
+        IsWatchpointHit(uint8_t wp_index) override;
 
         Error
-        IsWatchpointVacant(uint32_t wp_index);
+        IsWatchpointVacant(uint32_t wp_index) override;
 
         bool
         ClearHardwareWatchpoint(uint32_t wp_index) override;
 
         Error
         ClearAllHardwareWatchpoints () override;
 
         Error
         SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
                 uint32_t watch_flags, uint32_t wp_index);
 
         uint32_t
         SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
                 uint32_t watch_flags) override;
 
         lldb::addr_t
-        GetWatchpointAddress(uint32_t wp_index);
+        GetWatchpointAddress(uint32_t wp_index) override;
 
         uint32_t
         NumSupportedHardwareWatchpoints() override;
 
     private:
 
         // Private member types.
         enum FPRType

lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp

NativeThreadLinux::SetStoppedByBreakpoint ()
     m_stop_info.reason = StopReason::eStopReasonBreakpoint;
     m_stop_info.details.signal.signo = SIGTRAP;
     m_stop_description.clear();
 }
 
 void
 NativeThreadLinux::SetStoppedByWatchpoint ()
 {
+    Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
+    lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
+    if (log)
+    {
+        NativeProcessProtocolSP process_sp = m_process_wp.lock ();
+        if (process_sp)
+            pid = process_sp->GetID ();
+    }
+
     const StateType new_state = StateType::eStateStopped;
     MaybeLogStateChange (new_state);
     m_state = new_state;
 
-    m_stop_info.reason = StopReason::eStopReasonWatchpoint;
-    m_stop_info.details.signal.signo = SIGTRAP;
-
-    NativeRegisterContextLinux_x86_64 *reg_ctx =
-        reinterpret_cast<NativeRegisterContextLinux_x86_64*> (GetRegisterContext().get());
-    const uint32_t num_hw_watchpoints =
-        reg_ctx->NumSupportedHardwareWatchpoints();
+    NativeRegisterContextSP reg_ctx = GetRegisterContext ();
+    const uint32_t num_hw_watchpoints = reg_ctx->NumSupportedHardwareWatchpoints ();
 
     m_stop_description.clear ();
     for (uint32_t wp_index = 0; wp_index < num_hw_watchpoints; ++wp_index)
-        if (reg_ctx->IsWatchpointHit(wp_index).Success())
+    {
+        if (reg_ctx->IsWatchpointHit (wp_index).Success())
         {
+            if (log)
+                log->Printf ("NativeThreadLinux:%s (pid=%" PRIu64 ", tid=%" PRIu64 ") watchpoint found with idx: %u",
+                             __FUNCTION__, pid, GetID (), wp_index);
+
             std::ostringstream ostr;
-            ostr << reg_ctx->GetWatchpointAddress(wp_index) << " " << wp_index;
+            ostr << reg_ctx->GetWatchpointAddress (wp_index) << " " << wp_index;
             m_stop_description = ostr.str();
+
+            m_stop_info.reason = StopReason::eStopReasonWatchpoint;
+            m_stop_info.details.signal.signo = SIGTRAP;
             return;
         }
-    Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
-    if (log)
-    {
-        NativeProcessProtocolSP m_process_sp = m_process_wp.lock ();
-        lldb::pid_t pid = m_process_sp ? m_process_sp->GetID () : LLDB_INVALID_PROCESS_ID;
-        log->Printf ("NativeThreadLinux: thread (pid=%" PRIu64 ", tid=%" PRIu64 ") "
-                "stopped by a watchpoint, but failed to find it",
-                pid, GetID ());
     }
+
+    // The process reported a watchpoint was hit, but we haven't found the
+    // watchpoint. Assume that a stopped by trace is reported as a hardware
+    // watchpoint what happens on some linux kernels (e.g.: android-arm64
+    // platfrom-21).
+
+    if (log)
+        log->Printf ("NativeThreadLinux:%s (pid=%" PRIu64 ", tid=%" PRIu64 ") none of the watchpoint was hit.",
+                     __FUNCTION__, pid, GetID ());
+
+    SetStoppedByTrace ();
 }
 
 bool
 NativeThreadLinux::IsStoppedAtBreakpoint ()
 {
     return GetState () == StateType::eStateStopped &&
         m_stop_info.reason == StopReason::eStopReasonBreakpoint;
 }