[PATCH] D59013: [CMake][runtimes] Move libunwind, libc++abi and libc++ to lib/ and include/

Wed Mar 6 11:54:57 PST 2019

phosek marked an inline comment as done.
phosek added inline comments.

================
Comment at: libcxx/CMakeLists.txt:421
+  set(DEFAULT_INSTALL_PREFIX lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE}/)
+  set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}${LIBCXX_LIBDIR_SUFFIX})
+  set(LIBCXX_HEADER_DIR ${LLVM_BINARY_DIR})
----------------
smeenai wrote:
> Having the libdir suffix after the target triple looks weird ... also, the driver code doesn't appear to be taking the possibility of a suffix into account? I might be missing something here.
This is a bit of a hack, we allow building runtimes with sanitizer instrumentation and we install those sanitized versions into `lib/<target>/<sanitizer>`, e.g. `lib/x86_64-fuchsia/asan`. Those are never linked against (the ABI should be the same as for the non-sanitized version), they're only intended for packaging, hence the missing driver logic. We use the `<RUNTIME>_LIBDIR_SUFFIX` to set the `<sanitizer>` portion of the install path, see https://github.com/llvm/llvm-project/blob/master/llvm/runtimes/CMakeLists.txt#L517. However, if anyone ever tries to use both the `LIBDIR_SUFFIX` and instrumentation, things are going to break. Maybe we should introduce another variable for this, e.g. `<RUNTIME>_LIBDIR_VARIANT`?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59013/new/

https://reviews.llvm.org/D59013