[llvm] [orc-rt] Simplify ConnectorRegistry::connect and ogre setup. (PR #226772)
Lang Hames via llvm-commits
llvm-commits at lists.llvm.org
Sun Sep 27 01:36:46 PDT 2026
https://github.com/lhames created https://github.com/llvm/llvm-project/pull/226772
ConnectorRegistry::connect and ConnectorFn now take the Session and BootstrapInfo directly, replacing the GetAttachInfo callback and the AttachInfo struct. The callback let the Session be built lazily, after the connector had validated its spec, but a connection can fail after validation anyway, so callers already had to handle discarding a Session built for a failed connection. Build the Session first, then connect.
ogre's setup is restructured to match: makeSession detects the process info, builds a thread-pool dispatcher and the Session, adds the host services, then connects; runOgre waits for the detach. Errors are reported via Session::logErrors when logging is enabled.
SocketConnectorTest now connects a real Session. Its ownership test adopts a non-stream socket, which the connector accepts and createSimpleRemoteCAOverSocket then rejects, rather than relying on a failing GetAttachInfo.
>From 78127e486aa3efceb6767917aa3bb97e76e7b9f7 Mon Sep 17 00:00:00 2001
From: Lang Hames <lhames at gmail.com>
Date: Sun, 27 Sep 2026 13:05:55 +1000
Subject: [PATCH] [orc-rt] Simplify ConnectorRegistry::connect and ogre setup.
ConnectorRegistry::connect and ConnectorFn now take the Session and
BootstrapInfo directly, replacing the GetAttachInfo callback and the
AttachInfo struct. The callback let the Session be built lazily, after
the connector had validated its spec, but a connection can fail after
validation anyway, so callers already had to handle discarding a Session
built for a failed connection. Build the Session first, then connect.
ogre's setup is restructured to match: makeSession detects the process
info, builds a thread-pool dispatcher and the Session, adds the host
services, then connects; runOgre waits for the detach. Errors are
reported via Session::logErrors when logging is enabled.
SocketConnectorTest now connects a real Session. Its ownership test
adopts a non-stream socket, which the connector accepts and
createSimpleRemoteCAOverSocket then rejects, rather than relying on a
failing GetAttachInfo.
---
.../orc-rt/bedrock/ConnectorRegistry.h | 18 +---
orc-rt/lib/bedrock/ConnectorRegistry.cpp | 10 +-
.../lib/bedrock/sys/posix/SocketConnector.cpp | 12 +--
.../bedrock/sys/posix/SocketConnectorTest.cpp | 42 ++++----
orc-rt/tools/ogre/ogre.cpp | 102 ++++++++++--------
5 files changed, 88 insertions(+), 96 deletions(-)
diff --git a/orc-rt/include/orc-rt/bedrock/ConnectorRegistry.h b/orc-rt/include/orc-rt/bedrock/ConnectorRegistry.h
index 72150f3b66df1..3faf757411673 100644
--- a/orc-rt/include/orc-rt/bedrock/ConnectorRegistry.h
+++ b/orc-rt/include/orc-rt/bedrock/ConnectorRegistry.h
@@ -34,18 +34,10 @@ class Session;
/// ensures that only requested transport mechanisms are available.
class ConnectorRegistry {
public:
- struct AttachInfo {
- Session &S;
- BootstrapInfo BI;
- };
-
- /// Supplies the Session and BootstrapInfo to connect.
- using GetAttachInfoFn = move_only_function<Expected<AttachInfo>() noexcept>;
-
- /// Establishes the connection CS describes and attaches it to the Session
- /// that GetSession returns.
+ /// Establishes the connection CS describes and attaches it to S, handing
+ /// over BI.
using ConnectorFn = move_only_function<Error(
- GetAttachInfoFn GetAttachInfo, const ConnectionSpec &) noexcept>;
+ const ConnectionSpec &CS, Session &S, BootstrapInfo BI) noexcept>;
/// Registers Connector as the handler for Transport.
///
@@ -58,8 +50,8 @@ class ConnectorRegistry {
///
/// Fails if no connector is registered for it, which is how a spec naming a
/// transport this process was not built with is reported.
- Error connect(GetAttachInfoFn GetAttachInfo,
- const ConnectionSpec &CS) noexcept;
+ Error connect(const ConnectionSpec &CS, Session &S,
+ BootstrapInfo BI) noexcept;
private:
std::mutex M;
diff --git a/orc-rt/lib/bedrock/ConnectorRegistry.cpp b/orc-rt/lib/bedrock/ConnectorRegistry.cpp
index c850cca7f4cbd..d50350b76b00e 100644
--- a/orc-rt/lib/bedrock/ConnectorRegistry.cpp
+++ b/orc-rt/lib/bedrock/ConnectorRegistry.cpp
@@ -34,8 +34,8 @@ Error ConnectorRegistry::registerConnector(std::string Transport,
return Error::success();
}
-Error ConnectorRegistry::connect(GetAttachInfoFn GetAttachInfo,
- const ConnectionSpec &CS) noexcept {
+Error ConnectorRegistry::connect(const ConnectionSpec &CS, Session &S,
+ BootstrapInfo BI) noexcept {
ConnectorFn *Connector = nullptr;
{
std::scoped_lock<std::mutex> Lock(M);
@@ -49,11 +49,7 @@ Error ConnectorRegistry::connect(GetAttachInfoFn GetAttachInfo,
Connector = &I->second;
}
- // Run the connector without the lock: it blocks on IO, and may register
- // further connectors or connect again. The pointer stays good because
- // unordered_map does not move its elements on insert, and nothing removes
- // them.
- return (*Connector)(std::move(GetAttachInfo), CS);
+ return (*Connector)(CS, S, std::move(BI));
}
} // namespace orc_rt
diff --git a/orc-rt/lib/bedrock/sys/posix/SocketConnector.cpp b/orc-rt/lib/bedrock/sys/posix/SocketConnector.cpp
index 1ddbd474959e0..dd1fff2719417 100644
--- a/orc-rt/lib/bedrock/sys/posix/SocketConnector.cpp
+++ b/orc-rt/lib/bedrock/sys/posix/SocketConnector.cpp
@@ -24,8 +24,8 @@ using namespace orc_rt;
namespace {
-Error socketConnector(ConnectorRegistry::GetAttachInfoFn GetAttachInfo,
- const ConnectionSpec &CS) noexcept {
+Error socketConnector(const ConnectionSpec &CS, Session &S,
+ BootstrapInfo BI) noexcept {
auto BadCS = [&](const std::string &Reason) noexcept {
return make_error<StringError>((StringOutputStream()
<< "Invalid connection spec \"" << CS.str()
@@ -59,16 +59,12 @@ Error socketConnector(ConnectorRegistry::GetAttachInfoFn GetAttachInfo,
return BadCS("file descriptor " + std::string(FDStr) +
" is not a socket (" + sys::strError(ErrNum) + ")");
}
- SocketHandle Sock(FD);
- auto AI = GetAttachInfo();
- if (!AI)
- return AI.takeError();
- auto CA = createSimpleRemoteCAOverSocket(AI->S, std::move(Sock));
+ auto CA = createSimpleRemoteCAOverSocket(S, SocketHandle(FD));
if (!CA)
return CA.takeError();
- AI->S.attach(std::move(*CA), std::move(AI->BI));
+ S.attach(std::move(*CA), std::move(BI));
return Error::success();
}
diff --git a/orc-rt/test/unit/bedrock/sys/posix/SocketConnectorTest.cpp b/orc-rt/test/unit/bedrock/sys/posix/SocketConnectorTest.cpp
index 43d5d7784612f..2bf7648b1608f 100644
--- a/orc-rt/test/unit/bedrock/sys/posix/SocketConnectorTest.cpp
+++ b/orc-rt/test/unit/bedrock/sys/posix/SocketConnectorTest.cpp
@@ -11,7 +11,10 @@
//===----------------------------------------------------------------------===//
#include "orc-rt/bedrock/SocketConnector.h"
+#include "orc-rt/bedrock/Session.h"
+#include "BedrockTestUtils.h"
+#include "CommonTestUtils.h"
#include "ErrorMatchers.h"
#include "bedrock/SocketTestUtils.h"
@@ -28,22 +31,18 @@ using ::testing::HasSubstr;
namespace {
-/// Runs "socket:adopt=<FD>" through the socket connector. The GetAttachInfo it
-/// supplies records that it was called and then fails, so a descriptor that
-/// passes validation stops there rather than being attached.
-Error adoptFD(int FD, bool &AttachInfoRequested) {
+/// Runs "socket:adopt=<FD>" through the socket connector, targeting a fresh
+/// Session. Every case below fails before attach, so the Session never
+/// connects (and noErrors would catch an unexpected attach failure).
+Error adoptFD(int FD) {
ConnectorRegistry R;
if (auto Err = registerSocketConnector(R))
return Err;
auto CS = ConnectionSpec::parse("socket:adopt=" + std::to_string(FD));
if (!CS)
return CS.takeError();
- return R.connect(
- [&]() noexcept -> Expected<ConnectorRegistry::AttachInfo> {
- AttachInfoRequested = true;
- return make_error<StringError>("attach info requested");
- },
- *CS);
+ Session S(mockExecutorProcessInfo(), noDispatch, noErrors);
+ return R.connect(*CS, S, BootstrapInfo(S));
}
bool isOpen(int FD) { return ::fcntl(FD, F_GETFD) != -1; }
@@ -52,10 +51,8 @@ TEST(SocketConnectorTest, RejectsPipe) {
int P[2];
ASSERT_EQ(::pipe(P), 0);
- bool AttachInfoRequested = false;
- EXPECT_THAT_ERROR(adoptFD(P[0], AttachInfoRequested),
+ EXPECT_THAT_ERROR(adoptFD(P[0]),
FailedWithMessage(HasSubstr("is not a socket")));
- EXPECT_FALSE(AttachInfoRequested);
EXPECT_TRUE(isOpen(P[0])) << "a rejected descriptor must be left open";
::close(P[0]);
@@ -68,20 +65,19 @@ TEST(SocketConnectorTest, RejectsClosedDescriptor) {
::close(P[0]);
::close(P[1]);
- bool AttachInfoRequested = false;
- EXPECT_THAT_ERROR(adoptFD(P[0], AttachInfoRequested),
+ EXPECT_THAT_ERROR(adoptFD(P[0]),
FailedWithMessage(HasSubstr("is not a socket")));
- EXPECT_FALSE(AttachInfoRequested);
}
TEST(SocketConnectorTest, TakesOwnershipOfASocketEvenOnFailure) {
- auto H = makeNativeSocket();
- ASSERT_TRUE(H.has_value());
-
- bool AttachInfoRequested = false;
- EXPECT_THAT_ERROR(adoptFD(*H, AttachInfoRequested),
- FailedWithMessage("attach info requested"));
- EXPECT_TRUE(AttachInfoRequested);
+ // A non-stream socket passes the connector's is-a-socket check, so the
+ // connector takes ownership of it, but is then rejected when creating the
+ // ControllerAccess, which requires a stream socket.
+ auto H = makeNativeNonStreamSocket();
+ ASSERT_TRUE(H.has_value()) << "could not create a socket for the test";
+
+ EXPECT_THAT_ERROR(adoptFD(*H),
+ FailedWithMessage(HasSubstr("requires a stream socket")));
EXPECT_FALSE(isNativeSocketOpen(*H))
<< "an adopted socket must be closed when the connection fails";
}
diff --git a/orc-rt/tools/ogre/ogre.cpp b/orc-rt/tools/ogre/ogre.cpp
index 398622275066a..1e9f1f3f12215 100644
--- a/orc-rt/tools/ogre/ogre.cpp
+++ b/orc-rt/tools/ogre/ogre.cpp
@@ -21,6 +21,8 @@
#include "orc-rt/bedrock/ThreadPoolRunner.h"
#include "orc-rt/bedrock/sps/AllSPSCI.h"
+#include "orc-rt-c/support/Logging.h"
+
#include <cstdio>
#include <cstring>
#include <future>
@@ -76,20 +78,24 @@ static std::variant<Options, int> parseArgs(int argc, char *argv[]) noexcept {
return O;
}
-void reportError(Error Err) noexcept {
- fprintf(stderr, "reported error: %s\n", toString(std::move(Err)).c_str());
+static void reportError(Session &S, Error Err) noexcept {
+#if ORC_RT_LOG_ENABLED(Error)
+ Session::logErrors(S, std::move(Err));
+#else
+ fprintf(stderr, "Session %p error: %s\n", &S,
+ toString(std::move(Err)).c_str());
+#endif // ORC_RT_LOG_ENABLED(Error)
}
-void printExecutorProcessInfo(const ExecutorProcessInfo &EPI) noexcept {
- fprintf(stderr,
- "executor info: triple = \"%s\", page-size = %zu, "
- "cpu-features = \"%s\"\n",
- EPI.targetTriple().c_str(), EPI.pageSize(),
- EPI.targetCPUFeatures().c_str());
+static Expected<Session::DispatchFn> makeDispatcher() noexcept {
+ return [R = std::make_unique<ThreadPoolRunner>(4)](Session::Task T) {
+ (*R)(std::move(T));
+ };
}
-Error setupSession(Session &S, const Options &Opts,
- BootstrapInfo &BI) noexcept {
+/// Adds the services a host executor provides, publishing their entry points
+/// in BI for the controller.
+static Error addHostServices(Session &S, BootstrapInfo &BI) noexcept {
if (auto Err = sps_ci::addAll(BI.symbols()))
return Err;
@@ -104,53 +110,59 @@ Error setupSession(Session &S, const Options &Opts,
return Error::success();
}
-Error trySetupAndConnect(Session &S, const Options &Opts) noexcept {
- ConnectorRegistry ConnRegistry;
- if (auto Err = registerSocketConnector(ConnRegistry))
- return Err;
- // registerTCPConnect(ConnRegistry);
+static Expected<std::unique_ptr<Session>>
+makeSession(const Options &Opts) noexcept {
+ auto EPI = ExecutorProcessInfo::Detect();
+ if (!EPI)
+ return EPI.takeError();
- auto BI = BootstrapInfo::CreateDefault(S);
+ if (Opts.Verbose) {
+ fprintf(stderr,
+ "executor info: triple = \"%s\", page-size = %zu, "
+ "cpu-features = \"%s\"\n",
+ EPI->targetTriple().c_str(), EPI->pageSize(),
+ EPI->targetCPUFeatures().c_str());
+ }
+
+ auto D = makeDispatcher();
+ if (!D)
+ return D.takeError();
+
+ auto S =
+ std::make_unique<Session>(std::move(*EPI), std::move(*D), reportError);
+
+ auto BI = BootstrapInfo::CreateDefault(*S);
if (!BI)
return BI.takeError();
- if (auto Err = setupSession(S, Opts, *BI))
+ if (auto Err = addHostServices(*S, *BI))
return Err;
- return ConnRegistry.connect(
- [&]() noexcept -> Expected<ConnectorRegistry::AttachInfo> {
- return ConnectorRegistry::AttachInfo{S, std::move(*BI)};
- },
- Opts.ConnSpec);
-}
+ ConnectorRegistry Connectors;
+ if (auto Err = registerSocketConnector(Connectors))
+ return Err;
-Expected<int> runOgre(const Options &Opts) noexcept {
- // Get the process info.
- auto EPI = ExecutorProcessInfo::Detect();
- if (!EPI)
- return EPI.takeError();
- if (Opts.Verbose)
- printExecutorProcessInfo(*EPI);
+ if (auto Err = Connectors.connect(Opts.ConnSpec, *S, std::move(*BI)))
+ return Err;
- // Build the session.
- ThreadPoolRunner Run(4);
- Session S(
- std::move(*EPI), [&Run](Session::Task T) { Run(std::move(T)); },
- [](Session &, Error Err) noexcept { reportError(std::move(Err)); });
+ return S;
+}
- std::promise<void> StopP;
- auto StopF = StopP.get_future();
- S.setOnDisconnect([StopP = std::move(StopP)](Error Err) mutable noexcept {
- if (Err)
- reportError(std::move(Err));
- StopP.set_value();
- });
+static Expected<int> runOgre(const Options &Opts) noexcept {
+ auto S = makeSession(Opts);
+ if (!S)
+ return S.takeError();
- if (auto Err = trySetupAndConnect(S, Opts))
- return Err;
+ // makeSession has already connected, so the Session may have detached by
+ // now. That's fine: addOnDetach runs the callback immediately if so, so the
+ // wait below cannot miss the detach.
+ std::promise<void> StopP;
+ std::future<void> StopF = StopP.get_future();
+ (*S)->addOnDetach(
+ [StopP = std::move(StopP)]() mutable noexcept { StopP.set_value(); });
+ // Wait for detach.
StopF.get();
-
return 0;
}
More information about the llvm-commits
mailing list