[Build][Runtime] Add a new threading library. #59287
Conversation
|
@swift-ci Please test |
|
preset=buildbot,tools=RA,stdlib=RDA |
SWIFT_STDLIB_SINGLE_THREADED_RUNTIME is too much of a blunt instrument here. It covers both the Concurrency runtime and the rest of the runtime, but we'd like to be able to have e.g. a single-threaded Concurrency runtime while the rest of the runtime is still thread safe (for instance). So: rename it to SWIFT_STDLIB_SINGLE_THREADED_CONCURRENCY and make it just control the Concurrency runtime, then add a SWIFT_STDLIB_THREADING_PACKAGE setting at the CMake/build-script level, which defines SWIFT_STDLIB_THREADING_xxx where xxx depends on the chosen threading package. This is especially useful on systems where there may be a choice of threading package that you could use. rdar://90776105
Read/write locks are not as good as you'd think; a simple mutex is better in almost all cases. rdar://90776105
Moved all the threading code to one place. Added explicit support for Darwin, Linux, Pthreads, C11 threads and Win32 threads, including new implementations of Once for Linux, Pthreads, C11 and Win32. rdar://90776105
The threading unit tests currently just check the operation of Mutex. This used to be part of the runtime tests, but now it's a separate library we can test it separately. rdar://90776105
Added explicit tests for `swift::once` and also for the ulock implementation used on Linux. rdar://90776105
The file is called Nothreads.cpp, not NoThreads.cpp. rdar://90776105
Some things (LLDB's language plugin, for instance), include Swift headers from outside of our build system, and may not know to define one of the threading package defines. rdar://90776105
Some declarations were missing from non-Darwin sources, and the Linux header was slightly broken. rdar://90776105
A few fixes specifically for the Linux build. rdar://90776105
We want this initialized when we decide which threading package to use. This isn't a problem on Darwin or Linux where we're using the build script, but on Windows it relies on the defaulting mechanism, which didn't happen until further down the `CMakeLists.txt`. rdar://90776105
When we static link against libswiftCore, we need it to contain the actual object modules from the threading library (when it's dynamically linked, the shared object will already contain those modules because it's been linked against libswiftThreading), which means that the threading library in the stdlib needs to be an OBJECT_LIBRARY rather than a STATIC library (otherwise people will have to statically link against the threading library as well, and that'll need to be installed, and we don't want that). rdar://90776105
I'd renamed the setting, but failed to update build-presets.ini. rdar://90776105
Apparently I missed a close brace from the end of Win32.h. rdar://90776105
Looks like I forgot to update the Win32.h implementation. rdar://90776105
rdar://90776105
`windows.h` defines a couple of very unhelpfully named macros. Undefine them after including it. rdar://90776105
We're using some functions from Synchronization.lib now, after the threading changes. rdar://90776105
We can't safely include <windows.h> because it defines a large number of macros, some of which clash with things in the Swift source tree, and others of which clash with things in the LLVM source tree. Sadly we *also* can't just include the Windows headers we need, because they pull in some of the problematic macros. In this instance, the best thing seems to be to grab the definitions for the types and functions we are going to use and put them in their own header file. If we define them correctly, then #including <windows.h> before or after this header won't have any adverse effects. rdar://90776105
We shouldn't include <windows.h> implicitly from .cpp files, but should do it directly so that we know it's there. Also, if we're including <windows.h>, do it at the top of the file. rdar://90776105
The concurrency library can use the new threading library functions, which avoids the problem of including <windows.h>. rdar://90776105
Declaring _RTL_SRWLOCK ourselves causes clashes with <windows.h>. Rather than doing that, declare an equivalent struct, and some overloads. rdar://90776105
Actor.cpp does need <io.h> still, and Task.cpp should have been including <windows.h> rdar://90776105
TaskLocal.cpp doesn't need <handleapi.h> or <processthreadsapi.h>, both of which want <windows.h>, which isn't included any more. rdar://90776105
We need to link the threading unit test against Synchronization.lib to pick up the Win32 APIs we're using. rdar://90776105
The runtime unit tests also need to link with Synchronization.lib. rdar://90776105
Removing thread_get_main() means we don't need a static initializer on Darwin, which means we can delete Darwin.cpp as well. We can also delete Nothreads.cpp while we're about it, because there's nothing in that file. rdar://90776105
Just formatting changes. rdar://90776105
These changes are needed to get things building with a C11 threads shim header on macOS. rdar://90776105
…d special. While most systems aren't going to have their stack bottom at zero, using llvm::Optional<> here is cleaner. rdar://90776105
When we're building static libraries, we don't need to include the threading objects in both Concurrency and Core, and we also don't need two copies of the threading library's fatal error handler. rdar://90776105
`SWIFT_BUILD_STATIC_STDLIB` is not mutually exclusive with `SWIFT_BUILD_DYNAMIC_STDLIB`, and Linux sets both, so we can't use `SWIFT_BUILD_STATIC_STDLIB` to conditionalise things. The linker error about duplicate definitions for the threading error handler was happening because we were forced to include the object containing that symbol; moving it to another object should fix that. And it turns out there's a way to get CMake to include the threading object library only when building a shared object, so use that too. rdar://90776105
`gettid` wrapper was added in glibc 2.30, and Ubuntu 18.04 uses glibc 2.27, so use raw syscall instead of it.
The threading code is required by some of the compatibility modules, but we weren't pulling it in unless we were building the standard library. That breaks the build when the standard library build is disabled, so explicitly include it. rdar://90776105
al45tair
force-pushed
the
eng/PR-90776105-2
branch
from
b9af6b9 to
c46d4af
Compare
|
@swift-ci Please test |
|
preset=buildbot,tools=RA,stdlib=RDA |
al45tair
force-pushed
the
eng/PR-90776105-2
branch
from
ed93897 to
c46d4af
Compare
A copy of the older Concurrency sources was just added to BackDeployConcurrency, replacing the `add_subdirectory(../Concurrency)` trick it was using previously. These need to be adjusted to work with the threading library. Fortunately, the necessary adjustments are the same as those required for the existing Concurrency library, give or take. rdar://90776105
|
@swift-ci Please test |
|
preset=buildbot,tools=RA,stdlib=RDA |
|
@swift-ci Please test Ubuntu 18.04 platform |
|
@swift-ci Please test Amazon Linux 2 platform |
|
@swift-ci Please test Centos 7 platform |
|
Ah, apparently the specific Linux platform CI commands aren't supported presently. |
|
🥳 |
|
I'm seeing a static assert failure on Android armv7 since this was merged, is it possible this pull broke building the stdlib for 32-bit platforms? |
|
@buttaface Ah, yes, sorry, it looks like I did break 32-bit Linux. Should be a straightforward enough fix, I think. PR to fix: #59371 |
| precondition(SWIFT_HOST_VARIANT_SDK) | ||
| precondition(SWIFT_DARWIN_PLATFORMS) | ||
|
|
||
| string(TOUPPER "${SWIFT_THREADING_PACKAGE}" package) |
There was a problem hiding this comment.
I think setting this to SWIFT_THREADING_PACKAGE by default is a mistake, as it is set to the host platform by default. Since it's possible to cross-compile multiple non-host stdlibs, that means it will build all of them with the host threading library name.
I'm not sure how the couple lines just below are supposed to work either, how are you supposed to pass an empty string to build-script? I tried various things like --swift-threading-package="" and nothing worked. Even if it did, you only have threading_package_default() below taking one variable, when it needs two.
This just hit me because I cross-compile the Android armv7 SDK and it failed because it used pthreads, which doesn't support 32-bit here.
I was confused though because it built fine on the community Android armv7 CI, then I dug into it and it's because the host SDK there is LINUX so it applies that threading library to Android armv7 too, based on the default logic above. My own Android CI doesn't build for the linux host though, only for Android armv7 with the flags --cross-compile-hosts=android-armv7 --skip-local-build, so the host default logic chooses pthreads for the Android host and fails for armv7 only.
That suggests two fixes:
- Rethink the way the threading library is chosen by default for the stdlib, so you're not using the host threading library name for all stdlibs built.
- Add Android to the list of SDKs where a default threading library is chosen, and set a default.
Let me know what you think, @al45tair, you may want to get pthreads working for 32-bit too.
There was a problem hiding this comment.
Making pthreads work for 32-bit is a fairly trivial fix, I think, and probably worth doing (possibly the right thing is to use uintptr_t instead of uint64_t for the type of the swift::once_t), though maybe we should be picking the linux setting on Android?
The swift-threading-package setting isn't presently supposed to be explicitly set to empty, which is why that doesn't work. It's there to let you override it and explicitly specify what you want, not the opposite (not sure why you'd need to do that). If you're looking at the headers and have seen the bit in the header file where it tries to guess, that has to be there because of lldb, rather than because I thought that was a good idea.
As for the cross compilation problem, could you file a separate issue for that? The threading code gets used both for the host and the target, so I think you're right that the automatic detection might indeed need to be tweaked for the target platform.
There was a problem hiding this comment.
maybe we should be picking the linux setting on Android?
I'm not sure: both appear to work on 64-bit platforms, so I will look into it and let you know.
The swift-threading-package setting isn't presently supposed to be explicitly set to empty, which is why that doesn't work.
I thought you explicitly included the empty string in this list of threading options to allow people to set it, but I think the bash check a couple lines above wouldn't allow it through anyway.
It's there to let you override it and explicitly specify what you want, not the opposite (not sure why you'd need to do that)
OK, this check that I mentioned will never be true then? I thought the intent was for it to signify "auto-detect" for each stdlib, but it currently doesn't do that either.
As for the cross compilation problem, could you file a separate issue for that?
Will doDone, #59568.
This repairs the Windows x86 SDK build after swiftlang#59287. Use `intptr_t` rather than the explicitly sized integer as this is always guaranteed to be the size of the pointer width (irrespective of LP64/LP32 or LLP64/LLP32). Since this impacts only the once predicate, this shouldn't impact any assumptions around structure sizes.
A fair bit of this is really shuffling things around — that's largely true for the
swift::Mutex, which was an existing thing.swift::Threadis really a placeholder for when we need more support — right now, we don't need any of the functionality that e.g.std::threadhas, but we do need some functionality it doesn't have. It's also desirable to be able to choose which threading package to use, especially for embedded use or for testing purposes, and if we make everything defer to the C++ library we'll only be able to use whatever the C++ library is using.The main new things are:
#includes of implementation files in order to bring them in to the standard library or concurrency code.swift::Lazynow usesswift::oncerather than having its own implementation that uses a macro.swift::onceimplementations for Linux and Windows that should be somewhat faster on those platforms.rdar://90776105