Page MenuHomePhabricator

[CMake][runtimes] Move libunwind, libc++abi and libc++ to lib/ and include/
ClosedPublic

Authored by phosek on Mar 6 2019, 12:10 AM.

Details

Summary

This change is a consequence of the discussion in "RFC: Place libs in
Clang-dedicated directories", specifically the suggestion that
libunwind, libc++abi and libc++ shouldn't be using Clang resource
directory. Tools like clangd make this assumption, but this is
currently not true for the LLVM_ENABLE_PER_TARGET_RUNTIME_DIR build.
This change addresses that by moving the output of these libraries to
lib/<target> and include/ directories, leaving resource directory only
for compiler-rt runtimes and Clang builtin headers.

Diff Detail

Event Timeline

phosek created this revision.Mar 6 2019, 12:10 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
smeenai added inline comments.Mar 6 2019, 10:15 AM
clang/lib/Driver/ToolChain.cpp
94

Is this supposed to append lib twice?

libcxx/CMakeLists.txt
421

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.

phosek marked an inline comment as done.Mar 6 2019, 11:54 AM
phosek added inline comments.
libcxx/CMakeLists.txt
421

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?

phosek updated this revision to Diff 189643.Mar 6 2019, 7:49 PM
phosek marked 2 inline comments as done.
smeenai added inline comments.Mar 6 2019, 8:42 PM
clang/test/Driver/linux-per-target-runtime-dir.c
13

Shouldn't this one be different now?

libcxx/CMakeLists.txt
421

It's expected for LIBCXX_LIBDIR_SUBDIR to always begin with a /, correct? I feel like it would be less error-prone to have places using that variable add the / rather than relying on the variable containing it. In particular, since the variable name contains SUBDIR, I wouldn't expect to have to specify the leading / of the subdirectory myself.

Case in point: if I'm reading correctly, DEFAULT_INSTALL_PREFIX will end with a trailing /, and then LIBCXX_INSTALL_LIBDIR will begin with a leading /, so you'll end up with two consecutive / when you join the two in lib/CMakeLists.txt. Not a big deal, but also easily avoidable, I think.

phosek updated this revision to Diff 189654.Mar 6 2019, 10:25 PM
phosek marked an inline comment as done.
phosek updated this revision to Diff 189656.Mar 6 2019, 11:35 PM
phosek marked an inline comment as done.
smeenai accepted this revision.Mar 7 2019, 12:01 AM

LGTM, thanks!

libcxx/CMakeLists.txt
420–421

These two variables are unused now, right? (and their equivalents in libcxxabi and libunwind)

424–425

string(APPEND) could be used here – I don't know why that's so uncommon (I only see one instance of it in the entirety of llvm-project right now).

This revision is now accepted and ready to land.Mar 7 2019, 12:01 AM
phosek marked an inline comment as done.Mar 7 2019, 12:59 AM
phosek added inline comments.
libcxx/CMakeLists.txt
420–421

Yes, this is something I actually introduced in previous iterations of the runtimes build efforts. We could consider deleting them altogether, but I don't know how to find out if there are any users of these. I could send an email to libcxx-dev and see if anyone responds.

phosek updated this revision to Diff 189795.Mar 7 2019, 3:40 PM
phosek marked 2 inline comments as done.
This revision was automatically updated to reflect the committed changes.