diff --git a/CMakeLists.txt b/CMakeLists.txt index baf87963..e35ac8d7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -953,6 +953,29 @@ if (MOBILEGL_BUILD_DISAGGREGATED AND NOT WIN32) target_compile_options(MobileGLServer PRIVATE -fPIE) target_link_options(MobileGLServer PRIVATE -pie) + # $ORIGIN, OR THE SERVER CANNOT START AT ALL ON ANDROID. + # + # This executable links libMobileGL.so, and it is EXEC'd rather than dlopen'd - so it gets a + # fresh linker namespace with the default search path, not the app's. An APK's lib// is + # not on that path, and the failure is the bluntest one there is: + # + # F linker: CANNOT LINK EXECUTABLE ".../lib/arm64/libMobileGLServer.so": + # library "libMobileGL.so" not found: needed by main executable + # + # It cost a device run to find because it is invisible on desktop: CMake gives build-tree + # targets an absolute RPATH automatically, so the WSL server resolved its dependency through a + # path that does not exist on a phone. $ORIGIN says "beside me", which is true in the APK, true + # in the build tree, and true wherever somebody copies the pair - and it is what makes a + # deployment that is NOT a build tree work at all, which the end state is. + # THE PROPERTIES, NOT target_link_options. A raw `-Wl,-rpath,$ORIGIN` reaches the generator + # unquoted and the SHELL expands $ORIGIN to nothing - measured on device, where the linker + # said `normalize_path - invalid input: "$"` and then failed to find libMobileGL.so anyway. + # BUILD_RPATH / INSTALL_RPATH are the supported spelling and CMake escapes them correctly. + set_target_properties(MobileGLServer PROPERTIES + BUILD_RPATH "$ORIGIN" + INSTALL_RPATH "$ORIGIN" + BUILD_WITH_INSTALL_RPATH TRUE) + if (ANDROID) # An APK's only exec-able install location is lib//, and the packager # only puts a file there if it is named lib*.so. P0 spike A proved this diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 54a4262c..d6d981bf 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -625,10 +625,15 @@ namespace MobileGL::MG_Remote::Client { // a NAME; making it per-process-per-instant is what stops two runs on // one machine - which `ctest -j` produces by construction - finding each // other's server. - const std::string endpoint = std::string("/tmp/mgl-") + + // AN ABSTRACT NAME ('@'), not a path under /tmp. An Android app has no + // writable /tmp and the first device run proved it the blunt way: the + // server launched, bind() had nowhere to put the node, and the client + // refused by name 20 s later. The abstract namespace needs no directory, + // no mode bits and no cleanup after a crash, and its reach - the network + // namespace - is exactly one app's processes. + const std::string endpoint = std::string("@mgl-") + std::to_string(static_cast(::getpid())) + "-" + - std::to_string(reinterpret_cast(this)) + - ".sock"; + std::to_string(reinterpret_cast(this)); const MobileGLResult launched = Server::LaunchServer(std::string(MG_Config::Ipc.ServerPath.c_str()), endpoint, diff --git a/MobileGL/MG_Remote/Server/ServerSpawn.cpp b/MobileGL/MG_Remote/Server/ServerSpawn.cpp index 92008a27..74f219ab 100644 --- a/MobileGL/MG_Remote/Server/ServerSpawn.cpp +++ b/MobileGL/MG_Remote/Server/ServerSpawn.cpp @@ -96,7 +96,7 @@ namespace MobileGL::MG_Remote::Server { int removed = 0; }; - ScrubbedEnv BuildChildEnv() { + ScrubbedEnv BuildChildEnv(const std::string& imageDir) { // `environ` is glibc/bionic's, declared by at global // scope. Re-declaring it inside this anonymous namespace made it a // NEW symbol that nothing defines - which the linker caught, and @@ -115,6 +115,29 @@ namespace MobileGL::MG_Remote::Server { // ยง3.1 is the whole argument for why those are two axes. env.storage.emplace_back("MOBILEGL_IPC_ROLE=server"); env.storage.emplace_back("MOBILEGL_IPC_DIAL=no"); + // LD_LIBRARY_PATH, BECAUSE THE SERVER IS EXEC'd AND NOT dlopen'd. + // + // It links libMobileGL.so, and an exec'd process gets a FRESH linker + // namespace with the default search path - not the one the app built + // for its own libraries. An APK's lib// is not on that path, and + // the failure is total and immediate: + // + // CANNOT LINK EXECUTABLE ".../libMobileGLServer.so": + // library "libMobileGL.so" not found: needed by main executable + // + // It is invisible on desktop, where CMake gives build-tree targets an + // absolute RPATH of their own - so the WSL server had been resolving + // its dependency through a path no phone has. + // + // $ORIGIN WOULD BE THE TIDIER ANSWER and the target asks for it, but + // the NDK toolchain drops RPATH from the link: measured with readelf + // on the APK's own copy, which carries NEEDED libMobileGL.so and no + // RUNPATH at all. So the launcher states it instead - it is the one + // component that has already resolved where the image lives, and a + // pair that is not co-located is not a pair. + if (!imageDir.empty()) { + env.storage.emplace_back("LD_LIBRARY_PATH=" + imageDir); + } env.pointers.reserve(env.storage.size() + 1); for (auto& entry : env.storage) { env.pointers.push_back(entry.data()); @@ -183,7 +206,10 @@ namespace MobileGL::MG_Remote::Server { return MOBILEGL_ERR_INVALID_ARGUMENT; } - ScrubbedEnv env = BuildChildEnv(); + // The image's directory, which is where its libMobileGL.so is. + const std::string::size_type slash = image.find_last_of('/'); + ScrubbedEnv env = + BuildChildEnv(slash == std::string::npos ? std::string() : image.substr(0, slash)); std::string argv0 = image; std::string argv1 = endpoint; char* argv[] = {argv0.data(), argv1.data(), nullptr}; diff --git a/MobileGL/MG_Remote/Transport/SocketTransport.cpp b/MobileGL/MG_Remote/Transport/SocketTransport.cpp index e7d277b7..5271dca7 100644 --- a/MobileGL/MG_Remote/Transport/SocketTransport.cpp +++ b/MobileGL/MG_Remote/Transport/SocketTransport.cpp @@ -70,6 +70,27 @@ namespace MobileGL::MG_Remote::Transport { // does not fit rather than silently truncated - a truncated path is a // different socket, and both sides would then "succeed" onto two // different names. + // A LEADING '@' MEANS THE ABSTRACT NAMESPACE, which is the convention systemd and D-Bus + // use and the only rendezvous that works on Android. + // + // A filesystem AF_UNIX endpoint needs a writable directory, and an Android app has no + // /tmp - measured, not assumed: the first device run of the spawn arm launched its server + // fine and then died on `could not connect to "/tmp/mgl-15731-...sock" ... No such file or + // directory`, because bind() in the child had nowhere to put the node. The app's own cache + // dir would work, but only if every integrator remembered to say where it is, and the + // library has no way to ask. + // + // An abstract name has no filesystem presence at all: no directory, no mode bits, no stale + // node to unlink after a crash - which is the bind() failure `Listen` currently works + // around. It is scoped to the network namespace, so the two processes of ONE app find each + // other and nothing else can, which is exactly the reach this needs. + // + // Linux-only by construction. A path without '@' still works everywhere, and that is what + // an explicit endpoint (P12's "connect to something somebody else is listening on") uses. + bool IsAbstractEndpoint(const std::string& path) { + return !path.empty() && path[0] == '@'; + } + bool FillAddress(const std::string& path, sockaddr_un* out, socklen_t* outLen) { std::memset(out, 0, sizeof(*out)); out->sun_family = AF_UNIX; @@ -79,6 +100,16 @@ namespace MobileGL::MG_Remote::Transport { path.size(), sizeof(out->sun_path), path.c_str()); return false; } + if (IsAbstractEndpoint(path)) { + // sun_path[0] STAYS NUL and the rest is the name. The length carries the name's + // end - there is no terminator - so it is offsetof + 1 + (size - 1), which is + // offsetof + size. Getting this wrong does not fail: it binds a DIFFERENT name + // (one padded with NULs), and the peer then connects to something that is not + // there. + std::memcpy(out->sun_path + 1, path.c_str() + 1, path.size() - 1); + *outLen = static_cast(offsetof(sockaddr_un, sun_path) + path.size()); + return true; + } std::memcpy(out->sun_path, path.c_str(), path.size()); *outLen = static_cast(offsetof(sockaddr_un, sun_path) + path.size() + 1); return true; @@ -132,8 +163,13 @@ namespace MobileGL::MG_Remote::Transport { // A stale socket file from a server that died is the normal case, not the // exceptional one, and bind() fails on it with EADDRINUSE. Unlinking first // is what every AF_UNIX server does; the alternative is a server that - // cannot restart until someone cleans up by hand. - ::unlink(path.c_str()); + // cannot restart until someone cleans up by hand. An ABSTRACT name has no + // node to go stale and unlink("@...") would delete a file of that literal + // name if one happened to exist. + const bool abstractEndpoint = IsAbstractEndpoint(path); + if (!abstractEndpoint) { + ::unlink(path.c_str()); + } const int listenFd = ::socket(AF_UNIX, SOCK_STREAM, 0); if (listenFd < 0) { @@ -148,12 +184,18 @@ namespace MobileGL::MG_Remote::Transport { } // 0600: the rendezvous is between one user's processes. An AF_UNIX path // is a filesystem object and its mode is the only access control there is. - ::chmod(path.c_str(), 0600); + // An abstract name is not a filesystem object and has none; its reach is + // the network namespace, which for an Android app is the app. + if (!abstractEndpoint) { + ::chmod(path.c_str(), 0600); + } if (::listen(listenFd, 4) != 0) { WireLogError("MG_Remote SocketTransport: listen(\"%s\") failed: %s", path.c_str(), std::strerror(errno)); ::close(listenFd); - ::unlink(path.c_str()); + if (!abstractEndpoint) { + ::unlink(path.c_str()); + } return MOBILEGL_ERR_UNSUPPORTED; } *outListenFd = listenFd; diff --git a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp index 625dca6f..f65ef91f 100644 --- a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp @@ -191,6 +191,37 @@ TEST(ServerSpawnTest, AnEglControlOpCrossesToTheOtherProcessAndAnswers) { ASSERT_EQ(Server::ReapServer(session.server, 5000, &exitCode), MOBILEGL_OK); } +TEST(ServerSpawnTest, AnAbstractEndpointNeedsNoWritableDirectoryAndLeavesNoFile) { + // THE ONLY RENDEZVOUS THAT WORKS ON ANDROID, and the reason is not a preference. + // An app has no writable /tmp; the first device run of the spawn retrace launched + // its server, bind() had nowhere to put the node, and the client refused by name + // 20 s later. ClientSession::StartSpawned mints '@' names now, so this pins the + // shape that path depends on. + // + // TWO FACTS, and the second is what a filesystem endpoint cannot give: the + // handshake works over it, and NOTHING IS LEFT BEHIND - no node to go stale, no + // directory to be writable, no unlink to forget after a crash. + const std::string endpoint = std::string("@mgl-abstract-") + std::to_string(::getpid()); + + Session session; + ASSERT_EQ(Server::LaunchServer(ServerImage(), endpoint, &session.server), MOBILEGL_OK); + ASSERT_EQ(Transport::SocketTransport::ConnectTo(endpoint, 10000, session.client), MOBILEGL_OK); + ASSERT_EQ(Handshake(session), MOBILEGL_OK) + << "an abstract endpoint carried the connection but not the session"; + + // A leading '@' is a NAME, not a path. If FillAddress had treated it as one, the + // bind would have created a file literally called "@mgl-..." in the working + // directory - which would still have worked here, and would have failed on the + // one platform this exists for. + EXPECT_NE(::access(endpoint.c_str(), F_OK), 0) + << "an abstract name must leave no filesystem node: " << endpoint; + + Client::ClientSessionInstance().Stop(); + int exitCode = -1; + ASSERT_EQ(Server::ReapServer(session.server, 5000, &exitCode), MOBILEGL_OK); + EXPECT_EQ(exitCode, 0); +} + TEST(ServerSpawnTest, AnUnresolvableImageIsANamedRefusalAndStartsNothing) { const int before = Server::CountOwnChildren(); diff --git a/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp b/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp index 08320ad4..f77a6dc1 100644 --- a/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp +++ b/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp @@ -170,9 +170,21 @@ Java_top_mobilegl_plugin_trace_TraceReplayActivity_nativeRunTraceReplay(JNIEnv* request.benchmarkResultPath = ToString(env, benchmarkResultPath); request.envOverrides = SplitSemicolonList(ToString(env, envOverrides)); - const bool needsNativeWindow = - request.backend == "DirectVulkan" || - (request.backend == "DirectGLES" && !request.usePbuffer); + // THE SURFACE SHAPE DECIDES THIS, NOT THE BACKEND. DirectVulkan used to demand a native + // window unconditionally, which made `use_pbuffer` a DirectGLES-only knob in practice: the + // Android GLWS creates a window surface whenever gNativeWindow is non-null + // (apitrace_glws_android.cpp's createSurface), so handing Magma a window was the same as + // asking for one. + // + // P6 needs the pbuffer path on BOTH backends. Until P12 a spawned server cannot be given a + // window at all - an ANativeWindow* is a pointer into the CLIENT's process, and + // SetWindowHandle is refused on the way across with Fatal{UnmigratedSurface, + // "AndroidNativeWindow@P12"} (Rule H). Measured: the DirectVulkan spawn arm died on exactly + // that, one op after the server had come up green, while DirectGLES with the same flag passed. + // + // Every existing caller is unaffected: nobody asks for a Vulkan pbuffer today, and + // !usePbuffer reproduces the old answer for both backends in every other combination. + const bool needsNativeWindow = !request.usePbuffer; ANativeWindow *window = needsNativeWindow && surface != nullptr ? ANativeWindow_fromSurface(env, surface) : nullptr; if (needsNativeWindow && window == nullptr) { auto result = MakeFailureResult(request, mobilegl_trace::STATUS_RETRACE_FAILED, diff --git a/android-plugin/trace-replay-ci.sh b/android-plugin/trace-replay-ci.sh index 9f4027c8..b508cbda 100644 --- a/android-plugin/trace-replay-ci.sh +++ b/android-plugin/trace-replay-ci.sh @@ -44,7 +44,7 @@ Set MOBILEGL_ESPRYT_USE_ANGLE=1 to run DirectGLES replay with packaged ANGLE instead of the device system GLES driver. Set MOBILEGL_TRACE_ANGLE_VARIANT to the packaged ANGLE short hash used by DirectGLES replay. -Set MOBILEGL_RETRACE_USE_PBUFFER=1 or pass --use-pbuffer to run DirectGLES +Set MOBILEGL_RETRACE_USE_PBUFFER=1 or pass --use-pbuffer to run against an offscreen EGL pbuffer instead of the Activity surface. Set MOBILEGL_MAGMA_FIX_ITERATIONRP_SUBGROUP_SCRATCH=1, MOBILEGL_MAGMA_DERIVE_NUM_SUBGROUPS=1, and MOBILEGL_MAGMA_ITERATIONRP_FIX_BARRIER=1 to @@ -366,7 +366,7 @@ run_retrace() { use_angle=1 test -n "${MOBILEGL_TRACE_ANGLE_VARIANT:-}" || die "MOBILEGL_TRACE_ANGLE_VARIANT is required for DirectGLES ANGLE replay" fi - if [ "${MOBILEGL_RETRACE_USE_PBUFFER:-}" = "1" ] && [ "${backend}" = "DirectGLES" ]; then + if [ "${MOBILEGL_RETRACE_USE_PBUFFER:-}" = "1" ]; then use_pbuffer=1 fi @@ -392,7 +392,13 @@ run_retrace() { set -- "$@" --ez use_angle true set -- "$@" --es angle_variant "${MOBILEGL_TRACE_ANGLE_VARIANT}" fi - if [ "${use_pbuffer}" -eq 1 ] && [ "${backend}" = "DirectGLES" ]; then + # PBUFFER IS A SURFACE SHAPE, NOT A BACKEND PROPERTY, and the DirectGLES condition that used to + # guard this line made it one. It was harmless while the only caller was the ANGLE lane; P6 needs + # it on BOTH backends, because until P12 the spawn arm can only use pbuffer/surfaceless - an + # ANativeWindow* is a pointer into the CLIENT's process and SetWindowHandle is refused by name + # with Fatal{UnmigratedSurface, "AndroidNativeWindow@P12"} on the way across. Magma creates an + # EGL pbuffer exactly as Espryt does. + if [ "${use_pbuffer}" -eq 1 ]; then set -- "$@" --ez use_pbuffer true fi if [ "${avoid_angle_llvmpipe_sampler_mipmap_min_filter}" -eq 1 ] && [ "${backend}" = "DirectGLES" ]; then diff --git a/tools/trace_replay/run_android_retrace_local.py b/tools/trace_replay/run_android_retrace_local.py index 46f54069..f08a188f 100644 --- a/tools/trace_replay/run_android_retrace_local.py +++ b/tools/trace_replay/run_android_retrace_local.py @@ -19,7 +19,17 @@ SUMMARY_HTML = "mobilegl-android-retrace-overview.html" DEFAULT_ANGLE_VARIANT = "ec889e6ea831" BLISS_ANGLE_VARIANT = "90a62123d794" BLISS_CASE = "minecraft-1.21.4-fabric-iris-bliss-in-world" -TRACE_APK_DIR = ROOT / "android-plugin" / "app" / "build" / "outputs" / "apk" / "trace" / "debug" +# BOTH OUTPUT DIRECTORIES, newest wins. The trace variant is assembled as a RELEASE build +# (`:app:assembleTraceRelease`) and Gradle puts it under apk/trace/release, but CI has historically +# staged the same artifact under apk/trace/debug - so a tree that has built locally has two, and +# looking in one of them silently retraces the other one's APK. That is the "ran the wrong binary +# and went green" shape, on the platform where it is hardest to notice: nothing about the result +# says which APK produced it. +TRACE_APK_DIRS = ( + ROOT / "android-plugin" / "app" / "build" / "outputs" / "apk" / "trace" / "release", + ROOT / "android-plugin" / "app" / "build" / "outputs" / "apk" / "trace" / "debug", +) +TRACE_APK_DIR = TRACE_APK_DIRS[1] BACKENDS = { "DirectGLES": { @@ -46,7 +56,11 @@ def is_lfs_pointer(path): def find_trace_apk(): - candidates = list(TRACE_APK_DIR.glob("MobileGL-plugin-trace-release-*.apk")) + candidates = [ + path + for directory in TRACE_APK_DIRS + for path in directory.glob("MobileGL-plugin-trace-release-*.apk") + ] return max(candidates, key=lambda path: path.stat().st_mtime) if candidates else None @@ -120,14 +134,16 @@ def render_summary(): shutil.copyfile(SUMMARY_DIR / SUMMARY_HTML, SUMMARY_DIR / "index.html") -def run_case(case, backend, extra_args=None, timeout_seconds=None, env_overrides=None): +def run_case(case, backend, extra_args=None, timeout_seconds=None, env_overrides=None, + use_pbuffer=False): backend_info = BACKENDS[backend] apk = find_trace_apk() trace_archive = FIXTURES / case["trace_archive"] golden = FIXTURES / case["golden"] alternate = FIXTURES / case["alternate_golden"] if case.get("alternate_golden") else None if apk is None: - mark_skipped(case, backend, f"SKIPPED_MISSING_APK: no trace APK found under {TRACE_APK_DIR}") + mark_skipped(case, backend, "SKIPPED_MISSING_APK: no trace APK found under " + + " or ".join(str(directory) for directory in TRACE_APK_DIRS)) return 2 if not trace_archive.exists() or is_lfs_pointer(trace_archive): mark_skipped(case, backend, "SKIPPED_LFS_POINTER: trace archive is missing or still an LFS pointer") @@ -182,7 +198,7 @@ def run_case(case, backend, extra_args=None, timeout_seconds=None, env_overrides command.extend(extra_args or []) if alternate is not None: command[command.index("--target-call"):command.index("--target-call")] = ["--alternate-golden", bash_path(alternate)] - if backend_info["use_pbuffer"]: + if backend_info["use_pbuffer"] or use_pbuffer: command.append("--use-pbuffer") if backend_info["use_angle"] and case["name"] == BLISS_CASE: command.append("--avoid-angle-llvmpipe-sampler-mipmap-min-filter") @@ -403,6 +419,15 @@ def parse_args(): help="Do not glFinish at every frame boundary, so frame times measure CPU submission " "only instead of GPU completion.", ) + parser.add_argument( + "--use-pbuffer", + action="store_true", + help="Render into a pbuffer instead of the Activity's window surface. REQUIRED with " + "--transport spawn until P12: an ANativeWindow* is a pointer into the CLIENT's " + "process and means nothing in the server's, so SetWindowHandle is refused by name " + "with Fatal{UnmigratedSurface, \"AndroidNativeWindow@P12\"} (Rule H). The desktop " + "retrace has always run pbuffer and matches the same goldens.", + ) parser.add_argument( "--transport", choices=("monolith", "inproc", "spawn"), @@ -457,7 +482,8 @@ def main(): failures += run_benchmark_case(case, backend, args) continue print(f"=== Android retrace: {case['name']} / {backend} ===", flush=True) - rc = run_case(case, backend, env_overrides=transport_env(args) + list(args.env)) + rc = run_case(case, backend, env_overrides=transport_env(args) + list(args.env), + use_pbuffer=args.use_pbuffer) try: render_summary() except Exception as error: