[Lldb-commits] [lldb] [lldb] [Windows] Let LLGS TestClient tolerate async `O` output packets (PR #225521)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Yao Qi (qiyao)

<details>
<summary>Changes</summary>

My PR (swiftlang/llvm-project#<!-- -->14269) has a pre-merge test failure on
Windows, and that lead me looking at test LLGSTest.cpp deeply

  ```
  Send Packet: vCont;c
  Read Packet: O1b5b3f32356c
  lldb-server exiting...
  LLGSTest.cpp(30): error: Value of: llvm::detail::TakeError(Client.ContinueAll())
  Expected: succeeded
    Actual: failed  (Unable to parse StopReply: Invalid packet)
  ```

`TestClient::SendMessage` read exactly one packet and had no handling for `O`
(inferior stdout) packets, but inferior output arriving before a stop reply
was parsed as the stop reply itself.  `StopReply::create` accepts only `T` and
`W`, so it failed.  The `O1b5b3f32356c` decodes to `ESC [ ? 2 5 l`, the
sequence that hides the cursor.  Windows lldb-server gives the inferior a ConPTY
created with `PSEUDOCONSOLE_INHERIT_CURSOR`, and ConPTY writes its own VT
initialization bytes to the output pipe, which reach the client as an `O` packet.

The fix is that `SendMessage` now uses the inherited
`SendPacketAndReceiveResponseWithOutputSupport`, which loops over leading `O`
packets via `ReadPacketWithOutputSupport` and passes each payload to a
callback. `TestClient::Continue` had the same gap in the opposite order: after a
`W` or `X` reply it read one raw packet and required `ErrorDisconnected`, so an `O`
packet arriving before the socket closed failed the test.  It now reads through
`ReadPacketWithOutputSupport` as well.

The fix reverts 5032c6ea0803 ("[lldb-server] Disable a test on Windows until it can be
fixed"), which disabled `LaunchModePreservesEnvironment` on Windows.

18b211cb1521 ("Disable stdin/stdout for environment_check inferior process")
worked around the same root cause from a different source, where an allocator
message on stderr under sanitizers reached the client the same way.  That
commit noted the client "doesn't handle that unexpected output"; this handles
it.

Assisted-by: claude


---
Full diff: https://github.com/llvm/llvm-project/pull/225521.diff


2 Files Affected:

- (modified) lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp (-4) 
- (modified) lldb/unittests/tools/lldb-server/tests/TestClient.cpp (+8-2) 


``````````diff
diff --git a/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp b/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp
index b438d7496d8810..af0a88d7f91350 100644
--- a/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp
+++ b/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp
@@ -15,9 +15,6 @@ using namespace llgs_tests;
 using namespace lldb_private;
 using namespace llvm;
 
-// Disable this test on Windows as it appears to have a race condition
-// that causes lldb-server not to exit after the inferior hangs up.
-#if !defined(_WIN32)
 TEST_F(TestBase, LaunchModePreservesEnvironment) {
   putenv(const_cast<char *>("LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE"));
 
@@ -33,7 +30,6 @@ TEST_F(TestBase, LaunchModePreservesEnvironment) {
       HasValue(testing::Property(&StopReply::getKind,
                                  WaitStatus{WaitStatus::Exit, 0})));
 }
-#endif
 
 TEST_F(TestBase, DS_TEST(DebugserverEnv)) {
   // Test that --env takes precedence over inherited environment variables.
diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
index a459ae64254e89..fc5c82cb319776 100644
--- a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -31,6 +31,10 @@ static std::chrono::seconds GetDefaultTimeout() {
   return std::chrono::seconds{10};
 }
 
+static void LogInferiorOutput(StringRef output) {
+  GTEST_LOG_(INFO) << "Inferior output: " << output.str();
+}
+
 TestClient::TestClient(std::unique_ptr<Connection> Conn) {
   SetConnection(std::move(Conn));
   SetPacketTimeout(GetDefaultTimeout());
@@ -207,7 +211,8 @@ Error TestClient::SendMessage(StringRef message, std::string &response_string,
                               PacketResult expected_result) {
   StringExtractorGDBRemote response;
   GTEST_LOG_(INFO) << "Send Packet: " << message.str();
-  PacketResult result = SendPacketAndWaitForResponse(message, response);
+  PacketResult result = SendPacketAndReceiveResponseWithOutputSupport(
+      message, response, std::chrono::seconds(0), LogInferiorOutput);
   response.GetEscapedBinaryData(response_string);
   GTEST_LOG_(INFO) << "Read Packet: " << response_string;
   if (result != expected_result)
@@ -275,7 +280,8 @@ Error TestClient::Continue(StringRef message) {
   m_stop_reply = std::move(*StopReplyOr);
   if (!isa<StopReplyStop>(m_stop_reply)) {
     StringExtractorGDBRemote R;
-    PacketResult result = ReadPacket(R, GetPacketTimeout(), false);
+    PacketResult result = ReadPacketWithOutputSupport(R, GetPacketTimeout(),
+                                                      false, LogInferiorOutput);
     if (result != PacketResult::ErrorDisconnected) {
       return createStringErrorV("Expected connection close after sending {0}. "
                                 "Got {1}/{2} instead.",

``````````

</details>


https://github.com/llvm/llvm-project/pull/225521