Skip to content

[ClangImporter] clang's -iframework comes before builtin usr/local/include, but Swift's -Fsystem comes after#78303

Merged
ian-twilightcoder merged 1 commit into
swiftlang:mainfrom
ian-twilightcoder:clang-importer-search-paths
Jan 6, 2025
Merged

[ClangImporter] clang's -iframework comes before builtin usr/local/include, but Swift's -Fsystem comes after#78303
ian-twilightcoder merged 1 commit into
swiftlang:mainfrom
ian-twilightcoder:clang-importer-search-paths

Conversation

@ian-twilightcoder

Copy link
Copy Markdown
Contributor

When Swift passes search paths to clang, it does so directly into the HeaderSearch. That means that those paths get ordered inconsistently compared to the equivalent clang flag, and causes inconsistencies when building clang modules with clang and with Swift. Instead of touching the HeaderSearch directly, pass Swift search paths as driver flags, just do them after the -Xcc ones.

Swift doesn't have a way to pass a search path to clang as -isystem, only as -I which usually isn't the right flag. Add an -Isystem Swift flag so that those paths can be passed to clang as -isystem.

rdar://93951328

@artemcm artemcm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great. It's a significant improvement to have these be command-line flags, and to unify ClangImporter and ClandScanDeps config here. The SearchPath unification cleans things up a bunch too. Thanks!

Comment thread lib/ClangImporter/ClangModuleDependencyScanner.cpp Outdated
@ian-twilightcoder

Copy link
Copy Markdown
Contributor Author

I think this is great. It's a significant improvement to have these be command-line flags, and to unify ClangImporter and ClandScanDeps config here. The SearchPath unification cleans things up a bunch too. Thanks!

😁 Is it a problem that this requires a lock-step lldb change? I can't come up with anything clever to avoid that since I can't overload SearchPathOptions::getImportSearchPaths(). I could make a new method instead of changing the existing one, but I don't know what I could call it. getImportSearchPathsAsSearchPaths? 🤢

@artemcm

artemcm commented Dec 20, 2024

Copy link
Copy Markdown
Contributor

I think this is great. It's a significant improvement to have these be command-line flags, and to unify ClangImporter and ClandScanDeps config here. The SearchPath unification cleans things up a bunch too. Thanks!

😁 Is it a problem that this requires a lock-step lldb change? I can't come up with anything clever to avoid that since I can't overload SearchPathOptions::getImportSearchPaths(). I could make a new method instead of changing the existing one, but I don't know what I could call it. getImportSearchPathsAsSearchPaths? 🤢

Seems that an LLDB lockstep change is a lesser evil here, to me.

@ian-twilightcoder ian-twilightcoder changed the title clang's -iframework comes before builtin usr/local/include, but Swift's -Fsystem comes after [ClangImporter] clang's -iframework comes before builtin usr/local/include, but Swift's -Fsystem comes after Dec 23, 2024
@ian-twilightcoder

Copy link
Copy Markdown
Contributor Author

I think this is great. It's a significant improvement to have these be command-line flags, and to unify ClangImporter and ClandScanDeps config here. The SearchPath unification cleans things up a bunch too. Thanks!

😁 Is it a problem that this requires a lock-step lldb change? I can't come up with anything clever to avoid that since I can't overload SearchPathOptions::getImportSearchPaths(). I could make a new method instead of changing the existing one, but I don't know what I could call it. getImportSearchPathsAsSearchPaths? 🤢

Seems that an LLDB lockstep change is a lesser evil here, to me.

👍 swiftlang/llvm-project#9780

…clude, but Swift's -Fsystem comes after

When Swift passes search paths to clang, it does so directly into the HeaderSearch. That means that those paths get ordered inconsistently compared to the equivalent clang flag, and causes inconsistencies when building clang modules with clang and with Swift. Instead of touching the HeaderSearch directly, pass Swift search paths as driver flags, just do them after the -Xcc ones.

Swift doesn't have a way to pass a search path to clang as -isystem, only as -I which usually isn't the right flag. Add an -Isystem Swift flag so that those paths can be passed to clang as -isystem.

rdar://93951328
@ian-twilightcoder

Copy link
Copy Markdown
Contributor Author

swiftlang/llvm-project#9790

@swift-ci smoke test

// RUN: %target-swift-frontend -sdk %t/sdk -typecheck -I %t/sdk/custom/include -module-cache-path %t/mcp5 %t/SearchPathOrder.swift
// RUN: %target-swift-frontend -sdk %t/sdk -typecheck -Isystem %t/sdk/custom/include -module-cache-path %t/mcp6 %t/SearchPathOrder.swift
// RUN: %target-swift-frontend -sdk %t/sdk -typecheck -F %t/sdk/custom/Frameworks -module-cache-path %t/mcp7 %t/SearchPathOrder.swift
// RUN: %target-swift-frontend -sdk %t/sdk -typecheck -Fsystem %t/sdk/custom/Frameworks -module-cache-path %t/mcp8 %t/SearchPathOrder.swift

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why, but this passes in main but fails in the current Xcode version. I'll have to double check this test when this builds into an Xcode to make sure this change fixes it.

@ian-twilightcoder ian-twilightcoder merged commit 87d6979 into swiftlang:main Jan 6, 2025
@ian-twilightcoder ian-twilightcoder deleted the clang-importer-search-paths branch January 6, 2025 21:05
@cachemeifyoucan cachemeifyoucan mentioned this pull request Jan 9, 2025
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants