[5.8] Fix backdeploy compat-56 ABI #63531
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
The runtime compat libraries are not available during the stage-0 compiler ASTGen build. As a result, the compiler fails to link because the compat symbols are not available. Once we have better stage tracking, we should turn this on for stages after the compat56 library has been built. Turning it off for now is safe enough because the ASTGen library doesn't use concurrency, and thus far, that is the only area with backdeployed fixes.
|
@swift-ci please test |
etcwilde
changed the title
|
macOS failure: Restarting build. |
|
@swift-ci please test macOS |
DougGregor
approved these changes
Feb 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry-picking #63192
This patch contains a myriad of small but important fixes for the compat56 library.
Break link requirement on compiler-rt:
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 _platformIsAtLeast symbol required for the availability check in swift_voucher_needs_adopt. To avoid the availability check for the underlying voucher_needs_adopt function, I just use dlsym to 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.
AsyncContext layout change:
I apparently copied the Task.h from the wrong place, and we had a small ABI change in AsyncContext between 5.6/5.5/backdeploy and the 5.7 runtime that I copied the header from. AsyncContext lost the AsyncContextFlags field 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 the SWIFT_TASK_DEBUG_LOG was not working, so I fixed that up.
Additionally, I found that the continuation_validation test 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 an UNSUPPORTED line for that.
rdar://104084276
rdar://104241404
Symbol visibility:
Apparently the -fvisibility=hidden being 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
Stage-0 ASTGen:
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