Cherrypick fixes for issue 74866 to release/5.10 #77277
Closed
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
|
Please test with following PRs: @swift-ci please test |
1 similar comment
|
Please test with following PRs: @swift-ci please test |
|
Please test with following PRs: @swift-ci please test |
1 similar comment
|
Please test with following PRs: @swift-ci please test |
hjyamauchi
force-pushed
the
release/5.10
branch
from
8ce4710 to
060aade
Compare
|
Please test with following PRs: @swift-ci please test |
|
@swift-ci please test Windows Platform |
|
Hi @shahmishal I'd like to test this PR along with the associated swiftlang/llvm-project#9478. But the swift-ci directive doesn't seem to pick up the associated PR, as seen in the empty related PR list in the CI log line The same CI directive worked on a similar PR on the release/6.0 CI here: #77261 Is this a bug? Is there a way to work around this? Thanks. |
|
Please test with following PRs: @swift-ci please test |
hjyamauchi
force-pushed
the
release/5.10
branch
from
060aade to
1c15b1d
Compare
|
Please test with following PRs: @swift-ci please test |
|
This branch has been abandoned for almost 5 months now, there have been no commits since June. Swift always abandons the previous release branch once a new release comes out, so no 5.10 commits will be taken now that 6.0 is out. @compnerd, perhaps you should explain the Swift branching process to the BC team. |
|
Here's the context: to get the win/arm64 compiler back to a working state, we'd need to fix (the win/arm64 codegen in) the bootstrap/pinned toolchain, that we use to build a newer toolchain (as in the swift compiler self-hosting, or the fact that swift compiler is partially written in swift) and we currently use the 5.10 toolchain to bootstrap the 6.0 and the main toolchain. For reference: #74866 (comment) I'm not 100% sure if patching and using 5.10 as the bootstrap toolchain is strictly necessary, not just customary, and I think we probably can use a 6.0 build to bootstrap 6.0 and main, but to do that, we'd need to
If we can patch 5.10, it'd take to
Given this, patching 5.1 seemed to be a bit simpler, if possible. Hence a preference to make this fix in the 5.10 toolchain. I think @compnerd suggested patching 5.10 for that reason. @DougGregor @eeckstein @compnerd What do you think? |
|
Cherrypicking the fixes for issue 74866 into release/5.10 (this PR and swiftlang/llvm-project#9478) seems to cause the following test failures, which do not occur with
|
|
@hjyamauchi did you verify that a 5.10 toolchain built with this PR is good for building a windows arm64 compiler with SwiftCompilerSources enabled? |
|
As per #77277 (comment), I added back the debug assertions that I used for the I also tested with cherrypicking only the
The debug assertion patch on top of this PR: |
@eeckstein Yes, I was able to locally (tested up to ~Oct 9, 2024 on main):
I'll repeat it one more time to re-verify this. |
|
Yes, you may need to hunt down the last six months of patches in this area if we're serious about back porting the work to it. |
|
@egorzhdan @ahatanaka Do you know what code adds
This PR would like to drop the return type of an |
@rjmccall |
|
Swift decides the function signature that it passes to Clang, and I expect the bug fixes will be on that side. |
Do you mean the bug fixes will be on the Swift side? |
|
That's my guess, yes. |
@eeckstein Yes, I was able to confirm this works again. The 5.10 toolchain built with this PR is able to bootstrap a main-branch windows arm64 toolchain with SwiftCompilerSources enabled, as described above. |
|
Please test with following PRs: @swift-ci please test |
1 similar comment
|
Please test with following PRs: @swift-ci please test |
|
Please test with following PRs: @swift-ci please test |
1 similar comment
|
Please test with following PRs: @swift-ci please test |
|
Please test with following PRs: @swift-ci please test |
|
Please test with following PRs: @swift-ci please test |
|
Please test with following PRs: @swift-ci please test |
The use of 'nocapture' for parameters and return values is incorrect for C++ types, as they can actually capture a pointer into its own value (e.g. std::string in libstdc++) rdar://115062687 Cherrypick commit 4858cb6 Cherrypick PR swiftlang#68481
|
Please test with following PRs: @swift-ci please test |
We do not synthesize the inheritance thunks correctly for such methods. Do not try to synthesize them, as that causes issues when there are two overloads of the same method, one with rvalue this and one without. The proper solution is tracked as swiftlang#69745 Unblocks rdar://114282353 Cherrypick commit f9bf957 Cherrypick PR swiftlang#69746
…e setter Cherrypick commit 1d53ece Cherrypick PR swiftlang#69781
…e classes If a C++ type `Derived` inherits from `Base` privately, the public methods from `Base` should not be callable on an instance of `Derived`. However, C++ supports exposing such methods via a using declaration: `using MyPrivateBase::myPublicMethod;`. MSVC started using this feature for `std::optional` which means Swift doesn't correctly import `var pointee: Pointee` for instantiations of `std::optional` on Windows. This prevents the automatic conformance to `CxxOptional` from being synthesized. rdar://114282353 / resolves swiftlang#68068 Cherrypick commit efc008a Cherrypick PR swiftlang#69623
…kup table elsewhere NFC intended Cherrypick commit 0b69eb5 Cherrypick PR swiftlang#69991
…base class This is required for proper support for `std::vector::iterator` on Windows. rdar://118657936 / resolves swiftlang#69990 Cherrypick commit ece33a4 Cherrypick PR swiftlang#69991
…wiftlang#71790) This fixes a bug where the thunk for a C++ constructor call wasn't being called when the constructor was called the second time. Cherrypick commit 97a8148 Cherrypick PR swiftlang#71790
hjyamauchi
force-pushed
the
release/5.10
branch
from
1c15b1d to
7bc91d0
Compare
|
Please test with following PRs: @swift-ci please test |
|
Please test with following PRs: @swift-ci please test Windows Platform |
…lang#71459) Fix a bug in expandExternalSignatureTypes where it wasn't annotating a function call parameter type with sret when the result was being returned indirectly. The bug was causing calls to ObjC methods that return their results indirectly to crash. Additionally, fix the return type for C++ constructors computed in expandExternalSignatureTypes. Previously, the return type was always void even on targets that require constructors to return this (e.g., Apple arm64), which was causing C++ constructor thunks to be emitted needlessly. Resolves rdar://121618707 Cherrypick commit b3f302b Cherrypick PR swiftlang#71459
On Windows/AArch64, a different register is used between when an arugment is both inreg and sret (X0 or X1) and when it is just sret (X8) as the following comment indicates: https://github.com/llvm/llvm-project/blob/46fe36a4295f05d5d3731762e31fc4e6e99863e9/llvm/lib/Target/AArch64/AArch64CallingConvention.td#L42 ``` // In AAPCS, an SRet is passed in X8, not X0 like a normal pointer parameter. // However, on windows, in some circumstances, the SRet is passed in X0 or X1 // instead. The presence of the inreg attribute indicates that SRet is // passed in the alternative register (X0 or X1), not X8: // - X0 for non-instance methods. // - X1 for instance methods. // The "sret" attribute identifies indirect returns. // The "inreg" attribute identifies non-aggregate types. // The position of the "sret" attribute identifies instance/non-instance // methods. // "sret" on argument 0 means non-instance methods. // "sret" on argument 1 means instance methods. CCIfInReg<CCIfType<[i64], CCIfSRet<CCIfType<[i64], CCAssignToReg<[X0, X1]>>>>>, CCIfSRet<CCIfType<[i64], CCAssignToReg<[X8]>>>, ``` So missing/dropping inreg can cause a codegen bug. This is a partial fix for swiftlang#74866 Cherrypick commit 3f0de57 Cherrypick PR swiftlang#76159
In GenCall, fix the IR gen for C++ method calls as under MSVC as the calling conventions for free functions and C++ methods can be different. This also fixes the missing inreg (on sret arguments) issues on Windows ARM64. Also refactor to use CGFunctionInfo returnInfo isSretAfterThis to detect when to reorder the sret and the this arguments under MSVC. In ClagImporter, don't drop the return type for the compound assignment operators such as operator+= when the return value is a reference so that the CGFunctionInfo will be correctly indicate an indirect return for the compound assignment operators. Cherrypick commit fcc1f6b Cherrypick PR swiftlang#76324
hjyamauchi
force-pushed
the
release/5.10
branch
from
7bc91d0 to
b61997e
Compare
|
Please test with following PRs: @swift-ci please test Windows Platform |
|
Please test with following PRs: @swift-ci please test |
|
I managed to figure out a set of additional PRs (~10-ish in total) to get the failing tests fixed. The CI testing passed and this is ready for review. Please take a look @rjmccall. |
|
We decided not to cherrypick the fixes to 5.10 |
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.
arrangeCXXMethodCallfor C++ methods instead ofarrangeFreeFunctionCallin the IRGen.operator []is imported as a mutabl… #69781