Fix the swiftCxxStdlib link with explicit module build on Windows #85904
Conversation
|
@swift-ci please test |
hjyamauchi
requested review from
artemcm,
hborla,
rjmccall,
slavapestov and
xedin
as code owners
hjyamauchi
force-pushed
the
cxxstdlib
branch
from
adc36da to
4ee2161
Compare
|
@swift-ci please test |
| [mainModuleName](StringRef Name) { | ||
| return mainModuleName == Name; | ||
| })) | ||
| if (ScanASTContext.LangOpts.Target.getOS() == llvm::Triple::Win32) |
There was a problem hiding this comment.
Why is this scoped to just windows?
There was a problem hiding this comment.
To match the IRGenModule code where it's scoped to windows
swift/lib/IRGen/IRGenModule.cpp
Line 1758 in 472937e
There was a problem hiding this comment.
Why is the IRGenModule code scoped to just windows?
There was a problem hiding this comment.
I think because the library names differ between static and dynamic on Windows and it needs to know whether it's static or dynamic #67776
There was a problem hiding this comment.
I removed the Windows scoping here
|
I suspect that @egorzhdan might have some thoughts on this as well. Currently, IRGen will inject the CxxStdlib unconditionally, which this mirrors. |
| return mainModuleName == Name; | ||
| })) | ||
| if (ScanASTContext.LangOpts.Target.getOS() == llvm::Triple::Win32) | ||
| mainDependencies.addModuleImport("CxxStdlib", /* isExported */ false, |
There was a problem hiding this comment.
The string literal can be replaced with scanASTContext.Id_CxxStdlib.str()
| break; | ||
| } | ||
|
|
||
| if (ScanASTContext.LangOpts.EnableCXXInterop) { |
There was a problem hiding this comment.
Could you please add a comment with a detailed explanation for why Windows behavior differs and why the default flow in
is insufficient.There was a problem hiding this comment.
We (and the IRGenModule code) currently assume that we always link with and add dependency on CxxStdlib if C++ interop is enabled (dynamic or static but it needs to be currently static linking for Windows.) Hence this approach in this PR. But it looks like this lookupCxxStdLibOverlay code doesn't seem to assume that. Do you know why?
There was a problem hiding this comment.
I think this code and the lookupCxxStdLibOverlay code are orthogonal. We currently and do always link with CxxStdlib if C++ interop is enabled, dynamic or static. So, we explicitly add CxxStdlib to the (transitive) dependency set by adding a dependency to the main/root module for all platforms but in particular for Windows because it needs to know if it's static or dynamic to determine the correct library name, which this new code does. The lookupCxxStdLibOverlay code differs in that it only adds a CxxStdlib dep if a module depends on the clang std module. We still need the lookupCxxStdLibOverlay code because it affects the direct dependencies of non-root/individual modules (like adding a CxxStdlib dep to what only has a dep on the underlying std module or not adding it to the Darwin module, etc.) as the no-cxx-overlay-for-non-interop-interface.swift test checks.
| [mainModuleName](StringRef Name) { | ||
| return mainModuleName == Name; | ||
| })) | ||
| if (ScanASTContext.LangOpts.Target.getOS() == llvm::Triple::Win32) |
There was a problem hiding this comment.
Why is the IRGenModule code scoped to just windows?
We currently support static linking swiftCxxStdlib only on Windows. When C++ interop is enabled but swiftCxxStdlib isn't actually used or only when its underlying std module is used, we incorrectly emitted the dynamic version of the lib name and caused the link error. This change fixes it by always adding it to the dependency. Issue swiftlang#85876
hjyamauchi
force-pushed
the
cxxstdlib
branch
from
4ee2161 to
f9722f0
Compare
|
@swift-ci please test |
|
well hello again
David Daniel Skoda
…On Mon, Dec 15, 2025 at 13:01 Saleem Abdulrasool ***@***.***> wrote:
***@***.**** approved this pull request.
—
Reply to this email directly, view it on GitHub
<#85904 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B26S552VZNMVV5RNMH7V73D4B3ZPPAVCNFSM6AAAAACOMWABZ2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNZZGQ4DQMZTGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
4 participants
We currently support static linking swiftCxxStdlib only on Windows. When C++ interop is enabled but swiftCxxStdlib isn't actually used or only when its underlying std module is used, we incorrectly emitted the dynamic version of the lib name and caused the link error. This change fixes it by always adding it to the dependency.
Issue #85876