Skip to content

add %SDKROOT%\usr\include as -isystem for non-Windows non-Darwin #78649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andrurogerz
Copy link
Contributor

@andrurogerz andrurogerz commented Jan 15, 2025

Purpose

Automatically add %SDKROOT%\usr\include as the -isystem argument to clang when both SDK and sysroot are provided.

Background

Without this change, cross-compiling a Swift program for Android requires explicitly adding -Xswiftc -I -Xswiftc %SDKROOT%\usr\include to swift build or -Xcc -I%SDKROOT%\usr\include to CMAKE_Swift_FLAGS for cmake builds.

Validation

Built swift-argument-parser for Android on Windows without manually specifying %SDKROOT%\usr\include.

Using Swift build:

swift build --target ArgumentParser ^
    --triple aarch64-unknown-linux-android29 ^
    --sdk %ANDROID_NDK_ROOT%\toolchains\llvm\prebuilt\windows-x86_64\sysroot ^
    -Xswiftc -sdk -Xswiftc %SDKROOT%\..\..\..\..\Android.platform\Developer\SDKs\Android.sdk\ ^
    -Xswiftc -sysroot -Xswiftc %ANDROID_NDK_ROOT%\toolchains\llvm\prebuilt\windows-x86_64\sysroot ^
    -Xswiftc -Xclang-linker -Xswiftc -resource-dir -Xswiftc -Xclang-linker -Xswiftc %ANDROID_NDK_ROOT%\toolchains\llvm\prebuilt\windows-x86_64\lib\clang\17.0.2

Using CMake

cmake -B build -S . -G Ninja ^
    -D CMAKE_BUILD_WITH_INSTALL_RPATH=YES ^
    -D CMAKE_SYSTEM_NAME=Android ^
    -D CMAKE_ANDROID_ARCH_ABI=arm64-v8a ^
    -D CMAKE_SYSTEM_VERSION=29 ^
    -D CMAKE_Swift_COMPILER_TARGET=aarch64-unknown-linux-android29 ^
    -D CMAKE_Swift_FLAGS="-sdk %SDKROOT%..\..\..\..\Android.platform\Developer\SDKs\Android.sdk  -Xclang-linker -resource-dir -Xclang-linker %ANDROID_NDK_ROOT%\toolchains\llvm\prebuilt\windows-x86_64\lib\clang\17.0.2"
cmake --build build --target ArgumentParser

@andrurogerz
Copy link
Contributor Author

@compnerd this the change we put together locally last week

@andrurogerz andrurogerz changed the title add $SDK\usr\include as -isystem for non-Windows non-Darwin add %SDKROOT%\usr\include as -isystem for non-Windows non-Darwin Jan 15, 2025
@finagolfin
Copy link
Member

I don't think this is necessary normally on linux/macOS, as the sdk root is passed in as the clang sysroot already and clang automatically checks $clang-sysroot/usr/include/ and a couple other directories for headers. I suppose it's possible the Windows clang doesn't do the same, so this workaround may be needed there, but I'd look first into all the different --sdk/-sdk/-sysroot flags you're adding: that seems too many and you are likely confusing the compiler.

@ian-twilightcoder
Copy link
Contributor

Normally these paths are added in the clang driver/toolchain, I don't think we should be adding these in Swift

@ian-twilightcoder
Copy link
Contributor

ian-twilightcoder commented Jan 15, 2025

Maybe we need to add an Android toolchain at https://github.com/llvm/llvm-project/tree/main/clang/lib/Driver/ToolChains ? Or add Android affordances in the MinGW/MSVC toolchain?

@andrurogerz
Copy link
Contributor Author

@finagolfin @ian-twilightcoder thank you for the feedback! I will look into this issue further and revise the approach.

My goal here is to simplify Swift for Android builds to they only require two paths be manually specified:

  1. Swift SDK for Android root dir ($SDKROOT)
  2. Android NDK root dir ($ANDROID_NDK_ROOT)

However, currently when building a simple CMake project that includes Swift, two other paths must be explicitly provided via CMAKE_Swift_FLAGS for it to build properly (similar for swift build).

