[Lldb-commits] [PATCH] Patch for fixing the handling of hardware breakpoints
Greg Clayton
gclayton at apple.com
Wed Feb 19 11:14:59 PST 2014
I agree with the comments Jim made in his email.
I would like to see this changed to:
Try software breakpoints first, see they are not implemented on your hardware breakpoint only system, and then always use hardware breakpoints from there on out even when software was requested. This should solve your problem, and allow systems that support software breakpoints to continue to use them and opt into hardware breakpoint only when desired.
On Feb 19, 2014, at 6:50 AM, Deepak Panickal <deepak2427 at gmail.com> wrote:
> Hardware breakpoints were never being used, unless specifically requested by the user. We have modified it such that a software breakpoint is tried first, if that fails, we try to set a hardware breakpoint and if else, we try to handle the breakpoint from LLDB.
>
> There is now a clean fall-through so that all breakpoint methods are tried. This fixes working with a target with only hardware breakpoint support.
>
> http://llvm-reviews.chandlerc.com/D2826
>
> 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,18 +2650,16 @@
> 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())
> - return response.GetError();
> - }
> - else
> - {
> + // otherwise we assume that the breakpoint could not be set
> switch (type)
> {
> case eBreakpointSoftware: m_supports_z0 = false; break;
> @@ -2676,8 +2668,13 @@
> case eWatchpointRead: m_supports_z3 = false; break;
> case eWatchpointReadWrite: m_supports_z4 = false; break;
> }
> + // empty packet informs us that breakpoint is not supported
> + if (response.IsUnsupportedResponse())
> + // mark unsupported breakpoint as an error, so we can fall through
> + // to another breakpoint type if possible
> + return UINT8_MAX;
> + return response.GetError();
> }
> -
> 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
> @@ -2279,68 +2279,64 @@
> Error error;
> assert (bp_site != NULL);
>
> + // 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);
>
> + // 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);
> return error;
> }
> - else
> - {
> - const size_t bp_op_size = GetSoftwareBreakpointTrapOpcode (bp_site);
>
> - if (bp_site->HardwareRequired())
> + // get the software breakpoint trap opcode size
> + const size_t bp_op_size = GetSoftwareBreakpointTrapOpcode(bp_site);
> +
> + // condition to use software breakpoints (Z0)
> + bool useSW = (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointSoftware));
> + useSW &= !(bp_site->HardwareRequired());
> +
> + // do we use a software breakpoint
> + if (useSW)
> + {
> + if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointSoftware, 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");
> - }
> + bp_site->SetEnabled(true);
> + bp_site->SetType(BreakpointSite::eExternal);
> return error;
> }
> - else if (m_gdb_comm.SupportsGDBStoppointPacket (eBreakpointSoftware))
> - {
> - if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointSoftware, true, addr, bp_op_size) == 0)
> - {
> - bp_site->SetEnabled(true);
> - bp_site->SetType (BreakpointSite::eExternal);
> - return error;
> - }
> - }
> -
> - return EnableSoftwareBreakpoint (bp_site);
> }
>
> - if (log)
> + // condition to use hardware breakpoints (Z1)
> + bool useHW = (bp_site->HardwareRequired());
> + useHW |= !(m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointSoftware));
> + useHW &= (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointHardware));
> +
> + // do we use a hardware breakpoint
> + if (useHW)
> {
> - 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");
> + if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointHardware, true, addr, bp_op_size) == 0)
> + {
> + bp_site->SetEnabled(true);
> + bp_site->SetType(BreakpointSite::eHardware);
> + return error;
> + }
> + // error setting hardware breakpoint
> + error.SetErrorString("failed to set hardware breakpoint (hardware breakpoint resources might be exhausted or unavailable)");
> }
> - // 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. we placing a breakpoint
> + // instruction into the process memory using memory write packets
> + return EnableSoftwareBreakpoint(bp_site);
> }
>
> Error
> @@ -2366,7 +2362,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;
> <D2826.1.patch>_______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
More information about the lldb-commits
mailing list