From a70512a9587c40e66035c53c170e8d44c8d2e911 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Wed, 11 Apr 2018 13:30:54 +0000 Subject: [PATCH] llgs: Send "rich" errors in response to vAttach packets There are plenty of ways attaching can go wrong. Having the server report the exact error means we can give better feedback to the user. (This patch does not do the second part, it only makes sure the information is sent from the server.) Triggering all possible error conditions in a test would prove challenging, but there is one error that is very easy to reproduce (attempting to attach while debugging), so I write a test based on that. The test immediately exposed a bug where the m_send_error_strings field was being used uninitialized (so it was sometimes true from the get-go), so I fix that as well. llvm-svn: 329803 --- .../gdb-remote/GDBRemoteCommunicationServer.h | 4 ++-- .../GDBRemoteCommunicationServerLLGS.cpp | 4 ++-- .../tools/lldb-server/tests/LLGSTest.cpp | 22 +++++++++++++++++++ .../tools/lldb-server/tests/TestClient.cpp | 10 +++------ 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h index a35352480040..880caacd6414 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h @@ -57,8 +57,8 @@ protected: bool m_exit_now; // use in asynchronous handling to indicate process should // exit. - bool m_send_error_strings; // If the client enables this then - // we will send error strings as well. + bool m_send_error_strings = false; // If the client enables this then + // we will send error strings as well. PacketResult Handle_QErrorStringEnable(StringExtractorGDBRemote &packet); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp index 069720859a55..b72fd4cd9dc6 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp @@ -291,7 +291,7 @@ Status GDBRemoteCommunicationServerLLGS::AttachToProcess(lldb::pid_t pid) { // else. if (m_debugged_process_up && m_debugged_process_up->GetID() != LLDB_INVALID_PROCESS_ID) - return Status("cannot attach to a process %" PRIu64 + return Status("cannot attach to process %" PRIu64 " when another process with pid %" PRIu64 " is being debugged.", pid, m_debugged_process_up->GetID()); @@ -2935,7 +2935,7 @@ GDBRemoteCommunicationServerLLGS::Handle_vAttach( log->Printf("GDBRemoteCommunicationServerLLGS::%s failed to attach to " "pid %" PRIu64 ": %s\n", __FUNCTION__, pid, error.AsCString()); - return SendErrorResponse(0x01); + return SendErrorResponse(error); } // Notify we attached by sending a stop packet. diff --git a/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp b/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp index 8733adefa389..fab84f7dd6b5 100644 --- a/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp +++ b/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp @@ -46,3 +46,25 @@ TEST_F(TestBase, DS_TEST(DebugserverEnv)) { HasValue(testing::Property(&StopReply::getKind, WaitStatus{WaitStatus::Exit, 0}))); } + +TEST_F(TestBase, LLGS_TEST(vAttachRichError)) { + auto ClientOr = TestClient::launch(getLogFileName(), + {getInferiorPath("environment_check")}); + ASSERT_THAT_EXPECTED(ClientOr, Succeeded()); + auto &Client = **ClientOr; + + // Until we enable error strings we should just get the error code. + ASSERT_THAT_ERROR(Client.SendMessage("vAttach;1"), + Failed(testing::Property( + &ErrorInfoBase::message, "Error 255"))); + + ASSERT_THAT_ERROR(Client.SendMessage("QEnableErrorStrings"), Succeeded()); + + // Now, we expect the full error message. + ASSERT_THAT_ERROR( + Client.SendMessage("vAttach;1"), + Failed(testing::Property( + &ErrorInfoBase::message, + testing::StartsWith( + "cannot attach to process 1 when another process with pid")))); +} diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp index 5bc60018cdb0..7bd94588b2ae 100644 --- a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp +++ b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp @@ -162,13 +162,9 @@ Error TestClient::SendMessage(StringRef message) { Error TestClient::SendMessage(StringRef message, std::string &response_string) { if (Error E = SendMessage(message, response_string, PacketResult::Success)) return E; - if (response_string[0] == 'E') { - return make_error( - formatv("Error `{0}` while sending message: {1}", response_string, - message) - .str(), - inconvertibleErrorCode()); - } + StringExtractorGDBRemote Extractor(response_string); + if (Extractor.IsErrorResponse()) + return Extractor.GetStatus().ToError(); return Error::success(); }