[lldb] Implement JSON RPC (newline delimited) Transport #143946
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR implements JSON RPC-style (i.e. newline delimited) JSON transport. I moved the existing transport tests from DAP to Host and moved the PipeTest base class into TestingSupport so it can be shared by both.
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis PR implements JSON RPC-style (i.e. newline delimited) JSON transport. I moved the existing transport tests from DAP to Host and moved the PipeTest base class into TestingSupport so it can be shared by both. Full diff: https://github.com/llvm/llvm-project/pull/143946.diff 9 Files Affected:
diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h
index 4db5e417ea852..f7593750669e1 100644
--- a/lldb/include/lldb/Host/JSONTransport.h
+++ b/lldb/include/lldb/Host/JSONTransport.h
@@ -121,6 +121,21 @@ class HTTPDelimitedJSONTransport : public JSONTransport {
static constexpr llvm::StringLiteral kHeaderSeparator = "\r\n\r\n";
};
+/// A transport class for JSON RPC.
+class JSONRPCTransport : public JSONTransport {
+public:
+ JSONRPCTransport(lldb::IOObjectSP input, lldb::IOObjectSP output)
+ : JSONTransport(input, output) {}
+ virtual ~JSONRPCTransport() = default;
+
+protected:
+ virtual llvm::Error WriteImpl(const std::string &message) override;
+ virtual llvm::Expected<std::string>
+ ReadImpl(const std::chrono::microseconds &timeout) override;
+
+ static constexpr llvm::StringLiteral kMessageSeparator = "\n";
+};
+
} // namespace lldb_private
#endif
diff --git a/lldb/source/Host/common/JSONTransport.cpp b/lldb/source/Host/common/JSONTransport.cpp
index 103c76d25daf7..7b925372e0b74 100644
--- a/lldb/source/Host/common/JSONTransport.cpp
+++ b/lldb/source/Host/common/JSONTransport.cpp
@@ -92,7 +92,7 @@ void JSONTransport::Log(llvm::StringRef message) {
Expected<std::string>
HTTPDelimitedJSONTransport::ReadImpl(const std::chrono::microseconds &timeout) {
if (!m_input || !m_input->IsValid())
- return createStringError("transport output is closed");
+ return llvm::make_error<TransportClosedError>();
IOObject *input = m_input.get();
Expected<std::string> message_header =
@@ -142,6 +142,35 @@ Error HTTPDelimitedJSONTransport::WriteImpl(const std::string &message) {
return m_output->Write(Output.data(), num_bytes).takeError();
}
+Expected<std::string>
+JSONRPCTransport::ReadImpl(const std::chrono::microseconds &timeout) {
+ if (!m_input || !m_input->IsValid())
+ return make_error<TransportClosedError>();
+
+ IOObject *input = m_input.get();
+ Expected<std::string> raw_json =
+ ReadUntil(*input, kMessageSeparator, timeout);
+ if (!raw_json)
+ return raw_json.takeError();
+
+ Log(llvm::formatv("--> {0}", *raw_json).str());
+
+ return *raw_json;
+}
+
+Error JSONRPCTransport::WriteImpl(const std::string &message) {
+ if (!m_output || !m_output->IsValid())
+ return llvm::make_error<TransportClosedError>();
+
+ Log(llvm::formatv("<-- {0}", message).str());
+
+ std::string Output;
+ llvm::raw_string_ostream OS(Output);
+ OS << message << kMessageSeparator;
+ size_t num_bytes = Output.size();
+ return m_output->Write(Output.data(), num_bytes).takeError();
+}
+
char TransportEOFError::ID;
char TransportTimeoutError::ID;
char TransportClosedError::ID;
diff --git a/lldb/unittests/DAP/CMakeLists.txt b/lldb/unittests/DAP/CMakeLists.txt
index 37a6a81ad12a0..ee623d341ec69 100644
--- a/lldb/unittests/DAP/CMakeLists.txt
+++ b/lldb/unittests/DAP/CMakeLists.txt
@@ -7,7 +7,6 @@ add_lldb_unittest(DAPTests
LLDBUtilsTest.cpp
ProtocolTypesTest.cpp
TestBase.cpp
- TransportTest.cpp
VariablesTest.cpp
LINK_COMPONENTS
diff --git a/lldb/unittests/DAP/TestBase.cpp b/lldb/unittests/DAP/TestBase.cpp
index 4063b34250312..27ad42686fbbf 100644
--- a/lldb/unittests/DAP/TestBase.cpp
+++ b/lldb/unittests/DAP/TestBase.cpp
@@ -28,13 +28,8 @@ using lldb_private::File;
using lldb_private::NativeFile;
using lldb_private::Pipe;
-void PipeBase::SetUp() {
- ASSERT_THAT_ERROR(input.CreateNew(false).ToError(), Succeeded());
- ASSERT_THAT_ERROR(output.CreateNew(false).ToError(), Succeeded());
-}
-
void TransportBase::SetUp() {
- PipeBase::SetUp();
+ PipeTest::SetUp();
to_dap = std::make_unique<Transport>(
"to_dap", nullptr,
std::make_shared<NativeFile>(input.GetReadFileDescriptor(),
diff --git a/lldb/unittests/DAP/TestBase.h b/lldb/unittests/DAP/TestBase.h
index 70b3985271a92..25d37013954d5 100644
--- a/lldb/unittests/DAP/TestBase.h
+++ b/lldb/unittests/DAP/TestBase.h
@@ -8,26 +8,17 @@
#include "DAP.h"
#include "Protocol/ProtocolBase.h"
+#include "TestingSupport/Host/PipeTestUtilities.h"
#include "Transport.h"
-#include "lldb/Host/Pipe.h"
#include "llvm/ADT/StringRef.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
namespace lldb_dap_tests {
-/// A base class for tests that need a pair of pipes for communication.
-class PipeBase : public testing::Test {
-protected:
- lldb_private::Pipe input;
- lldb_private::Pipe output;
-
- void SetUp() override;
-};
-
/// A base class for tests that need transport configured for communicating DAP
/// messages.
-class TransportBase : public PipeBase {
+class TransportBase : public PipeTest {
protected:
std::unique_ptr<lldb_dap::Transport> to_dap;
std::unique_ptr<lldb_dap::Transport> from_dap;
diff --git a/lldb/unittests/DAP/TransportTest.cpp b/lldb/unittests/DAP/TransportTest.cpp
deleted file mode 100644
index aaf257993af23..0000000000000
--- a/lldb/unittests/DAP/TransportTest.cpp
+++ /dev/null
@@ -1,98 +0,0 @@
-//===-- TransportTest.cpp -------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "Transport.h"
-#include "Protocol/ProtocolBase.h"
-#include "TestBase.h"
-#include "lldb/Host/File.h"
-#include "lldb/Host/Pipe.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/Testing/Support/Error.h"
-#include "gtest/gtest.h"
-#include <chrono>
-#include <memory>
-#include <optional>
-
-using namespace llvm;
-using namespace lldb;
-using namespace lldb_dap;
-using namespace lldb_dap_tests;
-using namespace lldb_dap::protocol;
-using lldb_private::File;
-using lldb_private::NativeFile;
-using lldb_private::Pipe;
-using lldb_private::TransportEOFError;
-using lldb_private::TransportTimeoutError;
-
-class TransportTest : public PipeBase {
-protected:
- std::unique_ptr<Transport> transport;
-
- void SetUp() override {
- PipeBase::SetUp();
- transport = std::make_unique<Transport>(
- "stdio", nullptr,
- std::make_shared<NativeFile>(input.GetReadFileDescriptor(),
- File::eOpenOptionReadOnly,
- NativeFile::Unowned),
- std::make_shared<NativeFile>(output.GetWriteFileDescriptor(),
- File::eOpenOptionWriteOnly,
- NativeFile::Unowned));
- }
-};
-
-TEST_F(TransportTest, MalformedRequests) {
- std::string malformed_header = "COnTent-LenGth: -1{}\r\n\r\nnotjosn";
- ASSERT_THAT_EXPECTED(
- input.Write(malformed_header.data(), malformed_header.size()),
- Succeeded());
- ASSERT_THAT_EXPECTED(
- transport->Read<protocol::Message>(std::chrono::milliseconds(1)),
- FailedWithMessage(
- "expected 'Content-Length: ' and got 'COnTent-LenGth: '"));
-}
-
-TEST_F(TransportTest, Read) {
- std::string json =
- R"json({"seq": 1, "type": "request", "command": "abc"})json";
- std::string message =
- formatv("Content-Length: {0}\r\n\r\n{1}", json.size(), json).str();
- ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
- Succeeded());
- ASSERT_THAT_EXPECTED(
- transport->Read<protocol::Message>(std::chrono::milliseconds(1)),
- HasValue(testing::VariantWith<Request>(testing::FieldsAre(
- /*seq=*/1, /*command=*/"abc", /*arguments=*/std::nullopt))));
-}
-
-TEST_F(TransportTest, ReadWithTimeout) {
- ASSERT_THAT_EXPECTED(
- transport->Read<protocol::Message>(std::chrono::milliseconds(1)),
- Failed<TransportTimeoutError>());
-}
-
-TEST_F(TransportTest, ReadWithEOF) {
- input.CloseWriteFileDescriptor();
- ASSERT_THAT_EXPECTED(
- transport->Read<protocol::Message>(std::chrono::milliseconds(1)),
- Failed<TransportEOFError>());
-}
-
-TEST_F(TransportTest, Write) {
- ASSERT_THAT_ERROR(transport->Write(Event{"my-event", std::nullopt}),
- Succeeded());
- output.CloseWriteFileDescriptor();
- char buf[1024];
- Expected<size_t> bytes_read =
- output.Read(buf, sizeof(buf), std::chrono::milliseconds(1));
- ASSERT_THAT_EXPECTED(bytes_read, Succeeded());
- ASSERT_EQ(
- StringRef(buf, *bytes_read),
- StringRef("Content-Length: 43\r\n\r\n"
- R"json({"event":"my-event","seq":0,"type":"event"})json"));
-}
diff --git a/lldb/unittests/Host/CMakeLists.txt b/lldb/unittests/Host/CMakeLists.txt
index 5b8deed00af88..3b20f1d723d18 100644
--- a/lldb/unittests/Host/CMakeLists.txt
+++ b/lldb/unittests/Host/CMakeLists.txt
@@ -6,6 +6,7 @@ set (FILES
HostInfoTest.cpp
HostTest.cpp
MainLoopTest.cpp
+ JSONTransportTest.cpp
NativeProcessProtocolTest.cpp
PipeTest.cpp
ProcessLaunchInfoTest.cpp
diff --git a/lldb/unittests/Host/JSONTransportTest.cpp b/lldb/unittests/Host/JSONTransportTest.cpp
new file mode 100644
index 0000000000000..bb7958b4e95b5
--- /dev/null
+++ b/lldb/unittests/Host/JSONTransportTest.cpp
@@ -0,0 +1,146 @@
+//===-- JSONTransportTest.cpp ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Host/JSONTransport.h"
+#include "TestingSupport/Host/PipeTestUtilities.h"
+#include "lldb/Host/File.h"
+
+using namespace llvm;
+using namespace lldb_private;
+
+namespace {
+template <typename T> class JSONTransportTest : public PipeTest {
+protected:
+ std::unique_ptr<JSONTransport> transport;
+
+ void SetUp() override {
+ PipeTest::SetUp();
+ transport = std::make_unique<T>(
+ std::make_shared<NativeFile>(input.GetReadFileDescriptor(),
+ File::eOpenOptionReadOnly,
+ NativeFile::Unowned),
+ std::make_shared<NativeFile>(output.GetWriteFileDescriptor(),
+ File::eOpenOptionWriteOnly,
+ NativeFile::Unowned));
+ }
+};
+
+class HTTPDelimitedJSONTransportTest
+ : public JSONTransportTest<HTTPDelimitedJSONTransport> {
+public:
+ using JSONTransportTest::JSONTransportTest;
+};
+
+class JSONRPCTransportTest : public JSONTransportTest<JSONRPCTransport> {
+public:
+ using JSONTransportTest::JSONTransportTest;
+};
+
+struct JSONTestType {
+ std::string str;
+};
+
+llvm::json::Value toJSON(const JSONTestType &T) {
+ return llvm::json::Object{{"str", T.str}};
+}
+
+bool fromJSON(const llvm::json::Value &V, JSONTestType &T, llvm::json::Path P) {
+ llvm::json::ObjectMapper O(V, P);
+ return O && O.map("str", T.str);
+}
+} // namespace
+
+TEST_F(HTTPDelimitedJSONTransportTest, MalformedRequests) {
+ std::string malformed_header = "COnTent-LenGth: -1{}\r\n\r\nnotjosn";
+ ASSERT_THAT_EXPECTED(
+ input.Write(malformed_header.data(), malformed_header.size()),
+ Succeeded());
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ FailedWithMessage(
+ "expected 'Content-Length: ' and got 'COnTent-LenGth: '"));
+}
+
+TEST_F(HTTPDelimitedJSONTransportTest, Read) {
+ std::string json = R"json({"str": "foo"})json";
+ std::string message =
+ formatv("Content-Length: {0}\r\n\r\n{1}", json.size(), json).str();
+ ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
+ Succeeded());
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ HasValue(testing::FieldsAre(/*str=*/"foo")));
+}
+
+TEST_F(HTTPDelimitedJSONTransportTest, ReadWithTimeout) {
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ Failed<TransportTimeoutError>());
+}
+
+TEST_F(HTTPDelimitedJSONTransportTest, ReadWithEOF) {
+ input.CloseWriteFileDescriptor();
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ Failed<TransportEOFError>());
+}
+
+TEST_F(HTTPDelimitedJSONTransportTest, Write) {
+ ASSERT_THAT_ERROR(transport->Write(JSONTestType{"foo"}), Succeeded());
+ output.CloseWriteFileDescriptor();
+ char buf[1024];
+ Expected<size_t> bytes_read =
+ output.Read(buf, sizeof(buf), std::chrono::milliseconds(1));
+ ASSERT_THAT_EXPECTED(bytes_read, Succeeded());
+ ASSERT_EQ(StringRef(buf, *bytes_read), StringRef("Content-Length: 13\r\n\r\n"
+ R"json({"str":"foo"})json"));
+}
+
+TEST_F(JSONRPCTransportTest, MalformedRequests) {
+ std::string malformed_header = "notjson\n";
+ ASSERT_THAT_EXPECTED(
+ input.Write(malformed_header.data(), malformed_header.size()),
+ Succeeded());
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ llvm::Failed());
+}
+
+TEST_F(JSONRPCTransportTest, Read) {
+ std::string json = R"json({"str": "foo"})json";
+ std::string message = formatv("{0}\n", json).str();
+ ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
+ Succeeded());
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ HasValue(testing::FieldsAre(/*str=*/"foo")));
+}
+
+TEST_F(JSONRPCTransportTest, ReadWithTimeout) {
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ Failed<TransportTimeoutError>());
+}
+
+TEST_F(JSONRPCTransportTest, ReadWithEOF) {
+ input.CloseWriteFileDescriptor();
+ ASSERT_THAT_EXPECTED(
+ transport->Read<JSONTestType>(std::chrono::milliseconds(1)),
+ Failed<TransportEOFError>());
+}
+
+TEST_F(JSONRPCTransportTest, Write) {
+ ASSERT_THAT_ERROR(transport->Write(JSONTestType{"foo"}), Succeeded());
+ output.CloseWriteFileDescriptor();
+ char buf[1024];
+ Expected<size_t> bytes_read =
+ output.Read(buf, sizeof(buf), std::chrono::milliseconds(1));
+ ASSERT_THAT_EXPECTED(bytes_read, Succeeded());
+ ASSERT_EQ(StringRef(buf, *bytes_read), StringRef(R"json({"str":"foo"})json"
+ "\n"));
+}
diff --git a/lldb/unittests/TestingSupport/Host/PipeTestUtilities.h b/lldb/unittests/TestingSupport/Host/PipeTestUtilities.h
new file mode 100644
index 0000000000000..50d5d4117c898
--- /dev/null
+++ b/lldb/unittests/TestingSupport/Host/PipeTestUtilities.h
@@ -0,0 +1,28 @@
+//===-- PipeTestUtilities.cpp ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UNITTESTS_TESTINGSUPPORT_PIPETESTUTILITIES_H
+#define LLDB_UNITTESTS_TESTINGSUPPORT_PIPETESTUTILITIES_H
+
+#include "lldb/Host/Pipe.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+/// A base class for tests that need a pair of pipes for communication.
+class PipeTest : public testing::Test {
+protected:
+ lldb_private::Pipe input;
+ lldb_private::Pipe output;
+
+ void SetUp() override {
+ ASSERT_THAT_ERROR(input.CreateNew(false).ToError(), llvm::Succeeded());
+ ASSERT_THAT_ERROR(output.CreateNew(false).ToError(), llvm::Succeeded());
+ }
+};
+
+#endif
|
|
@JDevlieghere not a review, but any context on for the change? Just curious. |
ashgti
approved these changes
Jun 12, 2025
| Failed<TransportEOFError>()); | ||
| } | ||
|
|
||
| TEST_F(JSONRPCTransportTest, Write) { |
There was a problem hiding this comment.
Should we also have a test where we close the input or output handle before the read/write to trigger the TransportClosedError?
There was a problem hiding this comment.
Added a test and renamed the error as the current name isn't really accurate: this error occurs when the underlying IO object is invalid. If you close the write side, you get an EOF error, which we have a test for. If you close the read side, then you get a bad file descriptor error, which I've added a new test for too.
Comment on lines
124
to
128
| TEST_F(JSONRPCTransportTest, ReadWithTimeout) { | ||
| ASSERT_THAT_EXPECTED( | ||
| transport->Read<JSONTestType>(std::chrono::milliseconds(1)), | ||
| Failed<TransportTimeoutError>()); | ||
| } |
There was a problem hiding this comment.
I've just realized, should this test be skipped on Windows?
The lldb SelectHelper only works with sockets on Windows and under the hood the Pipe is making a named socket, but I'm not sure if we're using the right API to realize that or not. When we create the handle we're using NativeFile that may be handling this correctly.
This all might be working correctly on Windows, but I'll be honest that I cannot tell for sure.
The frustrating (for me) part is that on Windows, stdin/stdout are anonymous pipes and you cannot use overlapping IO operations on an anonymous pipe on Windows. Libraries like libdispatch (used by swift), libuv (used by nodejs and others), pythons asyncio, etc. on Windows emulate this behavior by spawning a thread to do the reading, but at the moment we don't have that sort of abstraction in lldb yet.
If we're planning on making more improvements in this area for a json-rpc server, we may need to add this to lldb so we can properly handle stopping these read threads.
I'll be honest though that I haven't done much programing on Windows and I tried seeing if I could figure this out when I was working on lldb-dap server mode and kept running into issues. I could be wrong on some of this, since I don't know the Windows APIs all that well.
There was a problem hiding this comment.
Ack, yeah I'm far from an expert myself, but it's good context, thanks.
Apologies for not providing more context. This is a continuation of #143806 which is partially motivated by https://discourse.llvm.org/t/rfc-adding-mcp-support-to-lldb/86798. |
|
I tried to see if the existing timeout test was working on the Windows buildbot and its not running due to https://github.com/llvm/llvm-project/blob/main/lldb/unittests/CMakeLists.txt#L50 so the timeout tests may need an |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/9422 Here is the relevant piece of the build log for the reference |
|
This is failing on Windows on Arm: Few clues but looks like |
|
Even prior to this PR, running HostTests.exe on its own in a terminal causes the terminal to completely close? Always something new to discover on Windows isn't there. |
DavidSpickett
added a commit
that referenced
this pull request
Jun 13, 2025
These were failing on our Windows on Arm bot, or more precisely, not even completing. This is because Microsoft's C runtime does extra parameter validation. So when we called _read with an invalid fd, it called an invalid parameter handler instead of returning an error. https://learn.microsoft.com/en-us/%20cpp/c-runtime-library/reference/read?view=msvc-170 https://learn.microsoft.com/en-us/%20cpp/c-runtime-library/parameter-validation?view=msvc-170 (lldb) run Process 8440 launched: 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\unittests\Host\HostTests.exe' (aarch64) Process 8440 stopped * thread #1, stop reason = Exception 0xc0000409 encountered at address 0x7ffb7453564c frame #0: 0x00007ffb7453564c ucrtbase.dll`_get_thread_local_invalid_parameter_handler + 652 ucrtbase.dll`_get_thread_local_invalid_parameter_handler: -> 0x7ffb7453564c <+652>: brk #0xf003 ucrtbase.dll`_invalid_parameter_noinfo: 0x7ffb74535650 <+0>: b 0x7ffb745354d8 ; _get_thread_local_invalid_parameter_handler + 280 0x7ffb74535654 <+4>: nop 0x7ffb74535658 <+8>: nop You can override this handler but I'm assuming that this reading after close isn't a crucial feature, so disabling the tests seems like the way to go. If it is crucial, we can check the fd before we use it. Tests added by #143946.
|
#144077 for the terminal closing. It's not related to this change. I had to disable two of the tests that try to read a closed file descriptor. MS CRT validates the fd before it reads it and this leads to a software breakpoint / crash. 82911f1 If this is a feature you actually rely on, we should check the fd is valid before we use it. As in: check and if it's invalid early return an error, instead of getting all the way to reading from it. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 13, 2025
…indows These were failing on our Windows on Arm bot, or more precisely, not even completing. This is because Microsoft's C runtime does extra parameter validation. So when we called _read with an invalid fd, it called an invalid parameter handler instead of returning an error. https://learn.microsoft.com/en-us/%20cpp/c-runtime-library/reference/read?view=msvc-170 https://learn.microsoft.com/en-us/%20cpp/c-runtime-library/parameter-validation?view=msvc-170 (lldb) run Process 8440 launched: 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\unittests\Host\HostTests.exe' (aarch64) Process 8440 stopped * thread #1, stop reason = Exception 0xc0000409 encountered at address 0x7ffb7453564c frame #0: 0x00007ffb7453564c ucrtbase.dll`_get_thread_local_invalid_parameter_handler + 652 ucrtbase.dll`_get_thread_local_invalid_parameter_handler: -> 0x7ffb7453564c <+652>: brk #0xf003 ucrtbase.dll`_invalid_parameter_noinfo: 0x7ffb74535650 <+0>: b 0x7ffb745354d8 ; _get_thread_local_invalid_parameter_handler + 280 0x7ffb74535654 <+4>: nop 0x7ffb74535658 <+8>: nop You can override this handler but I'm assuming that this reading after close isn't a crucial feature, so disabling the tests seems like the way to go. If it is crucial, we can check the fd before we use it. Tests added by llvm/llvm-project#143946.
The start of the The |
|
@ashgti AFAIK that only checks if the file descriptor is a valid number (i.e. >= 0) not whether the underlying FD is actually valid. On POSIX we definitely do check that and return an |
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
This PR implements JSON RPC-style (i.e. newline delimited) JSON transport. I moved the existing transport tests from DAP to Host and moved the PipeTest base class into TestingSupport so it can be shared by both.
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
These were failing on our Windows on Arm bot, or more precisely, not even completing. This is because Microsoft's C runtime does extra parameter validation. So when we called _read with an invalid fd, it called an invalid parameter handler instead of returning an error. https://learn.microsoft.com/en-us/%20cpp/c-runtime-library/reference/read?view=msvc-170 https://learn.microsoft.com/en-us/%20cpp/c-runtime-library/parameter-validation?view=msvc-170 (lldb) run Process 8440 launched: 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\unittests\Host\HostTests.exe' (aarch64) Process 8440 stopped * thread llvm#1, stop reason = Exception 0xc0000409 encountered at address 0x7ffb7453564c frame #0: 0x00007ffb7453564c ucrtbase.dll`_get_thread_local_invalid_parameter_handler + 652 ucrtbase.dll`_get_thread_local_invalid_parameter_handler: -> 0x7ffb7453564c <+652>: brk #0xf003 ucrtbase.dll`_invalid_parameter_noinfo: 0x7ffb74535650 <+0>: b 0x7ffb745354d8 ; _get_thread_local_invalid_parameter_handler + 280 0x7ffb74535654 <+4>: nop 0x7ffb74535658 <+8>: nop You can override this handler but I'm assuming that this reading after close isn't a crucial feature, so disabling the tests seems like the way to go. If it is crucial, we can check the fd before we use it. Tests added by llvm#143946.
|
Yep: I suspect we do the same thing everywhere, and being non-zero isn't enough to be "valid" if by "valid" you mean "is still open". |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements JSON RPC-style (i.e. newline delimited) JSON transport. I moved the existing transport tests from DAP to Host and moved the PipeTest base class into TestingSupport so it can be shared by both.