[Lldb-commits] [PATCH] Patch for fixing the handling of hardware breakpoints
jingham at apple.com
jingham at apple.com
Wed Feb 19 10:03:12 PST 2014
This change bothers me on a couple of counts. First of all, hardware breakpoints are generally a pretty limited resource. So any mechanism that will use them up without the user specifically asking for them doesn't seem like a great idea. For instance, setting a hardware breakpoints will generally succeed even if the memory you are watching isn't currently mapped. So if you set a breakpoint on an address that isn't mapped, the software breakpoint will fail, and you will end up using up a hardware resource for no good reason. Better to have the user explicitly request these.
Secondly, many systems that support the Z packet do so because it is important for the client stub to manage the breakpoints itself. So falling back to trap based breakpoints if the Z packet breakpoint is supported but didn't work doesn't seem like a good idea.
I also don't see why the approach you took "fixes working with a target with only hardware breakpoint support." All you've done is gotten lldb to try a bunch of things that you know are going to fail in that case. Moreover, in such a target the debugger user is going to have to be parsimonious about the breakpoints she sets, so you aren't doing them any favors by not telling them that. If you want to handle this automatically for such targets, it would be better to have a way for the target to say it only supports hardware breakpoints, and in that case, you could force all breakpoints to be hardware for that target.
Jim
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