Specifically:

-Xclang-linker -resource-dir -Xclang-linker %ANDROID_NDK_ROOT%\toolchains\llvm\prebuilt\windows-x86_64\lib\clang\17.0.2

and

-Xcc -I%SDKROOT%\usr\include"

The toolchain should be able to derive these paths, and this PR was my attempt at deriving the second one. I would truly appreciate any pointers to the component(s) where it makes the most sense to address this issue. Thank you!

@finagolfin
Copy link
Member

The toolchain should be able to derive these paths, and this PR was my attempt at deriving the second one. I would truly appreciate any pointers to the component(s) where it makes the most sense to address this issue.

The question is, why isn't it looking in that directory already? To determine that, leave off the -Xcc -I%SDKROOT%\usr\include flags and add -Xcc -v instead: that gets the clang frontend to dump the sysroot paths that it looks in for headers and should tell you where you're going wrong.

@ian-twilightcoder
Copy link
Contributor

ian-twilightcoder commented Jan 15, 2025

@finagolfin @ian-twilightcoder thank you for the feedback! I will look into this issue further and revise the approach.

My goal here is to simplify Swift for Android builds to they only require two paths be manually specified:

And that sounds great! But probably not Swift specific, which is why I suggest moving this into the llvm toolchain that's being used for your build.

The toolchain should be able to derive these paths, and this PR was my attempt at deriving the second one. I would truly appreciate any pointers to the component(s) where it makes the most sense to address this issue. Thank you!

Yep, I just think "that toolchain" is probably the llvm one and not the swift one.

@andrurogerz
Copy link
Contributor Author

Having investigated a bit more today, I actually don't think the current PR is too far off from being correct. Here's my current assessment:

  1. If a Swift SDK path is provided, it is passed to the compiler as --sysroot
  2. However, If a sysroot is provided, it is passed to the compiler as --sysroot instead
  3. The compiler will implicitly search the sysroot's usr/include subdir

