[runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++

[phosek]

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/c++ and include/c++ directories, leaving resource directory only
for compiler-rt runtimes and Clang builtin headers.

[phosek]

This is a reland of D59013, apart from fixing the failing test on Windows, it also changes one thing where libraries are installed in lib/clang/<target> rather than lib/<target> based on the discussion in "RFC: Place libs in Clang-dedicated directories (affects openmp, libcxx, libunwind, compiler-rt)".

[smeenai]

clang/test/Driver/linux-per-target-runtime-dir.c

15 ↗

(On Diff #189968)

Shouldn't this have the clang component now?

[phosek]

The layout currently looks as follows:

compiler-rt:
  headers: $prefix/lib/clang/$version/include
  libraries: $prefix/lib/clang/$version/$triple/lib/$name.$ext

libc++, libc++abi, libunwind:
  headers: $prefix/include/c++/v1
  libraries: $prefix/lib/clang/$triple/$name.$ext

So if we take x86_64-linux-gnu as an example target, it'd be:

include/c++/v1
lib/clang/x86_64-linux-gnu/{libc++.so,libc++abi.so,libunwind.so}
lib/clang/9.0.0/x86_64-linux-gnu/lib/libclang_rt.builtins.a

I'm not super enthusiastic about the duplicated triple, but the only way to eliminate it would be to move the Clang resource directory inside of lib/clang/x86_64-linux-gnu, i.e. we'd have lib/clang/x86_64-linux-gnu/9.0.0/{include,lib}.

[phosek]

clang/test/Driver/linux-per-target-runtime-dir.c

15 ↗

(On Diff #189968)

I haven't changed the location of headers, only libraries. Do you think we should move libc++ headers into clang subdirectory inside include as well?

[smeenai]

clang/test/Driver/linux-per-target-runtime-dir.c

15 ↗

(On Diff #189968)

This is -L, so it's the search path for the libraries, right?

[phosek]

clang/test/Driver/linux-per-target-runtime-dir.c

15 ↗

(On Diff #189968)

Sorry about that, I was looking at a wrong line, you're right.

[smeenai]

LGTM, though you may wanna wait for @jdenny too.

In D59168#1423587, @phosek wrote:

The layout currently looks as follows:

compiler-rt:
  headers: $prefix/lib/clang/$version/include
  libraries: $prefix/lib/clang/$version/$triple/lib/$name.$ext

libc++, libc++abi, libunwind:
  headers: $prefix/include/c++/v1
  libraries: $prefix/lib/clang/$triple/$name.$ext

So if we take x86_64-linux-gnu as an example target, it'd be:

include/c++/v1
lib/clang/x86_64-linux-gnu/{libc++.so,libc++abi.so,libunwind.so}
lib/clang/9.0.0/x86_64-linux-gnu/lib/libclang_rt.builtins.a

I'm not super enthusiastic about the duplicated triple, but the only way to eliminate it would be to move the Clang resource directory inside of lib/clang/x86_64-linux-gnu, i.e. we'd have lib/clang/x86_64-linux-gnu/9.0.0/{include,lib}.

I don't think the duplicated triple is too huge of a deal. I think the layout where the resource directory is moved inside the triple directory is a bit nicer, but I also don't know how much work that change would be and if it's worth it.

clang/test/Driver/linux-per-target-runtime-dir.c

15 ↗

(On Diff #189972)

Idk if it's worth making this a bit more specific – clang -### should print out its InstallerDir, so you could capture that in a FileCheck regex and use it to check the exact path.

[phosek]

In D59168#1424968, @smeenai wrote:

I don't think the duplicated triple is too huge of a deal. I think the layout where the resource directory is moved inside the triple directory is a bit nicer, but I also don't know how much work that change would be and if it's worth it.

One downside is that we would be duplicating Clang resource headers for each target which may not be too big of a deal but something worth mentioning.

clang/test/Driver/linux-per-target-runtime-dir.c

15 ↗

(On Diff #189972)

I did that in the previous version but that was breaking on Windows and haven't yet figured out how to make it work there.

[jdenny]

In D59168#1423587, @phosek wrote:

I'm not super enthusiastic about the duplicated triple, but the only way to eliminate it would be to move the Clang resource directory inside of lib/clang/x86_64-linux-gnu, i.e. we'd have lib/clang/x86_64-linux-gnu/9.0.0/{include,lib}.

lib/x86_64-linux-gnu/clang/9.0.0/{include,lib} would mean less duplication of the triple within system directories. Is there a reason not to do that? I'm not necessarily advocating for this. I'm just trying to understand what you've chosen.

In D59168#1425701, @phosek wrote:

In D59168#1424968, @smeenai wrote:

I don't think the duplicated triple is too huge of a deal. I think the layout where the resource directory is moved inside the triple directory is a bit nicer, but I also don't know how much work that change would be and if it's worth it.

One downside is that we would be duplicating Clang resource headers for each target which may not be too big of a deal but something worth mentioning.

In that case, could we symlink the target-specific include directories to a common directory?

Regardless of these points, your patch appears to strictly improve the situation, so maybe we should see how things go with it and then make further changes later if desired.

So, LGTM modulo the question I added inline. Thanks.

libcxx/lib/CMakeLists.txt

407 ↗

(On Diff #189972)

Assume LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True.

Without your patch, it looks to me like specifying LIBCXX_INSTALL_PREFIX collapses the library destination from $prefix/lib/clang/$version/$triple/lib to $prefix/lib.

With your patch, it looks to me like the library destination is always $prefix/lib/clang/$triple, and specifying LIBCXX_INSTALL_PREFIX simply changes $prefix.

Is that correct? If so, is that worthwhile to note in the summary?

[jdenny]

Is there anything I can do to help this patch make progress?

[phosek]

In D59168#1469186, @jdenny wrote:

Is there anything I can do to help this patch make progress?

I think it's ready to land. I was just waiting if anyone else wants to chime in, but it doesn't seem like.

I was also thinking about alternative names for the library path, specifically for headers we use include/c++ but for libraries we'll now use lib/clang/<target> which is not very consistent. I was considering lib/c++/<target> instead but that wouldn't work for libomp and I didn't come up with any other better alternatives.

[jdenny]

In D59168#1470578, @phosek wrote:

In D59168#1469186, @jdenny wrote:

I was also thinking about alternative names for the library path, specifically for headers we use include/c++ but for libraries we'll now use lib/clang/<target> which is not very consistent.

Does that inconsistency cause a practical problem?

I was considering lib/c++/<target> instead

A practical benefit might be the ability to use -L to expose clang's c++ libs without exposing other clang libs as well, but I'm not sure whether that's a real use case.

but that wouldn't work for libomp

If we decide libomp shouldn't be version-locked, then I guess lib/openmp/<target> would work.

Is it safe to assume that lib/c++ and lib/openmp would manage to be as clang-dedicated as lib/clang? If other packages don't install to include/c++, then I suppose they don't install to lib/c++.

[phosek]

In D59168#1472152, @jdenny wrote:

In D59168#1470578, @phosek wrote:

In D59168#1469186, @jdenny wrote:

I was also thinking about alternative names for the library path, specifically for headers we use include/c++ but for libraries we'll now use lib/clang/<target> which is not very consistent.

Does that inconsistency cause a practical problem?

Not at the moment as far as I'm aware, but I'd like to make sure we consider all aspects to avoid more transitions in the future.

I was considering lib/c++/<target> instead

A practical benefit might be the ability to use -L to expose clang's c++ libs without exposing other clang libs as well, but I'm not sure whether that's a real use case.

This may be also beneficial for other compilers that would like to support Clang's C++ library.

but that wouldn't work for libomp

If we decide libomp shouldn't be version-locked, then I guess lib/openmp/<target> would work.

SGTM

Is it safe to assume that lib/c++ and lib/openmp would manage to be as clang-dedicated as lib/clang? If other packages don't install to include/c++, then I suppose they don't install to lib/c++.

AFAIK no other libraries currently use lib/c++.

[jdenny]

Does the following match what you have in mind?

$prefix/
  include/
    c++/
      v1/
        limits.h
        ...
    openmp/
      v1/ <------ I don't think openmp has anything like this now
        omp.h
        ompt.h
        ...
  lib/
    c++/
      $target/
        libc++abi.so
        ...
    openmp/
      $target/
        libomp.so
        ...
    clang/
      9.0.0/ <----- resource directory
        include/
        lib/
          $target/
            libclang_rt.asan_cxx.a
            ...

[phosek]

In D59168#1474715, @jdenny wrote:

Does the following match what you have in mind?

$prefix/
  include/
    c++/
      v1/
        limits.h
        ...
    openmp/
      v1/ <------ I don't think openmp has anything like this now

Iibc++ uses this scheme to allow the possibility of evolving the API but I'm not sure if it's necessary for openmp.

      omp.h
      ompt.h
      ...
lib/
  c++/
    $target/
      libc++abi.so
      ...
  openmp/
    $target/
      libomp.so
      ...
  clang/
    9.0.0/ <----- resource directory
      include/
      lib/
        $target/
          libclang_rt.asan_cxx.a
          ...

Almost with a small variation:

$prefix/
  include/
    c++/
      v1/
        limits.h
        ...
    openmp/
      omp.h
      ompt.h
      ...
  lib/
    $target/ <--- This way we don't have to duplicate $target in each subdirectory and it matches the layout of the resource directory
      c++/
        libc++abi.so
        ...
      openmp/
        libomp.so
        ...
    clang/
      9.0.0/
        include/
        lib/
          $target/
            libclang_rt.asan_cxx.a
            ...

WDYT?

[jdenny]

In D59168#1479939, @phosek wrote:

In D59168#1474715, @jdenny wrote:

Does the following match what you have in mind?

$prefix/
  include/
    c++/
      v1/
        limits.h
        ...
    openmp/
      v1/ <------ I don't think openmp has anything like this now

Iibc++ uses this scheme to allow the possibility of evolving the API but I'm not sure if it's necessary for openmp.

It seems we have roughly the same situation for their ABIs. That is, for .so files, someone noted that libc++ and libunwind have always used the same version, .1, and openmp doesn't use a version (except in Debian packages). In other words, for each of these three, a change in ABI compatibility has never been indicated. For openmp only, it's apparently assumed a change never will need to be indicated (except in Debian packages).

There may be some very good rationale for why openmp is different in this way, but I haven't yet learned what it is.

      omp.h
      ompt.h
      ...
lib/
  c++/
    $target/
      libc++abi.so
      ...
  openmp/
    $target/
      libomp.so
      ...
  clang/
    9.0.0/ <----- resource directory
      include/
      lib/
        $target/
          libclang_rt.asan_cxx.a
          ...

Almost with a small variation:

$prefix/
  include/
    c++/
      v1/
        limits.h
        ...
    openmp/
      omp.h
      ompt.h
      ...
  lib/
    $target/ <--- This way we don't have to duplicate $target in each subdirectory

We duplicate c++, openmp, etc. in each $target instead. If you have only one target, I guess that's better. Otherwise, does it matter?

and it matches the layout of the resource directory

Does the resource directory have any subproject directories, like c++, openmp, etc? If not, then it matches just as well either way. You just remove the level of subproject directories, right?

  c++/
    libc++abi.so
    ...
  openmp/
    libomp.so
    ...
clang/
  9.0.0/
    include/
    lib/
      $target/
        libclang_rt.asan_cxx.a
        ...

WDYT?

To be clear, I'm not opposed to your proposals. I'm just trying to understand the relative advantages. Thanks.

[phosek]

In D59168#1480428, @jdenny wrote:

It seems we have roughly the same situation for their ABIs. That is, for .so files, someone noted that libc++ and libunwind have always used the same version, .1, and openmp doesn't use a version (except in Debian packages). In other words, for each of these three, a change in ABI compatibility has never been indicated. For openmp only, it's apparently assumed a change never will need to be indicated (except in Debian packages).

They already use .2 for ABI v2 which is currently considered unstable but some project like Fuchsia already use. There's also an ongoing discussion to stabilize v2. However, this is only affecting the ABI and therefore .so, not headers which will continue using v1 as far as I'm aware (since the API is defined by the C++ standard after all).

There may be some very good rationale for why openmp is different in this way, but I haven't yet learned what it is.

@EricWF might have some insight into this.

We duplicate c++, openmp, etc. in each $target instead. If you have only one target, I guess that's better. Otherwise, does it matter?

Not every target may distribute or even support every runtime. For example, we don't currently ship openmp on Fuchsia because we haven't ported it yet. Similarly we don't ship every compiler-rt runtime.

and it matches the layout of the resource directory

Does the resource directory have any subproject directories, like c++, openmp, etc? If not, then it matches just as well either way. You just remove the level of subproject directories, right?

Not at the moment, but when designing the current resource directory layout, I haven't given this much though to various aspect so I wouldn't take it as indicative.

To be clear, I'm not opposed to your proposals. I'm just trying to understand the relative advantages. Thanks.

That makes sense, I'd like to understand various requirements for different runtimes before settling on the final layout. Updating this patch should be straightforward.

[phosek]

I've updated and rebased the patch, let me know what you think.

[jdenny]

In D59168#1480801, @phosek wrote:

In D59168#1480428, @jdenny wrote:

It seems we have roughly the same situation for their ABIs. That is, for .so files, someone noted that libc++ and libunwind have always used the same version, .1, and openmp doesn't use a version (except in Debian packages). In other words, for each of these three, a change in ABI compatibility has never been indicated. For openmp only, it's apparently assumed a change never will need to be indicated (except in Debian packages).

They already use .2 for ABI v2 which is currently considered unstable but some project like Fuchsia already use. There's also an ongoing discussion to stabilize v2.

Thanks, I wasn't aware. I still see only .1 in my build, so apparently a different config is needed.

libunwind/CMakeLists.txt

190 ↗

(On Diff #197241)

I naively assumed libunwind would have its own directory so it could be selected independently. Does this mean any library for C++ goes in c++?

[phosek]

libunwind/CMakeLists.txt

190 ↗

(On Diff #197241)

We could do that, but I'm not sure if it's worth doing without a clear use case? Do you know of any client that would want to consume libunwind independently of other C++ libraries? I'm aware of Rust but they build libunwind independently so there's no need for this.

[phosek]

In D59168#1484467, @jdenny wrote:

In D59168#1480801, @phosek wrote:

In D59168#1480428, @jdenny wrote:

It seems we have roughly the same situation for their ABIs. That is, for .so files, someone noted that libc++ and libunwind have always used the same version, .1, and openmp doesn't use a version (except in Debian packages). In other words, for each of these three, a change in ABI compatibility has never been indicated. For openmp only, it's apparently assumed a change never will need to be indicated (except in Debian packages).

They already use .2 for ABI v2 which is currently considered unstable but some project like Fuchsia already use. There's also an ongoing discussion to stabilize v2.

Thanks, I wasn't aware. I still see only .1 in my build, so apparently a different config is needed.

It's the LIBCXX_ABI_VERSION CMake option, see https://github.com/llvm/llvm-project/blob/master/libcxx/CMakeLists.txt#L121

[jdenny]

In D59168#1484754, @phosek wrote:

It's the LIBCXX_ABI_VERSION CMake option, see https://github.com/llvm/llvm-project/blob/master/libcxx/CMakeLists.txt#L121

Thanks.

libunwind/CMakeLists.txt

190 ↗

(On Diff #197241)

I'm afraid I have no idea. I'm not close enough to these libraries to be an adequate reviewer by myself. Another reviewer should probably take a look at the new version of the patch.

[phosek]

libunwind/CMakeLists.txt

190 ↗

(On Diff #197241)

@EricWF @rsmith is this something you have an opinion on (or can you recommend someone who might)?

[echristo]

Adding Sterling and Chris to this to take a look at the new layout :)

[kparzysz]

This commit stopped clang++ from finding libc++:

main.cc:

#include <vector>

int main() {
  std::vector<int> x;
  return x.size();
}

$ clang++ main.cc -stdlib=libc++ -v
clang version 9.0.0 (/w/src/llvm.org/tools/clang 754813c4737a444eeb4ea5b4070073dd251504cc) (/w/src/llvm.org e145e44a7c34acf851da0d6a2c66b274b6ebc82d)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /w/c/org/bin
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8.4
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9.3
Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Candidate multilib: x32;@mx32
Selected multilib: .;@m64
 "/w/c/org/bin/clang-9" -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -main-file-name main.cc -mrelocation-model static -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -dwarf-column-info -debugger-tuning=gdb -v -resource-dir /w/c/org/lib/clang/9.0.0 -internal-isystem /usr/local/include -internal-isystem /w/c/org/lib/clang/9.0.0/include -internal-externc-isystem /usr/include/x86_64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -fdebug-compilation-dir /w/test -ferror-limit 19 -fmessage-length 0 -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -fcolor-diagnostics -faddrsig -o /tmp/main-32e7f3.o -x c++ main.cc
clang -cc1 version 9.0.0 based upon LLVM 9.0.0svn default target x86_64-unknown-linux-gnu
ignoring nonexistent directory "/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /w/c/org/lib/clang/9.0.0/include
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
main.cc:1:10: fatal error: 'vector' file not found
#include <vector>
         ^~~~~~~~
1 error generated.

lib/Driver/ToolChain.cpp

 }
 
 ToolChain::ToolChain(const Driver &D, const llvm::Triple &T,
                      const ArgList &Args)
     : D(D), Triple(T), Args(Args), CachedRTTIArg(GetRTTIArgument(Args)),
       CachedRTTIMode(CalculateRTTIMode(Args, Triple, CachedRTTIArg)) {
   SmallString<128> P;
 
+  if (D.CCCIsCXX()) {
+    P.assign(D.Dir);
+    llvm::sys::path::append(P, "..", "lib", D.getTargetTriple(), "c++");
+    if (getVFS().exists(P))
+      getLibraryPaths().push_back(P.str());
+  }
+
   P.assign(D.ResourceDir);
   llvm::sys::path::append(P, D.getTargetTriple(), "lib");
   if (getVFS().exists(P))
     getLibraryPaths().push_back(P.str());
 
   P.assign(D.ResourceDir);
   llvm::sys::path::append(P, Triple.str(), "lib");
   if (getVFS().exists(P))

lib/Driver/ToolChains/Fuchsia.cpp

 void Fuchsia::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
                                            ArgStringList &CC1Args) const {
   if (DriverArgs.hasArg(options::OPT_nostdlibinc) ||
       DriverArgs.hasArg(options::OPT_nostdincxx))
     return;
 
   switch (GetCXXStdlibType(DriverArgs)) {
   case ToolChain::CST_Libcxx: {
-    SmallString<128> P(getDriver().ResourceDir);
-    llvm::sys::path::append(P, "include", "c++", "v1");
+    SmallString<128> P(getDriver().Dir);
+    llvm::sys::path::append(P, "..", "include", "c++", "v1");
     addSystemInclude(DriverArgs, CC1Args, P.str());
     break;
   }
 
   default:
     llvm_unreachable("invalid stdlib name");
   }
 }

lib/Driver/ToolChains/Linux.cpp

static std::string DetectLibcxxIncludePath(StringRef base) {
   }
   return MaxVersion ? (base + "/" + MaxVersionString).str() : "";
 }
 
 void Linux::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                                   llvm::opt::ArgStringList &CC1Args) const {
   const std::string& SysRoot = computeSysRoot();
   const std::string LibCXXIncludePathCandidates[] = {
-      DetectLibcxxIncludePath(getDriver().ResourceDir + "/include/c++"),
       DetectLibcxxIncludePath(getDriver().Dir + "/../include/c++"),
       // If this is a development, non-installed, clang, libcxx will
       // not be found at ../include/c++ but it likely to be found at
       // one of the following two locations:
       DetectLibcxxIncludePath(SysRoot + "/usr/local/include/c++"),
       DetectLibcxxIncludePath(SysRoot + "/usr/include/c++") };
   for (const auto &IncludePath : LibCXXIncludePathCandidates) {
     if (IncludePath.empty() || !getVFS().exists(IncludePath))

test/Driver/linux-per-target-runtime-dir.c

-// RUN: %clang -no-canonical-prefixes -x c++ %s -### -o %t.o 2>&1 \
+// RUN: %clangxx -no-canonical-prefixes -x c++ %s -### -o %t.o 2>&1 \
 // RUN:     --target=x86_64-linux-gnu \
 // RUN:     -stdlib=libc++ \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:     -ccc-install-dir %S/Inputs/basic_linux_libcxx_tree/usr/bin \
 // RUN:     --gcc-toolchain="" \
 // RUN:     --sysroot=%S/Inputs/basic_linux_libcxx_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-PER-TARGET-RUNTIME %s
 // CHECK-PER-TARGET-RUNTIME: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-PER-TARGET-RUNTIME: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-PER-TARGET-RUNTIME: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[RESDIR]]/include/c++/v1"
+// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}/../include/c++/v1"
 // CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
+// CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|\\\\}}..{{/|\\\\}}lib{{/|\\\\}}x86_64-linux-gnu{{/|\\\\}}c++"
 // CHECK-PER-TARGET-RUNTIME: "-L[[RESDIR]]{{/|\\\\}}x86_64-linux-gnu{{/|\\\\}}lib"
 
 // RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name 2>&1 \
 // RUN:     --target=x86_64-linux-gnu \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-X8664 %s
 // CHECK-CLANGRT-X8664: x86_64-linux-gnu{{/|\\}}lib{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN:     --target=x86_64-linux-gnu \
 // RUN:     -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-X8664 %s
 // CHECK-FILE-NAME-X8664: x86_64-linux-gnu{{/|\\}}lib{{/|\\}}libclang_rt.builtins.a