This addresses the concerns raised by Greg and Jim. Fixed the patch so that
if software breakpoints are supported, we do not fall through to other
breakpoint types. Only if it's not supported, we try the next option.
http://llvm-reviews.chandlerc.com/D2826
CHANGE SINCE LAST DIFF
http://llvm-reviews.chandlerc.com/D2826?vs=7207&id=7278#toc
Files:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2635,19 +2635,13 @@
return false;
}
-
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),
@@ -2656,28 +2650,35 @@
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: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2274,73 +2274,109 @@
}
Error
-ProcessGDBRemote::EnableBreakpointSite (BreakpointSite *bp_site)
+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
@@ -2366,7 +2402,7 @@
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;
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
