Patch for fixing the handling of hardware breakpoints.
authorDeepak Panickal <deepak@codeplay.com>
Mon, 24 Feb 2014 11:50:46 +0000 (11:50 +0000)
committerDeepak Panickal <deepak@codeplay.com>
Mon, 24 Feb 2014 11:50:46 +0000 (11:50 +0000)
Differential Revision: http://llvm-reviews.chandlerc.com/D2826

llvm-svn: 202028

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

index ff12b65..ab3bf7f 100644 (file)
@@ -2675,15 +2675,10 @@ GDBRemoteCommunicationClient::GetThreadStopInfo (lldb::tid_t tid, StringExtracto
 uint8_t
 GDBRemoteCommunicationClient::SendGDBStoppointTypePacket (GDBStoppointType type, bool insert,  addr_t addr, uint32_t length)
 {
-    switch (type)
-    {
-    case eBreakpointSoftware:   if (!m_supports_z0) return UINT8_MAX; break;
-    case eBreakpointHardware:   if (!m_supports_z1) return UINT8_MAX; break;
-    case eWatchpointWrite:      if (!m_supports_z2) return UINT8_MAX; break;
-    case eWatchpointRead:       if (!m_supports_z3) return UINT8_MAX; break;
-    case eWatchpointReadWrite:  if (!m_supports_z4) return UINT8_MAX; break;
-    }
-
+    // Check if the stub is known not to support this breakpoint type
+    if (!SupportsGDBStoppointPacket(type))
+        return UINT8_MAX;
+    // Construct the breakpoint packet
     char packet[64];
     const int packet_len = ::snprintf (packet, 
                                        sizeof(packet), 
@@ -2692,28 +2687,35 @@ GDBRemoteCommunicationClient::SendGDBStoppointTypePacket (GDBStoppointType type,
                                        type, 
                                        addr, 
                                        length);
-
+    // Check we havent overwritten the end of the packet buffer
     assert (packet_len + 1 < (int)sizeof(packet));
     StringExtractorGDBRemote response;
+    // Try to send the breakpoint packet, and check that it was correctly sent
     if (SendPacketAndWaitForResponse(packet, packet_len, response, true) == PacketResult::Success)
     {
+        // Receive and OK packet when the breakpoint successfully placed
         if (response.IsOKResponse())
             return 0;
-        else if (response.IsErrorResponse())
+
+        // Error while setting breakpoint, send back specific error
+        if (response.IsErrorResponse())
             return response.GetError();
-    }
-    else
-    {
-        switch (type)
+
+        // Empty packet informs us that breakpoint is not supported
+        if (response.IsUnsupportedResponse())
         {
+            // Disable this breakpoint type since it is unsupported
+            switch (type)
+            {
             case eBreakpointSoftware:   m_supports_z0 = false; break;
             case eBreakpointHardware:   m_supports_z1 = false; break;
             case eWatchpointWrite:      m_supports_z2 = false; break;
             case eWatchpointRead:       m_supports_z3 = false; break;
             case eWatchpointReadWrite:  m_supports_z4 = false; break;
+            }
         }
     }
-
+    // Signal generic faliure
     return UINT8_MAX;
 }
 
index 96701bd..1a73f6a 100644 (file)
@@ -2310,70 +2310,106 @@ Error
 ProcessGDBRemote::EnableBreakpointSite (BreakpointSite *bp_site)
 {
     Error error;
-    assert (bp_site != NULL);
+    assert(bp_site != NULL);
 
-    Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_BREAKPOINTS));
+    // Get logging info
+    Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_BREAKPOINTS));
     user_id_t site_id = bp_site->GetID();
+
+    // Get the breakpoint address
     const addr_t addr = bp_site->GetLoadAddress();
+
+    // Log that a breakpoint was requested
     if (log)
-        log->Printf ("ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") address = 0x%" PRIx64, site_id, (uint64_t)addr);
+        log->Printf("ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") address = 0x%" PRIx64, site_id, (uint64_t)addr);
 
+    // Breakpoint already exists and is enabled
     if (bp_site->IsEnabled())
     {
         if (log)
-            log->Printf ("ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") address = 0x%" PRIx64 " -- SUCCESS (already enabled)", site_id, (uint64_t)addr);
+            log->Printf("ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") address = 0x%" PRIx64 " -- SUCCESS (already enabled)", site_id, (uint64_t)addr);
         return error;
     }
