[Lldb-commits] [lldb] r238323 - Change ProcessGDBRemote last stop packet to a container.
Ryan Brown
ribrdb at google.com
Mon Jun 1 10:38:55 PDT 2015
On Wed, May 27, 2015 at 7:12 AM, Ewan Crawford <ewan at codeplay.com> wrote:
> Author: ewancrawford
> Date: Wed May 27 09:12:34 2015
> New Revision: 238323
>
> URL: http://llvm.org/viewvc/llvm-project?rev=238323&view=rev
> Log:
> Change ProcessGDBRemote last stop packet to a container.
>
> In ProcessGDBRemote we currently have a single packet, m_last_stop_packet,
> used to set the thread stop info.
> However in non-stop mode we can receive several stop reply packets in a
> sequence for different threads. As a result we need to use a container to
> hold them before they are processed.
>
> This patch also changes the return type of CheckPacket() so we can detect
> async notification packets.
>
> Reviewers: clayborg
>
> Subscribers: labath, ted, deepak2427, lldb-commits
>
> Differential Revision: http://reviews.llvm.org/D9853
>
> Modified:
> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
> lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
> lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
>
> Modified:
> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp?rev=238323&r1=238322&r2=238323&view=diff
>
> ==============================================================================
> ---
> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
> (original)
> +++
> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Wed
> May 27 09:12:34 2015
> @@ -329,7 +329,7 @@ GDBRemoteCommunication::WaitForPacketWit
> Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet
> (GDBR_LOG_PACKETS | GDBR_LOG_VERBOSE));
>
> // Check for a packet from our cache first without trying any
> reading...
> - if (CheckForPacket (NULL, 0, packet))
> + if (CheckForPacket(NULL, 0, packet) != PacketType::Invalid)
> return PacketResult::Success;
>
> bool timed_out = false;
> @@ -349,7 +349,7 @@ GDBRemoteCommunication::WaitForPacketWit
>
> if (bytes_read > 0)
> {
> - if (CheckForPacket (buffer, bytes_read, packet))
> + if (CheckForPacket(buffer, bytes_read, packet) !=
> PacketType::Invalid)
> return PacketResult::Success;
> }
> else
> @@ -383,7 +383,7 @@ GDBRemoteCommunication::WaitForPacketWit
> return PacketResult::ErrorReplyFailed;
> }
>
> -bool
> +GDBRemoteCommunication::PacketType
> GDBRemoteCommunication::CheckForPacket (const uint8_t *src, size_t
> src_len, StringExtractorGDBRemote &packet)
> {
> // Put the packet data into the buffer in a thread safe fashion
> @@ -405,6 +405,8 @@ GDBRemoteCommunication::CheckForPacket (
> m_bytes.append ((const char *)src, src_len);
> }
>
> + bool isNotifyPacket = false;
> +
> // Parse up the packets into gdb remote packets
> if (!m_bytes.empty())
> {
> @@ -425,6 +427,9 @@ GDBRemoteCommunication::CheckForPacket (
> break;
>
> case '%': // Async notify packet
> + isNotifyPacket = true;
> + // Intentional fall through
> +
> case '$':
> // Look for a standard gdb packet?
> {
> @@ -487,7 +492,7 @@ GDBRemoteCommunication::CheckForPacket (
> if (content_length == std::string::npos)
> {
> packet.Clear();
> - return false;
> + return GDBRemoteCommunication::PacketType::Invalid;
> }
> else if (total_length > 0)
> {
> @@ -626,11 +631,15 @@ GDBRemoteCommunication::CheckForPacket (
>
> m_bytes.erase(0, total_length);
> packet.SetFilePos(0);
> - return success;
> +
> + if (isNotifyPacket)
> + return GDBRemoteCommunication::PacketType::Notify;
> + else
> + return GDBRemoteCommunication::PacketType::Standard;
> }
> }
> packet.Clear();
> - return false;
> + return GDBRemoteCommunication::PacketType::Invalid;
> }
>
> Error
>
> Modified:
> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
> URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h?rev=238323&r1=238322&r2=238323&view=diff
>
> ==============================================================================
> --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
> (original)
> +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
> Wed May 27 09:12:34 2015
> @@ -49,7 +49,14 @@ public:
> {
> eBroadcastBitRunPacketSent = kLoUserBroadcastBit
> };
> -
> +
> + enum class PacketType
> + {
> + Invalid = 0,
> + Standard,
> + Notify
> + };
> +
> enum class PacketResult
> {
> Success = 0, // Success
> @@ -101,7 +108,7 @@ public:
> bool
> GetSequenceMutex (Mutex::Locker& locker, const char *failure_message
> = NULL);
>
> - bool
> + PacketType
> CheckForPacket (const uint8_t *src,
> size_t src_len,
> StringExtractorGDBRemote &packet);
>
> Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp?rev=238323&r1=238322&r2=238323&view=diff
>
> ==============================================================================
> --- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
> (original)
> +++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Wed
> May 27 09:12:34 2015
> @@ -374,7 +374,6 @@ ProcessGDBRemote::ProcessGDBRemote(Targe
> m_flags (0),
> m_gdb_comm (),
> m_debugserver_pid (LLDB_INVALID_PROCESS_ID),
> - m_last_stop_packet (),
> m_last_stop_packet_mutex (Mutex::eMutexTypeNormal),
> m_register_info (),
> m_async_broadcaster (NULL,
> "lldb.process.gdb-remote.async-broadcaster"),
> @@ -769,8 +768,10 @@ ProcessGDBRemote::DoConnectRemote (Strea
> // We have a valid process
> SetID (pid);
> GetThreadList();
> - if (m_gdb_comm.GetStopReply(m_last_stop_packet))
> + StringExtractorGDBRemote response;
> + if (m_gdb_comm.GetStopReply(response))
> {
> + SetLastStopPacket(response);
>
> // '?' Packets must be handled differently in non-stop mode
> if (GetTarget().GetNonStopModeEnabled())
> @@ -788,7 +789,7 @@ ProcessGDBRemote::DoConnectRemote (Strea
> }
> }
>
> - const StateType state = SetThreadStopInfo
> (m_last_stop_packet);
> + const StateType state = SetThreadStopInfo (response);
> if (state == eStateStopped)
> {
> SetPrivateState (state);
> @@ -1057,9 +1058,10 @@ ProcessGDBRemote::DoLaunch (Module *exe_
> return error;
> }
>
> - if (m_gdb_comm.GetStopReply(m_last_stop_packet))
> + StringExtractorGDBRemote response;
> + if (m_gdb_comm.GetStopReply(response))
> {
> -
> + SetLastStopPacket(response);
>
Why call SetLastStopPacket if you're calling SetThreadStopInfo()?
> // '?' Packets must be handled differently in non-stop
> mode
> if (GetTarget().GetNonStopModeEnabled())
> HandleStopReplySequence();
> @@ -1077,7 +1079,7 @@ ProcessGDBRemote::DoLaunch (Module *exe_
> m_target.MergeArchitecture(host_arch);
> }
>
> - SetPrivateState (SetThreadStopInfo (m_last_stop_packet));
> + SetPrivateState (SetThreadStopInfo (response));
>
> if (!disable_stdio)
> {
> @@ -2123,7 +2125,25 @@ ProcessGDBRemote::RefreshStateAfterStop
> // Set the thread stop info. It might have a "threads" key whose
> value is
> // a list of all thread IDs in the current process, so m_thread_ids
> might
> // get set.
> - SetThreadStopInfo (m_last_stop_packet);
> +
> + // Scope for the lock
> + {
> + // Lock the thread stack while we access it
> + Mutex::Locker stop_stack_lock(m_last_stop_packet_mutex);
> + // Get the number of stop packets on the stack
> + int nItems = m_stop_packet_stack.size();
> + // Iterate over them
> + for (int i = 0; i < nItems; i++)
> + {
> + // Get the thread stop info
> + StringExtractorGDBRemote stop_info = m_stop_packet_stack[i];
> + // Process thread stop info
> + SetThreadStopInfo(stop_info);
>
This doesn't seem right.
SetThreadStopInfo needs an up to date threadlist for
m_thread_list.FindThreadByProtocolID().
If these packets contain threads, only the first will be processed with the
correct threads because UpdateThreadListIfNeeded only checks the stop id.
So it seems we should either be bumping the stop id for each packet, or
calling UpdateThreadList directly before using m_thread_list.
Also if these packets don't have the thread list you're in trouble because
UpdateThreadListIfNeeded will get the current list, not the one from the
time of the stop.
> + }
> + // Clear the thread stop stack
> + m_stop_packet_stack.clear();
> + }
> +
> // Check to see if SetThreadStopInfo() filled in m_thread_ids?
> if (m_thread_ids.empty())
> {
> @@ -2395,7 +2415,6 @@ ProcessGDBRemote::DoDestroy ()
> void
> ProcessGDBRemote::SetLastStopPacket (const StringExtractorGDBRemote
> &response)
> {
> - Mutex::Locker locker (m_last_stop_packet_mutex);
> const bool did_exec = response.GetStringRef().find(";reason:exec;")
> != std::string::npos;
> if (did_exec)
> {
> @@ -2408,7 +2427,16 @@ ProcessGDBRemote::SetLastStopPacket (con
> BuildDynamicRegisterInfo (true);
> m_gdb_comm.ResetDiscoverableSettings();
> }
> - m_last_stop_packet = response;
> +
> + // Scope the lock
> + {
> + // Lock the thread stack while we access it
> + Mutex::Locker stop_stack_lock(m_last_stop_packet_mutex);
> + // Add this stop packet to the stop packet stack
> + // This stack will get popped and examined when we switch to the
> + // Stopped state
> + m_stop_packet_stack.push_back(response);
> + }
> }
>
>
>
> Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
> URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h?rev=238323&r1=238322&r2=238323&view=diff
>
> ==============================================================================
> --- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
> (original)
> +++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h Wed
> May 27 09:12:34 2015
> @@ -333,7 +333,7 @@ protected:
> Flags m_flags; // Process specific flags (see eFlags enums)
> GDBRemoteCommunicationClient m_gdb_comm;
> std::atomic<lldb::pid_t> m_debugserver_pid;
> - StringExtractorGDBRemote m_last_stop_packet;
> + std::vector<StringExtractorGDBRemote> m_stop_packet_stack; // The
> stop packet stack replaces the last stop packet variable
> Mutex m_last_stop_packet_mutex;
> GDBRemoteDynamicRegisterInfo m_register_info;
> Broadcaster m_async_broadcaster;
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150601/0d649806/attachment.html>
More information about the lldb-commits
mailing list