[stdlib] Properly identify assert.h. #59987
Conversation
This should be qualified with GLIBC_INCLUDE_PATH, otherwise it will look for assert.h relative to the modulemap file, which is likely not what was intended. Partially buildfixes OpenBSD and FreeBSD.
|
cc @egorzhdan |
Bring drexin's FreeBSD support patch up to date with main. Resolved conflicts in prior merge commit and made some minor buildfixes. Still requires some other more general fixes outside this pr to have this build properly at HEAD but have verified only this builds OK with those fixes possibly TK including swiftlang#59987 on FreeBSD 12.3. Note: I don't intend to support this platform as a priority.
Bring drexin's FreeBSD support patch up to date with main. Resolved conflicts in prior merge commit and made some minor buildfixes. Still requires some other more general fixes outside this pr to have this build properly at HEAD but have verified only this builds OK with those fixes possibly TK including swiftlang#59987 and bootstrapping disabled on FreeBSD 12.3. Note: I don't intend to support this platform as a priority.
There was a problem hiding this comment.
it will look for assert.h relative to the modulemap file, which is likely not what was
intended.
This was intended: the modulemap file is now injected into the Glibc include path (see getClangInvocationFileMapping), so the headers can be referenced directly from the modulemap.
This works by calling into Clang driver to extract include paths (clangToolchain.AddClangSystemIncludeArgs) and find the include path with Glibc. Perhaps Clang returns incorrect include paths on Android/OpenBSD?
I was actually going to remove GLIBC_INCLUDE_PATH from the build entirely to improve portability of the toolchains, I won't do that now to avoid breaking more of Android/OpenBSD, but I don't think we should accumulate more usages of it.
Yes, that change probably doesn't work when cross-compiling with the Android NDK- looking into it today- though it does work when building natively on Android itself.
I agree with removing that path, once we figure this last issue out, and Android had already disabled the last use of it. |
|
It appears I was wrong to say this is needed on Android, as this line depends on #59846 working, and I had reverted that pull when natively building on Android earlier. After applying that pull again and testing locally, it turns out this pull is not needed on Android, and as you both discussed elsewhere, probably won't be needed on OpenBSD either once this mistake is corrected. |
|
Awesome, thanks @buttaface for checking that! |
|
OpenBSD still needs the underlying VFS changes to make this work, but that's another issue. I'll close this for now. |
Does #59846 not work for you? It works for me when natively compiling on Android but not when cross-compiling with the NDK, going to look into why. |
|
No -- this requires https://reviews.llvm.org/D129654 plus the |
|
Got it, I don't think it passes the right include paths when cross-compiling, think it's remapping |
This should be qualified with GLIBC_INCLUDE_PATH, otherwise it will look
for assert.h relative to the modulemap file, which is likely not what was
intended.
Partially buildfixes OpenBSD and FreeBSD.