-    else
+
+    // Get the software breakpoint trap opcode size
+    const size_t bp_op_size = GetSoftwareBreakpointTrapOpcode(bp_site);
+
+    // SupportsGDBStoppointPacket() simply checks a boolean, indicating if this breakpoint type
+    // is supported by the remote stub. These are set to true by default, and later set to false
+    // only after we receive an unimplemented response when sending a breakpoint packet. This means
+    // initially that unless we were specifically instructed to use a hardware breakpoint, LLDB will
+    // attempt to set a software breakpoint. HardwareRequired() also queries a boolean variable which
+    // indicates if the user specifically asked for hardware breakpoints.  If true then we will
+    // skip over software breakpoints.
+    if (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointSoftware) && (!bp_site->HardwareRequired()))
     {
-        const size_t bp_op_size = GetSoftwareBreakpointTrapOpcode (bp_site);
+        // Try to send off a software breakpoint packet ($Z0)
+        if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointSoftware, true, addr, bp_op_size) == 0)
+        {
+            // The breakpoint was placed successfully
+            bp_site->SetEnabled(true);
+            bp_site->SetType(BreakpointSite::eExternal);
+            return error;
+        }
 
-        if (bp_site->HardwareRequired())
+        // SendGDBStoppointTypePacket() will return an error if it was unable to set this
+        // breakpoint. We need to differentiate between a error specific to placing this breakpoint
+        // or if we have learned that this breakpoint type is unsupported. To do this, we
+        // must test the support boolean for this breakpoint type to see if it now indicates that
+        // this breakpoint type is unsupported.  If they are still supported then we should return
+        // with the error code.  If they are now unsupported, then we would like to fall through
+        // and try another form of breakpoint.
+        if (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointSoftware))
+            return error;
+
+        // We reach here when software breakpoints have been found to be unsupported. For future
+        // calls to set a breakpoint, we will not attempt to set a breakpoint with a type that is
+        // known not to be supported.
+        if (log)
+            log->Printf("Software breakpoints are unsupported");
+
+        // So we will fall through and try a hardware breakpoint
+    }
+
+    // The process of setting a hardware breakpoint is much the same as above.  We check the
+    // supported boolean for this breakpoint type, and if it is thought to be supported then we
+    // will try to set this breakpoint with a hardware breakpoint.
+    if (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointHardware))
+    {
+        // Try to send off a hardware breakpoint packet ($Z1)
+        if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointHardware, true, addr, bp_op_size) == 0)
         {
-            // Try and set hardware breakpoint, and if that fails, fall through
-            // and set a software breakpoint?
-            if (m_gdb_comm.SupportsGDBStoppointPacket (eBreakpointHardware))
-            {
-                if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointHardware, true, addr, bp_op_size) == 0)
-                {
-                    bp_site->SetEnabled(true);
-                    bp_site->SetType (BreakpointSite::eHardware);
-                }
-                else
-                {
-                    error.SetErrorString("failed to set hardware breakpoint (hardware breakpoint resources might be exhausted or unavailable)");
-                }
-            }
-            else
-            {
-                error.SetErrorString("hardware breakpoints are not supported");
-            }
+            // The breakpoint was placed successfully
+            bp_site->SetEnabled(true);
+            bp_site->SetType(BreakpointSite::eHardware);
             return error;
         }
-        else if (m_gdb_comm.SupportsGDBStoppointPacket (eBreakpointSoftware))
+
+        // Check if the error was something other then an unsupported breakpoint type
+        if (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointHardware))
         {
-            if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointSoftware, true, addr, bp_op_size) == 0)
-            {
-                bp_site->SetEnabled(true);
-                bp_site->SetType (BreakpointSite::eExternal);
-                return error;
-            }
+            // Unable to set this hardware breakpoint
+            error.SetErrorString("failed to set hardware breakpoint (hardware breakpoint resources might be exhausted or unavailable)");
+            return error;
         }
 
-        return EnableSoftwareBreakpoint (bp_site);
+        // We will reach here when the stub gives an unsported response to a hardware breakpoint
+        if (log)
+            log->Printf("Hardware breakpoints are unsupported");
+
+        // Finally we will falling through to a #trap style breakpoint
     }
 
-    if (log)
+    // Don't fall through when hardware breakpoints were specifically requested
+    if (bp_site->HardwareRequired())
     {
-        const char *err_string = error.AsCString();
-        log->Printf ("ProcessGDBRemote::EnableBreakpointSite () error for breakpoint at 0x%8.8" PRIx64 ": %s",
-                     bp_site->GetLoadAddress(),
-                     err_string ? err_string : "NULL");
+        error.SetErrorString("hardware breakpoints are not supported");
+        return error;
     }
-    // We shouldn't reach here on a successful breakpoint enable...
-    if (error.Success())
-        error.SetErrorToGenericError();
-    return error;
+
+    // As a last resort we want to place a manual breakpoint. An instruction
+    // is placed into the process memory using memory write packets.
+    return EnableSoftwareBreakpoint(bp_site);
 }
 
 Error
@@ -2399,7 +2435,7 @@ ProcessGDBRemote::DisableBreakpointSite (BreakpointSite *bp_site)
             break;
 
         case BreakpointSite::eHardware:
-            if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointSoftware, false, addr, bp_op_size))
+            if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointHardware, false, addr, bp_op_size))
                 error.SetErrorToGenericError();
             break;