Since cross-compilation requires both paths be provided, only the sysroot is passed to the compiler-- it is never told to search the Swift SDK usr/include subdir. I have confirmed this with -Xcc -v logging. This patch fixes the issue by explicitly adding the Swift SDK usr/include subdir to the include search path. It quite correct, though: the patch should be scoped to the if (auto sysroot = searchPathOpts.getSysRoot()) { block so it only impacts the scenario when both paths are provided. Also, the path does not necessarily need to be passed as -isystem, but it does seem more appropriate than using -I in this case.

Please let me know what you think; I could still be wildly off because I'm not very familiar with the Swift build tools yet. If my assessment looks right, I'll revise the PR and update the description to properly describe the issue and the fix.

@ian-twilightcoder
Copy link
Contributor

Having investigated a bit more today, I actually don't think the current PR is too far off from being correct. Here's my current assessment:

  1. If a Swift SDK path is provided, it is passed to the compiler as --sysroot
  2. However, If a sysroot is provided, it is passed to the compiler as --sysroot instead
  3. The compiler will implicitly search the sysroot's usr/include subdir

Since cross-compilation requires both paths be provided, only the sysroot is passed to the compiler-- it is never told to search the Swift SDK usr/include subdir. I have confirmed this with -Xcc -v logging. This patch fixes the issue by explicitly adding the Swift SDK usr/include subdir to the include search path. It quite correct, though: the patch should be scoped to the if (auto sysroot = searchPathOpts.getSysRoot()) { block so it only impacts the scenario when both paths are provided. Also, the path does not necessarily need to be passed as -isystem, but it does seem more appropriate than using -I in this case.

Please let me know what you think; I could still be wildly off because I'm not very familiar with the Swift build tools yet. If my assessment looks right, I'll revise the PR and update the description to properly describe the issue and the fix.

If I understand you right, you're saying that when you do swiftc -sdk A -sysroot B, that A isn't being passed to clang at all?

@finagolfin
Copy link
Member

only the sysroot is passed to the compiler-- it is never told to search the Swift SDK usr/include subdir. I have confirmed this with -Xcc -v logging

Can you paste that output here? It would be good to know precisely which header it's missing. Try also adding the flags -Xswiftc -Xfrontend -Xswiftc -dump-clang-diagnostics to that debug command, which will tell you exactly what flags it's passing to the clang importer frontend.

There are currently some issues when compiling for ELF platforms with the trunk swift-driver because of the ongoing work to change the meaning of certain core flags in the Swift compiler, such as #76381 which I filed. It is possible this is a consequence of those changes, which have still not been completed.

@andrurogerz
Copy link
Contributor Author

Can you paste that output here? It would be good to know precisely which header it's missing. Try also adding the flags -Xswiftc -Xfrontend -Xswiftc -dump-clang-diagnostics to that debug command, which will tell you exactly what flags it's passing to the clang importer frontend.

A build without the additional $SDKROOT/usr/include path fails to find the Dispatch module. I pasted a full log with extra verbosity project here, but some highlights:

error: emit-module command failed with exit code 1 (use -v to see invocation)
<unknown>:0: error: cannot load underlying module for 'Dispatch'
<unknown>:0: error: cannot load underlying module for 'Dispatch'

The include search path passed to clang:

clang -cc1 version 19.1.5 based upon LLVM 19.1.5 default target x86_64-unknown-windows-msvc
ignoring nonexistent directory "C:\Android\android-sdk\ndk\26.3.11579264\toolchains\llvm\prebuilt\windows-x86_64\sysroot/usr/local/include"
ignoring nonexistent directory "C:\Android\android-sdk\ndk\26.3.11579264\toolchains\llvm\prebuilt\windows-x86_64\sysroot/include"
#include "..." search starts here:
#include <...> search starts here:
 S:\swift-hello\.build\aarch64-unknown-linux-android29\debug\Modules
 C:\Users\Andrew\AppData\Local\Programs\Swift\Platforms\0.0.0\Android.platform\Developer\SDKs\Android.sdk\usr\lib\swift\shims
 C:\Users\Andrew\AppData\Local\Programs\Swift\Toolchains\0.0.0+Asserts\usr\lib\swift
 C:\Users\Andrew\AppData\Local\Programs\Swift\Toolchains\0.0.0+Asserts\usr\lib\swift\clang\include
 C:\Android\android-sdk\ndk\26.3.11579264\toolchains\llvm\prebuilt\windows-x86_64\sysroot/usr/include/aarch64-linux-android
 C:\Android\android-sdk\ndk\26.3.11579264\toolchains\llvm\prebuilt\windows-x86_64\sysroot/usr/include
End of search list.

The clang importer args:

clang importer driver args: 
'C:\Users\Andrew\AppData\Local\Programs\Swift\Toolchains\0.0.0+Asserts\usr\bin\clang'
'-isystem' 'C:\Users\Andrew\AppData\Local\Programs\Swift\Platforms\0.0.0\Android.platform\Developer\SDKs\Android.sdk\usr\lib\swift\shims'
'-fsyntax-only'
'-fblocks'
'-D__swift__=60200'
'-fretain-comments-from-system-headers'
'-isystem' 'C:\Users\Andrew\AppData\Local\Programs\Swift\Toolchains\0.0.0+Asserts\usr\lib\swift'
'-fPIC'
'-fmodules'
'-Xclang' '-fmodule-feature'
'-Xclang' 'swift'
'-x' 'c'
'-std=gnu11'
'-D_GNU_SOURCE'
'-D__SWIFT_ATTR_SUPPORTS_SENDING=1'
'--sysroot' 'C:\Android\android-sdk\ndk\26.3.11579264\toolchains\llvm\prebuilt\windows-x86_64\sysroot'
'-fmodules-cache-path=S:\swift-hello\.build\aarch64-unknown-linux-android29\debug\ModuleCache'
'-fmodules-validate-system-headers'
'-Xclang' '-fmodule-format=obj'
'-fapinotes-modules'
'-fapinotes-swift-version=6'
'-iapinotes-modules' 'C:\Users\Andrew\AppData\Local\Programs\Swift\Platforms\0.0.0\Android.platform\Developer\SDKs\Android.sdk\usr\lib\swift\apinotes'
'-iapinotes-modules' 'C:\Users\Andrew\AppData\Local\Programs\Swift\Toolchains\0.0.0+Asserts\usr\lib\swift\apinotes'
'-target' 'aarch64-unknown-linux-android29'
'<swift-imported-modules>'
'-resource-dir' 'C:\Users\Andrew\AppData\Local\Programs\Swift\Toolchains\0.0.0+Asserts\usr\lib\swift\clang'
'-fansi-escape-codes'
'-working-directory' 'S:\swift-hello'
'--sysroot' 'C:\Android\android-sdk\ndk\26.3.11579264\toolchains\llvm\prebuilt\windows-x86_64\sysroot'
'-g'
'-v'
'-fno-omit-frame-pointer'
'-ffile-compilation-dir=S:\swift-hello'
'-IS:\swift-hello\.build\aarch64-unknown-linux-android29\debug\Modules'

@ian-twilightcoder
Copy link
Contributor

That's a weird value for -isystem. I would've expected it to stop at the .sdk part, i.e. C:\Users\Andrew\AppData\Local\Programs\Swift\Platforms\0.0.0\Android.platform\Developer\SDKs\Android.sdk

@andrurogerz
Copy link
Contributor Author

That's a weird value for -isystem. I would've expected it to stop at the .sdk part, i.e. C:\Users\Andrew\AppData\Local\Programs\Swift\Platforms\0.0.0\Android.platform\Developer\SDKs\Android.sdk

I just confirmed that explicitly appending the "usr/include" to the sdk root is required here or it doesn't search that location.

@finagolfin
Copy link
Member

No, it isn't required: the static linux Musl SDK and my Android SDK bundle build just fine today without it. However, I disable using swift-driver from 6.1 onwards because of recent regressions in how it works, so it's possible you're hitting those too.

The current swift-driver Unix config for 6.1 onwards is a mess that will have to be cleaned up, while this pull simply adds yet another workaround for likely that underlying problem. I will discuss it with Saleem and some others and let you know.

@ian-twilightcoder
Copy link
Contributor

That's a weird value for -isystem. I would've expected it to stop at the .sdk part, i.e. C:\Users\Andrew\AppData\Local\Programs\Swift\Platforms\0.0.0\Android.platform\Developer\SDKs\Android.sdk

I just confirmed that explicitly appending the "usr/include" to the sdk root is required here or it doesn't search that location.

That sounds like another issue in the clang driver. Which driver do you end up getting?

@finagolfin
Copy link
Member

OK, I finally spent some time looking into this. All current Unix toolchains and bundles expect Dispatch to be in the Swift -resource-dir, which according to your linked gist, you are passing in as -resource-dir C:\Users\Andrew\AppData\Local\Programs\Swift\Toolchains\0.0.0+Asserts\usr\lib\swift. However, you say that your toolchain instead places Dispatch at $SDKROOT/usr/include, ie C:\Users\Andrew\AppData\Local\Programs\Swift\Platforms\0.0.0\Android.platform\Developer\SDKs\Android.sdk\usr\include\? That is not where the toolchain expects to find such Swift-related headers.

I'm not blaming you for this, but I've had a brief discussion about this with Saleem and Alex in the past and it sounds like your TBC Android SDK moves a bunch of files around and places them in highly non-standard locations.

I suppose that's fine if you all want to do that in your local SDKs, but you can't expect upstream to start supporting such non-standard layouts, especially since you have never explicitly detailed and proposed this new layout upstream. If we add this change you're proposing, that means the Swift compiler will also start looking in <sdkroot>/usr/include for the Dispatch headers. Since in the general case, that might even be /usr/include/, it might cause duplication conflicts between a system-installed libdispatch and the Swift one in /usr/lib/swift/. Even if that's not a problem, it's not a good idea to put in a bunch of random includes like this to accommodate non-standard layouts downstream.

I suggest you go talk to your TBC team and either change your SDK layout to match what all other linux and Unix toolchains and bundles are doing, or come up with an explicit proposal for how these SDK layouts should be changed upstream.

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.

3 participants