From bb33ca0583cb76f1115e094b10d3462cf824398b Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 27 Jan 2025 12:00:33 -0500 Subject: [PATCH 1/2] Remove Stubs.cpp and reimplement what it has in Swift. This PR removes Stubs.cpp, which currently houses some thunks for functions that are conditionally unavailable in glibc, and replaces it with runtime function lookups in Swift. Is there potentially a one-time non-zero performance cost? Yes. Is that performance cost prohibitive given that the functions are only looked up once and then cached? No. These functions won't get called on Linux if `SWT_NO_DYNAMIC_LINKING` is defined but we don't currently support that combination anyway. Even if you're using a statically-linked Swift standard library, we'd expect Linux to still support calling `dlsym()`. --- Sources/Testing/ExitTests/SpawnProcess.swift | 18 +++++++- Sources/Testing/ExitTests/WaitFor.swift | 15 ++++++- Sources/_TestingInternals/CMakeLists.txt | 1 - Sources/_TestingInternals/Stubs.cpp | 45 -------------------- Sources/_TestingInternals/include/Stubs.h | 19 --------- 5 files changed, 31 insertions(+), 67 deletions(-) delete mode 100644 Sources/_TestingInternals/Stubs.cpp diff --git a/Sources/Testing/ExitTests/SpawnProcess.swift b/Sources/Testing/ExitTests/SpawnProcess.swift index fd18aad8a..8cfc9abd2 100644 --- a/Sources/Testing/ExitTests/SpawnProcess.swift +++ b/Sources/Testing/ExitTests/SpawnProcess.swift @@ -26,6 +26,20 @@ typealias ProcessID = HANDLE typealias ProcessID = Never #endif +#if os(Linux) && !SWT_NO_DYNAMIC_LINKING +/// Close file descriptors above a given value when spawing a new process. +/// +/// This symbol is provided because the underlying function was added to glibc +/// relatively recently and may not be available on all targets. Checking +/// `__GLIBC_PREREQ()` is insufficient because `_DEFAULT_SOURCE` may not be +/// defined at the point spawn.h is first included. +private let _posix_spawn_file_actions_addclosefrom_np = symbol(named: "posix_spawn_file_actions_addclosefrom_np").map { + unsafeBitCast($0, to: (@convention(c) (UnsafeMutablePointer, CInt) -> CInt).self) +} +#endif + +#endif + /// Spawn a process and wait for it to terminate. /// /// - Parameters: @@ -141,11 +155,13 @@ func spawnExecutable( // Close all other file descriptors open in the parent. flags |= CShort(POSIX_SPAWN_CLOEXEC_DEFAULT) #elseif os(Linux) +#if !SWT_NO_DYNAMIC_LINKING // This platform doesn't have POSIX_SPAWN_CLOEXEC_DEFAULT, but we can at // least close all file descriptors higher than the highest inherited one. // We are assuming here that the caller didn't set FD_CLOEXEC on any of // these file descriptors. - _ = swt_posix_spawn_file_actions_addclosefrom_np(fileActions, highestFD + 1) + _ = _posix_spawn_file_actions_addclosefrom_np?(fileActions, highestFD + 1) +#endif #elseif os(FreeBSD) // Like Linux, this platform doesn't have POSIX_SPAWN_CLOEXEC_DEFAULT. // Unlike Linux, all non-EOL FreeBSD versions (≥13.1) support diff --git a/Sources/Testing/ExitTests/WaitFor.swift b/Sources/Testing/ExitTests/WaitFor.swift index 239b4a4ba..fac3e1496 100644 --- a/Sources/Testing/ExitTests/WaitFor.swift +++ b/Sources/Testing/ExitTests/WaitFor.swift @@ -94,6 +94,17 @@ private nonisolated(unsafe) let _waitThreadNoChildrenCondition = { return result }() +#if os(Linux) && !SWT_NO_DYNAMIC_LINKING +/// Set the name of the current thread. +/// +/// This function declaration is provided because `pthread_setname_np()` is +/// only declared if `_GNU_SOURCE` is set, but setting it causes build errors +/// due to conflicts with Swift's Glibc module. +private let _pthread_setname_np = symbol(named: "pthread_setname_np").map { + unsafeBitCast($0, to: (@convention(c) (pthread_t, UnsafePointer) -> CInt).self) +} +#endif + /// Create a waiter thread that is responsible for waiting for child processes /// to exit. private let _createWaitThread: Void = { @@ -152,7 +163,9 @@ private let _createWaitThread: Void = { #if SWT_TARGET_OS_APPLE _ = pthread_setname_np("Swift Testing exit test monitor") #elseif os(Linux) - _ = swt_pthread_setname_np(pthread_self(), "SWT ExT monitor") +#if !SWT_NO_DYNAMIC_LINKING + _ = _pthread_setname_np?(pthread_self(), "SWT ExT monitor") +#endif #elseif os(FreeBSD) _ = pthread_set_name_np(pthread_self(), "SWT ex test monitor") #elseif os(OpenBSD) diff --git a/Sources/_TestingInternals/CMakeLists.txt b/Sources/_TestingInternals/CMakeLists.txt index e72143e63..ed707cd78 100644 --- a/Sources/_TestingInternals/CMakeLists.txt +++ b/Sources/_TestingInternals/CMakeLists.txt @@ -12,7 +12,6 @@ include(LibraryVersion) include(TargetTriple) add_library(_TestingInternals STATIC Discovery.cpp - Stubs.cpp Versions.cpp WillThrow.cpp) target_include_directories(_TestingInternals PUBLIC diff --git a/Sources/_TestingInternals/Stubs.cpp b/Sources/_TestingInternals/Stubs.cpp deleted file mode 100644 index 5fb8b4ff4..000000000 --- a/Sources/_TestingInternals/Stubs.cpp +++ /dev/null @@ -1,45 +0,0 @@ -// -// This source file is part of the Swift.org open source project -// -// Copyright (c) 2024 Apple Inc. and the Swift project authors -// Licensed under Apache License v2.0 with Runtime Library Exception -// -// See https://swift.org/LICENSE.txt for license information -// See https://swift.org/CONTRIBUTORS.txt for Swift project authors -// - -/// This source file includes implementations of functions that _should_ simply -/// be `static` stubs in Stubs.h, but which for technical reasons cannot be -/// imported into Swift when defined in a header. -/// -/// Do not, as a rule, add function implementations in this file. Prefer to add -/// them to Stubs.h so that they can be inlined at compile- or link-time. Only -/// include functions here if Swift cannot successfully import and call them -/// otherwise. - -#undef _DEFAULT_SOURCE -#define _DEFAULT_SOURCE 1 -#undef _GNU_SOURCE -#define _GNU_SOURCE 1 - -#include "Stubs.h" - -#if defined(__linux__) -int swt_pthread_setname_np(pthread_t thread, const char *name) { - return pthread_setname_np(thread, name); -} -#endif - -#if defined(__GLIBC__) -int swt_posix_spawn_file_actions_addclosefrom_np(posix_spawn_file_actions_t *fileActions, int from) { - int result = 0; - -#if defined(__GLIBC_PREREQ) -#if __GLIBC_PREREQ(2, 34) - result = posix_spawn_file_actions_addclosefrom_np(fileActions, from); -#endif -#endif - - return result; -} -#endif diff --git a/Sources/_TestingInternals/include/Stubs.h b/Sources/_TestingInternals/include/Stubs.h index 303cf0c46..8093a3722 100644 --- a/Sources/_TestingInternals/include/Stubs.h +++ b/Sources/_TestingInternals/include/Stubs.h @@ -117,25 +117,6 @@ static char *_Nullable *_Null_unspecified swt_environ(void) { } #endif -#if defined(__linux__) -/// Set the name of the current thread. -/// -/// This function declaration is provided because `pthread_setname_np()` is -/// only declared if `_GNU_SOURCE` is set, but setting it causes build errors -/// due to conflicts with Swift's Glibc module. -SWT_EXTERN int swt_pthread_setname_np(pthread_t thread, const char *name); -#endif - -#if defined(__GLIBC__) -/// Close file descriptors above a given value when spawing a new process. -/// -/// This symbol is provided because the underlying function was added to glibc -/// relatively recently and may not be available on all targets. Checking -/// `__GLIBC_PREREQ()` is insufficient because `_DEFAULT_SOURCE` may not be -/// defined at the point spawn.h is first included. -SWT_EXTERN int swt_posix_spawn_file_actions_addclosefrom_np(posix_spawn_file_actions_t *fileActions, int from); -#endif - #if !defined(__ANDROID__) #if __has_include() && defined(si_pid) /// Get the value of the `si_pid` field of a `siginfo_t` structure. From ca2846133a6d42b9dd4beef670de4293a4451c53 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 27 Jan 2025 12:14:35 -0500 Subject: [PATCH 2/2] Fix typo and update FreeBSD comment talking about Linux --- Sources/Testing/ExitTests/SpawnProcess.swift | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/Sources/Testing/ExitTests/SpawnProcess.swift b/Sources/Testing/ExitTests/SpawnProcess.swift index 8cfc9abd2..2d4a67442 100644 --- a/Sources/Testing/ExitTests/SpawnProcess.swift +++ b/Sources/Testing/ExitTests/SpawnProcess.swift @@ -38,8 +38,6 @@ private let _posix_spawn_file_actions_addclosefrom_np = symbol(named: "posix_spa } #endif -#endif - /// Spawn a process and wait for it to terminate. /// /// - Parameters: @@ -165,9 +163,8 @@ func spawnExecutable( #elseif os(FreeBSD) // Like Linux, this platform doesn't have POSIX_SPAWN_CLOEXEC_DEFAULT. // Unlike Linux, all non-EOL FreeBSD versions (≥13.1) support - // `posix_spawn_file_actions_addclosefrom_np`. Therefore, we don't need - // `swt_posix_spawn_file_actions_addclosefrom_np` to guard the - // availability of this function. + // `posix_spawn_file_actions_addclosefrom_np`, and FreeBSD does not use + // glibc nor guard symbols behind `_DEFAULT_SOURCE`. _ = posix_spawn_file_actions_addclosefrom_np(fileActions, highestFD + 1) #elseif os(OpenBSD) // OpenBSD does not have posix_spawn_file_actions_addclosefrom_np().