Author: labath Date: Thu Nov 24 04:54:49 2016 New Revision: 287864 URL: http://llvm.org/viewvc/llvm-project?rev=287864&view=rev Log: Introduce chrono to more gdb-remote functions
Summary: This replaces the usage of raw integers with duration classes in the gdb-remote packet management functions. The values are still converted back to integers once they go into the generic Communication class -- that I am leaving to a separate change. The changes are mostly straight-forward (*), the only tricky part was representation of infinite timeouts. Currently, we use UINT32_MAX to denote infinite timeout. This is not well suited for duration classes, as they tend to do arithmetic on the values, and the identity of the MAX value can easily get lost (e.g. microseconds(seconds(UINT32_MAX)).count() != UINT32_MAX). We cannot use zero to represent infinity (as Listener classes do) because we already use it to do non-blocking polling reads. For this reason, I chose to have an explicit value for infinity. The way I achieved that is via llvm::Optional, and I think it reads quite natural. Passing llvm::None as "timeout" means "no timeout", while passing zero means "poll". The only tricky part is this breaks implicit conversions (seconds are implicitly convertible to microseconds, but Optional<seconds> cannot be easily converted into Optional<microseconds>). For this reason I added a special class Timeout, inheriting from Optional, and enabling the necessary conversions one would normally expect. (*) The other tricky part was GDBRemoteCommunication::PopPacketFromQueue, which was needlessly complicated. I've simplified it, but that one is only used in non-stop mode, and so is untested. Reviewers: clayborg, zturner, jingham Subscribers: lldb-commits Differential Revision: https://reviews.llvm.org/D26971 Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp 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/GDBRemoteCommunicationClient.cpp lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp lldb/trunk/tools/lldb-server/lldb-platform.cpp Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp?rev=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp Thu Nov 24 04:54:49 2016 @@ -20,8 +20,9 @@ using namespace lldb; using namespace lldb_private; using namespace lldb_private::process_gdb_remote; +using namespace std::chrono; -static const std::chrono::seconds kInterruptTimeout(5); +static const seconds kInterruptTimeout(5); ///////////////////////// // GDBRemoteClientBase // @@ -51,18 +52,13 @@ StateType GDBRemoteClientBase::SendConti OnRunPacketSent(true); for (;;) { - PacketResult read_result = ReadPacket( - response, - std::chrono::duration_cast<std::chrono::microseconds>(kInterruptTimeout) - .count(), - false); + PacketResult read_result = ReadPacket(response, kInterruptTimeout, false); switch (read_result) { case PacketResult::ErrorReplyTimeout: { std::lock_guard<std::mutex> lock(m_mutex); if (m_async_count == 0) continue; - if (std::chrono::steady_clock::now() >= - m_interrupt_time + kInterruptTimeout) + if (steady_clock::now() >= m_interrupt_time + kInterruptTimeout) return eStateInvalid; } case PacketResult::Success: @@ -188,11 +184,7 @@ GDBRemoteClientBase::SendPacketAndWaitFo const size_t max_response_retries = 3; for (size_t i = 0; i < max_response_retries; ++i) { - packet_result = ReadPacket( - response, std::chrono::duration_cast<std::chrono::microseconds>( - GetPacketTimeout()) - .count(), - true); + packet_result = ReadPacket(response, GetPacketTimeout(), true); // Make sure we received a response if (packet_result != PacketResult::Success) return packet_result; @@ -232,7 +224,7 @@ bool GDBRemoteClientBase::SendvContPacke OnRunPacketSent(true); // wait for the response to the vCont - if (ReadPacket(response, UINT32_MAX, false) == PacketResult::Success) { + if (ReadPacket(response, llvm::None, false) == PacketResult::Success) { if (response.IsOKResponse()) return true; } @@ -254,8 +246,7 @@ bool GDBRemoteClientBase::ShouldStop(con // additional packet to make sure the packet sequence does not get // skewed. StringExtractorGDBRemote extra_stop_reply_packet; - uint32_t timeout_usec = 100000; // 100ms - ReadPacket(extra_stop_reply_packet, timeout_usec, false); + ReadPacket(extra_stop_reply_packet, milliseconds(100), false); // Interrupting is typically done using SIGSTOP or SIGINT, so if // the process stops with some other signal, we definitely want to @@ -268,11 +259,9 @@ bool GDBRemoteClientBase::ShouldStop(con // We probably only stopped to perform some async processing, so continue // after that is done. // TODO: This is not 100% correct, as the process may have been stopped with - // SIGINT or SIGSTOP - // that was not caused by us (e.g. raise(SIGINT)). This will normally cause a - // stop, but if it's - // done concurrently with a async interrupt, that stop will get eaten - // (llvm.org/pr20231). + // SIGINT or SIGSTOP that was not caused by us (e.g. raise(SIGINT)). This will + // normally cause a stop, but if it's done concurrently with a async + // interrupt, that stop will get eaten (llvm.org/pr20231). return false; } @@ -368,7 +357,7 @@ void GDBRemoteClientBase::Lock::SyncWith } if (log) log->PutCString("GDBRemoteClientBase::Lock::Lock sent packet: \\x03"); - m_comm.m_interrupt_time = std::chrono::steady_clock::now(); + m_comm.m_interrupt_time = steady_clock::now(); } m_comm.m_cv.wait(lock, [this] { return m_comm.m_is_running == false; }); m_did_interrupt = true; 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=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Thu Nov 24 04:54:49 2016 @@ -263,11 +263,7 @@ GDBRemoteCommunication::SendPacketNoLock GDBRemoteCommunication::PacketResult GDBRemoteCommunication::GetAck() { StringExtractorGDBRemote packet; - PacketResult result = ReadPacket( - packet, - std::chrono::duration_cast<std::chrono::microseconds>(GetPacketTimeout()) - .count(), - false); + PacketResult result = ReadPacket(packet, GetPacketTimeout(), false); if (result == PacketResult::Success) { if (packet.GetResponseType() == StringExtractorGDBRemote::ResponseType::eAck) @@ -280,13 +276,12 @@ GDBRemoteCommunication::PacketResult GDB GDBRemoteCommunication::PacketResult GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response, - uint32_t timeout_usec, + Timeout<std::micro> timeout, bool sync_on_timeout) { if (m_read_thread_enabled) - return PopPacketFromQueue(response, timeout_usec); + return PopPacketFromQueue(response, timeout); else - return WaitForPacketWithTimeoutMicroSecondsNoLock(response, timeout_usec, - sync_on_timeout); + return WaitForPacketNoLock(response, timeout, sync_on_timeout); } // This function is called when a packet is requested. @@ -295,50 +290,34 @@ GDBRemoteCommunication::ReadPacket(Strin // See GDBRemoteCommunication::AppendBytesToCache. GDBRemoteCommunication::PacketResult GDBRemoteCommunication::PopPacketFromQueue(StringExtractorGDBRemote &response, - uint32_t timeout_usec) { - auto until = std::chrono::system_clock::now() + - std::chrono::microseconds(timeout_usec); - - while (true) { - // scope for the mutex - { - // lock down the packet queue - std::unique_lock<std::mutex> lock(m_packet_queue_mutex); - - // Wait on condition variable. - if (m_packet_queue.size() == 0) { - std::cv_status result = - m_condition_queue_not_empty.wait_until(lock, until); - if (result == std::cv_status::timeout) - break; - } - - if (m_packet_queue.size() > 0) { - // get the front element of the queue - response = m_packet_queue.front(); - - // remove the front element - m_packet_queue.pop(); - - // we got a packet - return PacketResult::Success; - } - } - - // Disconnected + Timeout<std::micro> timeout) { + auto pred = [&] { return !m_packet_queue.empty() && IsConnected(); }; + // lock down the packet queue + std::unique_lock<std::mutex> lock(m_packet_queue_mutex); + + if (!timeout) + m_condition_queue_not_empty.wait(lock, pred); + else { + if (!m_condition_queue_not_empty.wait_for(lock, *timeout, pred)) + return PacketResult::ErrorReplyTimeout; if (!IsConnected()) return PacketResult::ErrorDisconnected; - - // Loop while not timed out } - return PacketResult::ErrorReplyTimeout; + // get the front element of the queue + response = m_packet_queue.front(); + + // remove the front element + m_packet_queue.pop(); + + // we got a packet + return PacketResult::Success; } GDBRemoteCommunication::PacketResult -GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock( - StringExtractorGDBRemote &packet, uint32_t timeout_usec, - bool sync_on_timeout) { +GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet, + Timeout<std::micro> timeout, + bool sync_on_timeout) { uint8_t buffer[8192]; Error error; @@ -354,12 +333,13 @@ GDBRemoteCommunication::WaitForPacketWit while (IsConnected() && !timed_out) { lldb::ConnectionStatus status = eConnectionStatusNoConnection; size_t bytes_read = - Read(buffer, sizeof(buffer), timeout_usec, status, &error); + Read(buffer, sizeof(buffer), timeout ? timeout->count() : UINT32_MAX, + status, &error); if (log) - log->Printf("%s: Read (buffer, (sizeof(buffer), timeout_usec = 0x%x, " + log->Printf("%s: Read (buffer, (sizeof(buffer), timeout = %ld us, " "status = %s, error = %s) => bytes_read = %" PRIu64, - LLVM_PRETTY_FUNCTION, timeout_usec, + LLVM_PRETTY_FUNCTION, long(timeout ? timeout->count() : -1), Communication::ConnectionStatusAsCString(status), error.AsCString(), (uint64_t)bytes_read); @@ -422,8 +402,8 @@ GDBRemoteCommunication::WaitForPacketWit uint32_t successful_responses = 0; for (uint32_t i = 0; i < max_retries; ++i) { StringExtractorGDBRemote echo_response; - echo_packet_result = WaitForPacketWithTimeoutMicroSecondsNoLock( - echo_response, timeout_usec, false); + echo_packet_result = + WaitForPacketNoLock(echo_response, timeout, false); if (echo_packet_result == PacketResult::Success) { ++successful_responses; if (response_regex.Execute(echo_response.GetStringRef())) { 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=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h Thu Nov 24 04:54:49 2016 @@ -53,6 +53,29 @@ enum class CompressionType { class ProcessGDBRemote; +template <typename Ratio> +class Timeout : public llvm::Optional<std::chrono::duration<int64_t, Ratio>> { +private: + template <typename Ratio2> using Dur = std::chrono::duration<int64_t, Ratio2>; + template <typename Ratio2> + using EnableIf = std::enable_if< + std::is_convertible<std::chrono::duration<int64_t, Ratio2>, + std::chrono::duration<int64_t, Ratio>>::value>; + + using Base = llvm::Optional<Dur<Ratio>>; + +public: + Timeout(llvm::NoneType none) : Base(none) {} + Timeout(const Timeout &other) = default; + + template <typename Ratio2, typename = typename EnableIf<Ratio2>::type> + Timeout(const Timeout<Ratio2> &other) + : Base(other ? Base(Dur<Ratio>(*other)) : llvm::None) {} + + template <typename Ratio2, typename = typename EnableIf<Ratio2>::type> + Timeout(const Dur<Ratio2> &other) : Base(Dur<Ratio>(other)) {} +}; + class GDBRemoteCommunication : public Communication { public: enum { @@ -226,16 +249,15 @@ protected: PacketResult SendPacketNoLock(llvm::StringRef payload); PacketResult ReadPacket(StringExtractorGDBRemote &response, - uint32_t timeout_usec, bool sync_on_timeout); + Timeout<std::micro> timeout, bool sync_on_timeout); // Pop a packet from the queue in a thread safe manner PacketResult PopPacketFromQueue(StringExtractorGDBRemote &response, - uint32_t timeout_usec); + Timeout<std::micro> timeout); - PacketResult - WaitForPacketWithTimeoutMicroSecondsNoLock(StringExtractorGDBRemote &response, - uint32_t timeout_usec, - bool sync_on_timeout); + PacketResult WaitForPacketNoLock(StringExtractorGDBRemote &response, + Timeout<std::micro> timeout, + bool sync_on_timeout); bool CompressionIsEnabled() { return m_compression_type != CompressionType::None; Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp?rev=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp Thu Nov 24 04:54:49 2016 @@ -49,6 +49,7 @@ using namespace lldb; using namespace lldb_private; using namespace lldb_private::process_gdb_remote; +using namespace std::chrono; //---------------------------------------------------------------------- // GDBRemoteCommunicationClient constructor @@ -122,9 +123,8 @@ bool GDBRemoteCommunicationClient::Hands // GDB server and flush them all StringExtractorGDBRemote response; PacketResult packet_result = PacketResult::Success; - const uint32_t timeout_usec = 10 * 1000; // Wait for 10 ms for a response while (packet_result == PacketResult::Success) - packet_result = ReadPacket(response, timeout_usec, false); + packet_result = ReadPacket(response, milliseconds(10), false); // The return value from QueryNoAckModeSupported() is true if the packet // was sent and _any_ response (including UNIMPLEMENTED) was received), @@ -202,8 +202,7 @@ bool GDBRemoteCommunicationClient::Query // longer than normal to receive a reply. Wait at least 6 seconds for a // reply to this packet. - ScopedTimeout timeout( - *this, std::max(GetPacketTimeout(), std::chrono::seconds(6))); + ScopedTimeout timeout(*this, std::max(GetPacketTimeout(), seconds(6))); StringExtractorGDBRemote response; if (SendPacketAndWaitForResponse("QStartNoAckMode", response, false) == @@ -315,7 +314,7 @@ void GDBRemoteCommunicationClient::Reset m_hostname.clear(); m_gdb_server_name.clear(); m_gdb_server_version = UINT32_MAX; - m_default_packet_timeout = std::chrono::seconds(0); + m_default_packet_timeout = seconds(0); m_max_packet_size = 0; m_qSupported_response.clear(); m_supported_async_json_packets_is_valid = false; @@ -1200,7 +1199,7 @@ bool GDBRemoteCommunicationClient::GetHo } else if (name.equals("default_packet_timeout")) { uint32_t timeout_seconds; if (!value.getAsInteger(0, timeout_seconds)) { - m_default_packet_timeout = std::chrono::seconds(timeout_seconds); + m_default_packet_timeout = seconds(timeout_seconds); SetPacketTimeout(m_default_packet_timeout); ++num_keys_decoded; } @@ -1329,8 +1328,7 @@ GDBRemoteCommunicationClient::GetHostArc return m_host_arch; } -std::chrono::seconds -GDBRemoteCommunicationClient::GetHostDefaultPacketTimeout() { +seconds GDBRemoteCommunicationClient::GetHostDefaultPacketTimeout() { if (m_qHostInfo_is_valid == eLazyBoolCalculate) GetHostInfo(); return m_default_packet_timeout; @@ -2022,7 +2020,7 @@ uint32_t GDBRemoteCommunicationClient::F StringExtractorGDBRemote response; // Increase timeout as the first qfProcessInfo packet takes a long time // on Android. The value of 1min was arrived at empirically. - ScopedTimeout timeout(*this, std::chrono::seconds(60)); + ScopedTimeout timeout(*this, minutes(1)); if (SendPacketAndWaitForResponse(packet.GetString(), response, false) == PacketResult::Success) { do { @@ -2132,9 +2130,9 @@ static void MakeSpeedTestPacket(StreamSt } } -std::chrono::duration<float> calculate_standard_deviation( - const std::vector<std::chrono::duration<float>> &v) { - using Dur = std::chrono::duration<float>; +duration<float> +calculate_standard_deviation(const std::vector<duration<float>> &v) { + using Dur = duration<float>; Dur sum = std::accumulate(std::begin(v), std::end(v), Dur()); Dur mean = sum / v.size(); float accum = 0; @@ -2151,8 +2149,6 @@ void GDBRemoteCommunicationClient::TestP uint32_t max_recv, uint64_t recv_amount, bool json, Stream &strm) { - using namespace std::chrono; - uint32_t i; if (SendSpeedTestPacket(0, 0)) { StreamString packet; @@ -2324,7 +2320,7 @@ bool GDBRemoteCommunicationClient::Launc } } // give the process a few seconds to startup - ScopedTimeout timeout(*this, std::chrono::seconds(10)); + ScopedTimeout timeout(*this, seconds(10)); if (SendPacketAndWaitForResponse(stream.GetString(), response, false) == PacketResult::Success) { Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp?rev=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp Thu Nov 24 04:54:49 2016 @@ -38,14 +38,11 @@ void GDBRemoteCommunicationServer::Regis } GDBRemoteCommunication::PacketResult -GDBRemoteCommunicationServer::GetPacketAndSendResponse(uint32_t timeout_usec, - Error &error, - bool &interrupt, - bool &quit) { +GDBRemoteCommunicationServer::GetPacketAndSendResponse( + Timeout<std::micro> timeout, Error &error, bool &interrupt, bool &quit) { StringExtractorGDBRemote packet; - PacketResult packet_result = - WaitForPacketWithTimeoutMicroSecondsNoLock(packet, timeout_usec, false); + PacketResult packet_result = WaitForPacketNoLock(packet, timeout, false); if (packet_result == PacketResult::Success) { const StringExtractorGDBRemote::ServerPacketType packet_type = packet.GetServerPacketType(); Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h?rev=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h Thu Nov 24 04:54:49 2016 @@ -43,8 +43,9 @@ public: RegisterPacketHandler(StringExtractorGDBRemote::ServerPacketType packet_type, PacketHandler handler); - PacketResult GetPacketAndSendResponse(uint32_t timeout_usec, Error &error, - bool &interrupt, bool &quit); + PacketResult GetPacketAndSendResponse(Timeout<std::micro> timeout, + Error &error, bool &interrupt, + bool &quit); // After connecting, do a little handshake with the client to make sure // we are at least communicating Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp?rev=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp Thu Nov 24 04:54:49 2016 @@ -926,8 +926,8 @@ void GDBRemoteCommunicationServerLLGS::D bool done = false; Error error; while (true) { - const PacketResult result = - GetPacketAndSendResponse(0, error, interrupt, done); + const PacketResult result = GetPacketAndSendResponse( + std::chrono::microseconds(0), error, interrupt, done); if (result == PacketResult::ErrorReplyTimeout) break; // No more packets in the queue Modified: lldb/trunk/tools/lldb-server/lldb-platform.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-server/lldb-platform.cpp?rev=287864&r1=287863&r2=287864&view=diff ============================================================================== --- lldb/trunk/tools/lldb-server/lldb-platform.cpp (original) +++ lldb/trunk/tools/lldb-server/lldb-platform.cpp Thu Nov 24 04:54:49 2016 @@ -364,7 +364,7 @@ int main_platform(int argc, char *argv[] bool interrupt = false; bool done = false; while (!interrupt && !done) { - if (platform.GetPacketAndSendResponse(UINT32_MAX, error, interrupt, + if (platform.GetPacketAndSendResponse(llvm::None, error, interrupt, done) != GDBRemoteCommunication::PacketResult::Success) break; _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
