[Lldb-commits] [lldb] r329803 - llgs: Send "rich" errors in response to vAttach packets

[Pavel Labath via lldb-commits]

Author: labath
Date: Wed Apr 11 06:30:54 2018
New Revision: 329803

URL: http://llvm.org/viewvc/llvm-project?rev=329803&view=rev
Log:
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.

Modified:
    lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
    lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
    lldb/trunk/unittests/tools/lldb-server/tests/LLGSTest.cpp
    lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp

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=329803&r1=329802&r2=329803&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h Wed Apr 11 06:30:54 2018
@@ -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);
 

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=329803&r1=329802&r2=329803&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp Wed Apr 11 06:30:54 2018
@@ -291,7 +291,7 @@ Status GDBRemoteCommunicationServerLLGS:
   // 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
       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.

Modified: lldb/trunk/unittests/tools/lldb-server/tests/LLGSTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/tools/lldb-server/tests/LLGSTest.cpp?rev=329803&r1=329802&r2=329803&view=diff
==============================================================================
--- lldb/trunk/unittests/tools/lldb-server/tests/LLGSTest.cpp (original)
+++ lldb/trunk/unittests/tools/lldb-server/tests/LLGSTest.cpp Wed Apr 11 06:30:54 2018
@@ -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<ErrorInfoBase>(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<ErrorInfoBase>(testing::Property(
+          &ErrorInfoBase::message,
+          testing::StartsWith(
+              "cannot attach to process 1 when another process with pid"))));
+}

Modified: lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp?rev=329803&r1=329802&r2=329803&view=diff
==============================================================================
--- lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp (original)
+++ lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp Wed Apr 11 06:30:54 2018
@@ -162,13 +162,9 @@ Error TestClient::SendMessage(StringRef
 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<StringError>(
-        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();
 }