[driver] Conditionally include installed libc++ headers for Android #1
Conversation
This file contains 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
https://reviews.llvm.org/D71154 prevented Clang from search for libc++ headers installed alongside the driver when targeting Android. The motivation was the NDK's use of a different libc++ inline namespace (`__ndk1` instead of the standard `__1`), which made regular libc++ headers incompatible with the NDK's libc++ library. Since then, libc++ has gained the ability to install its `__config_site` header (which controls the inline namespace, among other things) to a per-target include directory, which enables per-target customizations. If this directory is present, the user has expressly built libc++ for Android, and we should use those headers. The motivation is that, with the current setup, if a user builds their own libc++ for Android, they'll use the library they built themselves but the NDK's headers instead of their own, which is surprising at best and can cause all sorts of problems (e.g. if you built your own libc++ with a different ABI configuration). It's important to match the headers and libraries in that scenario, and checking for an Android per-target include directory lets us do so without regressing the original scenario which https://reviews.llvm.org/D71154 was addressing. While I'm here, switch to using sys::path::append instead of slashes directly, to get system path separators on Windows, which is consistent with how library paths are constructed (and that consistency will be important in a follow-up, where we use a common search function for the include and library path construction). (As an aside, one of the motivations for https://reviews.llvm.org/D71154 was to support targeting both Android and Apple platforms, which expected libc++ headers to be provided by the toolcain at the time. Apple has since switched to including libc++ headers in the platform SDK instead of in the toolchain, so that specific motivation no longer applies either.) Differential Revision: https://reviews.llvm.org/D159292 [ghstack-poisoned]
This was referenced Aug 31, 2023
…r Android" https://reviews.llvm.org/D71154 prevented Clang from search for libc++ headers installed alongside the driver when targeting Android. The motivation was the NDK's use of a different libc++ inline namespace (`__ndk1` instead of the standard `__1`), which made regular libc++ headers incompatible with the NDK's libc++ library. Since then, libc++ has gained the ability to install its `__config_site` header (which controls the inline namespace, among other things) to a per-target include directory, which enables per-target customizations. If this directory is present, the user has expressly built libc++ for Android, and we should use those headers. The motivation is that, with the current setup, if a user builds their own libc++ for Android, they'll use the library they built themselves but the NDK's headers instead of their own, which is surprising at best and can cause all sorts of problems (e.g. if you built your own libc++ with a different ABI configuration). It's important to match the headers and libraries in that scenario, and checking for an Android per-target include directory lets us do so without regressing the original scenario which https://reviews.llvm.org/D71154 was addressing. While I'm here, switch to using sys::path::append instead of slashes directly, to get system path separators on Windows, which is consistent with how library paths are constructed (and that consistency will be important in a follow-up, where we use a common search function for the include and library path construction). (As an aside, one of the motivations for https://reviews.llvm.org/D71154 was to support targeting both Android and Apple platforms, which expected libc++ headers to be provided by the toolcain at the time. Apple has since switched to including libc++ headers in the platform SDK instead of in the toolchain, so that specific motivation no longer applies either.) Differential Revision: https://reviews.llvm.org/D159292 [ghstack-poisoned]
2 tasks
smeenai
added a commit
that referenced
this pull request
Sep 1, 2023
https://reviews.llvm.org/D71154 prevented Clang from search for libc++ headers installed alongside the driver when targeting Android. The motivation was the NDK's use of a different libc++ inline namespace (`__ndk1` instead of the standard `__1`), which made regular libc++ headers incompatible with the NDK's libc++ library. Since then, libc++ has gained the ability to install its `__config_site` header (which controls the inline namespace, among other things) to a per-target include directory, which enables per-target customizations. If this directory is present, the user has expressly built libc++ for Android, and we should use those headers. The motivation is that, with the current setup, if a user builds their own libc++ for Android, they'll use the library they built themselves but the NDK's headers instead of their own, which is surprising at best and can cause all sorts of problems (e.g. if you built your own libc++ with a different ABI configuration). It's important to match the headers and libraries in that scenario, and checking for an Android per-target include directory lets us do so without regressing the original scenario which https://reviews.llvm.org/D71154 was addressing. While I'm here, switch to using sys::path::append instead of slashes directly, to get system path separators on Windows, which is consistent with how library paths are constructed (and that consistency will be important in a follow-up, where we use a common search function for the include and library path construction). (As an aside, one of the motivations for https://reviews.llvm.org/D71154 was to support targeting both Android and Apple platforms, which expected libc++ headers to be provided by the toolcain at the time. Apple has since switched to including libc++ headers in the platform SDK instead of in the toolchain, so that specific motivation no longer applies either.) Differential Revision: https://reviews.llvm.org/D159292 ghstack-source-id: 68060e855f045fe40425d494ee09f07f13645051 Pull Request resolved: #1
smeenai
pushed a commit
that referenced
this pull request
Sep 20, 2023
…ttempting to dereferencing iterators.
Runnign some tests with asan built of LLD would throw errors similar to the following:
AddressSanitizer:DEADLYSIGNAL
#0 0x55d8e6da5df7 in operator() /mnt/ssd/repo/lld/llvm-project/lld/MachO/Arch/ARM64.cpp:612
#1 0x55d8e6daa514 in operator() /mnt/ssd/repo/lld/llvm-project/lld/MachO/Arch/ARM64.cpp:650
Differential Revision: https://reviews.llvm.org/D157027
smeenai
pushed a commit
that referenced
this pull request
Oct 2, 2023
…fine.parallel verifier
This patch updates AffineParallelOp::verify() to check each result type matches
its corresponding reduction op (i.e, the result type must be a `FloatType` if
the reduction attribute is `addf`)
affine.parallel will crash on --lower-affine if the corresponding result type
cannot match the reduction attribute.
```
%128 = affine.parallel (%arg2, %arg3) = (0, 0) to (8, 7) reduce ("maxf") -> (memref<8x7xf32>) {
%alloc_33 = memref.alloc() : memref<8x7xf32>
affine.yield %alloc_33 : memref<8x7xf32>
}
```
This will crash and report a type conversion issue when we run `mlir-opt --lower-affine`
```
Assertion failed: (isa<To>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file Casting.h, line 572.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0. Program arguments: mlir-opt --lower-affine temp.mlir
#0 0x0000000102a18f18 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/workspacebin/mlir-opt+0x1002f8f18)
#1 0x0000000102a171b4 llvm::sys::RunSignalHandlers() (/workspacebin/mlir-opt+0x1002f71b4)
#2 0x0000000102a195c4 SignalHandler(int) (/workspacebin/mlir-opt+0x1002f95c4)
#3 0x00000001be7894c4 (/usr/lib/system/libsystem_platform.dylib+0x1803414c4)
#4 0x00000001be771ee0 (/usr/lib/system/libsystem_pthread.dylib+0x180329ee0)
#5 0x00000001be6ac340 (/usr/lib/system/libsystem_c.dylib+0x180264340)
#6 0x00000001be6ab754 (/usr/lib/system/libsystem_c.dylib+0x180263754)
#7 0x0000000106864790 mlir::arith::getIdentityValueAttr(mlir::arith::AtomicRMWKind, mlir::Type, mlir::OpBuilder&, mlir::Location) (.cold.4) (/workspacebin/mlir-opt+0x104144790)
#8 0x0000000102ba66ac mlir::arith::getIdentityValueAttr(mlir::arith::AtomicRMWKind, mlir::Type, mlir::OpBuilder&, mlir::Location) (/workspacebin/mlir-opt+0x1004866ac)
#9 0x0000000102ba6910 mlir::arith::getIdentityValue(mlir::arith::AtomicRMWKind, mlir::Type, mlir::OpBuilder&, mlir::Location) (/workspacebin/mlir-opt+0x100486910)
...
```
Fixes llvm#64068
Reviewed By: mehdi_amini
Differential Revision: https://reviews.llvm.org/D157985
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.
Stack from ghstack (oldest at bottom):
https://reviews.llvm.org/D71154 prevented Clang from search for libc++
headers installed alongside the driver when targeting Android. The
motivation was the NDK's use of a different libc++ inline namespace
(
__ndk1instead of the standard__1), which made regular libc++headers incompatible with the NDK's libc++ library.
Since then, libc++ has gained the ability to install its
__config_siteheader (which controls the inline namespace, among other things) to a
per-target include directory, which enables per-target customizations.
If this directory is present, the user has expressly built libc++ for
Android, and we should use those headers.
The motivation is that, with the current setup, if a user builds their
own libc++ for Android, they'll use the library they built themselves
but the NDK's headers instead of their own, which is surprising at best
and can cause all sorts of problems (e.g. if you built your own libc++
with a different ABI configuration). It's important to match the headers
and libraries in that scenario, and checking for an Android per-target
include directory lets us do so without regressing the original scenario
which https://reviews.llvm.org/D71154 was addressing.
While I'm here, switch to using sys::path::append instead of slashes
directly, to get system path separators on Windows, which is consistent
with how library paths are constructed (and that consistency will be
important in a follow-up, where we use a common search function for the
include and library path construction).
(As an aside, one of the motivations for https://reviews.llvm.org/D71154
was to support targeting both Android and Apple platforms, which
expected libc++ headers to be provided by the toolcain at the time.
Apple has since switched to including libc++ headers in the platform SDK
instead of in the toolchain, so that specific motivation no longer
applies either.)
Differential Revision: https://reviews.llvm.org/D159292