Fixes for the compat56 library #63192
Conversation
This patch replaces the platform version check with a dlsym to grab the symbol we need. If we're on a new enough platform, the symbol should be available and we can use it. Otherwise, it will be empty and we should return true. This breaks the link requirement for the platform version symbol.
I seem to have copied over the wrong version of Task.h. There is an ABI mismatch in the size of AsyncContext due to the removal of Flags. This resulted in programs crashing when running against the backdeploy library and should have crashed when running on the swift 5.6 runtime. The successResultPointer pointer was set in AsyncTask::waitFuture, but with the wrong layout. When the pointer was read in the concurrency backdeploy library, it was at a different offset, and thus contained a nullptr. I pulled the AsyncContextKind and AsyncContextFlags from the old MetadataValues.h into Task.h as they were removed in commit aca744b, but are necessary with the flags included.
The 5.5/5.6 concurrency runtime in the concurrency backdeploy library just doesn't have the pieces necessary to emit the desired error message. Disabling it in that environment. Commit afc5116 added the support to track re-used continuations.
|
@swift-ci please test |
The threading library changed in the BackDeployConcurrency library resulting in the `SWIFT_TASK_DEBUG_LOG` macro not compiling. Fixing that. Also adding the logging pieces to the Compatibility 56 library.
Apparently `-fvisibility=hidden` is not sufficient to hide some of the symbols in this library. I've explicitly marked the symbols that were flagged as being incorrectly exported as "hidden" so hopefully no one drags them out again. This is a statically linked library, so the symbols shouldn't need to be exported for this work work. To ensure that this is the case and that we're still hitting the overridden API with all of the hidden symbols, I added a debug log line to the fixed `AsyncTask::waitFuture` implementation and have verified that we see the compat56 log message emitted from executables that need the compat56 library.
etcwilde
force-pushed
the
ewilde/fix-backdeploy-compat56-crash
branch
from
ca54f1a to
a89d20b
Compare
|
@swift-ci please test |
| // _Z19voucher_needs_adoptP9voucher_s | ||
| const auto voucherNeedsAdopt = | ||
| reinterpret_cast<bool(*)(voucher_t)>(SWIFT_LAZY_CONSTANT( | ||
| dlsym(RTLD_DEFAULT, "_Z19voucher_needs_adoptP9voucher_s"))); |
There was a problem hiding this comment.
This is an inline function, are we picking up a copy that got inlined into the main concurrency runtime or something? I'm not sure this actually works.
I will note that return true is always OK, false is just a performance optimization, so worst case we still don't break anything.
| @@ -7,6 +7,9 @@ | |||
| // REQUIRES: concurrency | |||
| // REQUIRES: concurrency_runtime | |||
| // UNSUPPORTED: back_deployment_runtime | |||
|
|
|||
| // Commit afc5116ef0de added support for the error check | |||
| // UNSUPPORTED: back_deploy_concurrency | |||
There was a problem hiding this comment.
I totally didn't think about the difference between this and use_os_stdlib.
| # The compat56 library is not available during a stage-0 compiler build. | ||
| # Once we have proper stage tracking, we should turn this on for stages that | ||
| # are guaranteed to have the compatibility libraries. | ||
| target_compile_options(swiftASTGen PRIVATE |
There was a problem hiding this comment.
Just a question here: why does this matter if the stage 0 build for ASTGen builds against the host tools? Shouldn’t the host driver provide whatever compat libs we shipped in the host’s toolchain?
It’s totally fine to set this since, yeah, we aren’t exactly trying to use bleeding-edge features in ASTGen.
There was a problem hiding this comment.
The issue is that the compatibility library isn't in the host toolchain yet (have to be able to build two major release versions back) , so we don't link it because it doesn't exist, and we end up failing to link due to the missing __swift_FORCE_LOAD_$_swiftCompatibility56 symbol;
__swift_FORCE_LOAD_$_swiftCompatibility56", referenced from:
__swift_FORCE_LOAD_$_swiftCompatibility56_$_swiftASTGen in ASTGen.swift.o
__swift_FORCE_LOAD_$_swiftCompatibility56_$_swiftASTGen in Decls.swift.o
__swift_FORCE_LOAD_$_swiftCompatibility56_$_swiftASTGen in Diagnostics.swift.o
__swift_FORCE_LOAD_$_swiftCompatibility56_$_swiftASTGen in Exprs.swift.o
__swift_FORCE_LOAD_$_swiftCompatibility56_$_swiftASTGen in Generics.swift.o
__swift_FORCE_LOAD_$_swiftCompatibility56_$_swiftASTGen in Literals.swift.o
__swift_FORCE_LOAD_$_swiftCompatibility56_$_swiftASTGen in Macros.swift.o
There was a problem hiding this comment.
Wait a sec, I see your point. Looking back at the logs, it's the bootstrapping1 compiler that's failing to link. That makes more sense. The stage-0 compiler knows about this library and adds the symbols. Then the stage-0 compiler tries linking the stage-1 compiler against the hostlibs instead of the matching libraries, which don't have the compat56 library, and so that fails.
|
Haven't heard any responses in two weeks. Going to go ahead and merge after re-running the tests. |
|
@swift-ci please test |
|
I might be able to re-enable combat in these tests after these changes; |
The runtime compat 5.6 library is not available in current toolchains. The stage-0 compiler builds fine since the builder compiler is not aware of the 5.6 library, but the stage-1 compiler will not link since the builder libraries don't have the compat 5.6 library, while the stage-0 compiler is aware of the library and attempts to link it into the built products. The issue is that the bootstrap goes and uses the libraries off of the builder instead of the things that actually match the compiler. This is currently safe because the 5.6 compat library only contains concurrency runtime fixes and the compiler frontend does not use concurrency yet. If this changes, we will need to re-enable this and make the bootstrap use the libraries correctly.
etcwilde
force-pushed
the
ewilde/fix-backdeploy-compat56-crash
branch
from
a89d20b to
c1ef992
Compare
|
@swift-ci please smoke test |
This patch contains a myriad of small but important fixes for the compat56 library.
This is a static library, and therefore was not triggering linking against compiler-rt. This resulted in a failure for binaries to launch since they were missing the
_platformIsAtLeastsymbol required for the availability check inswift_voucher_needs_adopt. To avoid the availability check for the underlyingvoucher_needs_adoptfunction, I just usedlsymto see if we have the symbol available. Since this library has to run on systems that both do and don't have it, this check must be done dynamically, so this seemed like the most reasonable way to detect whether we have it or not without needing the platform version check.This way builds do not need to be changed to run with the compat library.
AsyncContextlayout change:I apparently copied the Task.h from the wrong place, and we had a small ABI change in
AsyncContextbetween 5.6/5.5/backdeploy and the 5.7 runtime that I copied the header from.AsyncContextlost theAsyncContextFlagsfield in 5.7, resulting in lots of carnage. I feel sorry for the person who has to do the next compat library, but for now, this should work. In debugging this, I found that theSWIFT_TASK_DEBUG_LOGwas not working, so I fixed that up.Additionally, I found that the
continuation_validationtest was breaking. Note that I think the only reason it was running was because I was running a very cursed system. It looks like it doesn't run on a reasonable 10.15/11.0 system, but it does when you shove the runtimes from 10.15 into macOS 13.1. The test crashes, as we expect, but doesn't have the error message because the backdeploy-concurrency runtime simply does not have the pieces. I added anUNSUPPORTEDline for that.rdar://104084276
rdar://104241404
Apparently the
-fvisibility=hiddenbeing passed to the compiler is not sufficient to hide all the symbols. I had some complaints about this library exposing symbols. I've gone through and labelled all of the API's that tooling was reporting was being exported that shouldn't have been. Here's to hoping that works. I'd like to avoid having to annotate every function with the hidden visibility attribute.rdar://104237736
rdar://104237749
The stage-0 ASTGen build doesn't have a compatibility library to link against. I've disabled the runtime compatibility mode on the library until we have better stage tracking for the compiler as a whole. This is safe enough since ASTGen doesn't use concurrency and that's the only part with fixes being back deploy in the compat56 library.
rdar://104576917