Page MenuHomePhabricator

[OpenMP] Add libs to clang-dedicated directories
Needs ReviewPublic

Authored by jdenny on Dec 14 2018, 2:57 PM.

Details

Summary

Clang by default prefers system openmp libraries over the ones built
alongside clang. The effect is that clang-compiled openmp
applications sometimes misbehave if the user doesn't adjust the
library search path.

For example, where $LLVM_INSTALL_DIR is something like
/home/jdenny/llvm and is built from git master, but where my system
openmp libraries under /usr/lib/x86_64-linux-gnu are built from LLVM
6.0:

$ cd $LLVM_INSTALL_DIR
$ cat test.c
 #include <stdio.h>
int main() {
  #pragma omp target teams
  printf("hello\n");
  return 0;
}
$ ./bin/clang -fopenmp -fopenmp-targets=nvptx64 test.c
$ ./a.out
./a.out: error while loading shared libraries: libomptarget.so: cannot open shared object file: No such file or directory
$ LD_LIBRARY_PATH=lib ./a.out
Segmentation fault (core dumped)
$ LD_LIBRARY_PATH=lib ldd a.out | grep libomp
        libomp.so.5 => /usr/lib/x86_64-linux-gnu/libomp.so.5 (0x00007fab59748000)
        libomptarget.so => lib/libomptarget.so (0x00007fab59515000)

Of course, I can specify -L to link against the right libomp.so, but
it seems like clang should just do that for me automatically.

This patch makes that happen and addresses three use cases:

  1. openmp and clang subprojects are built together: In this case, in both the build and install trees, all openmp libraries appear in the lib directory as they do without this patch, but they also appear in a clang-dedicated directory, lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib, for which clang then adds a -L. This patch doesn't need to adjust clang for that to happen as clang without this patch already looks in that directory. 9.0.0 and x86_64-unknown-linux-gnu are expanded from PACKAGE_VERSION and LLVM_DEFAULT_TARGET_TRIPLE.
  1. openmp is built standalone and either (a) the new cmake variables OPENMP_CLANG_VERSION and OPENMP_CLANG_TARGET_TRIPLE are specified or, if not, (b) their values are successfully obtained from the selected C++ compiler because it's a clang that supports the --print-target-triple option:
    • In the install tree, openmp libraries appear in the same places as in case 1. Automatically computing the name of the clang-dedicated directory when the C++ compiler is clang and then installing copies of the openmp libraries there is useful in the following scenario: building openmp after clang in order to build bitcode libraries as in step 4 in the following instructions:

      https://www.hahnjo.de/blog/2018/10/08/clang-7.0-openmp-offloading-nvidia.html
    • In the build tree, openmp libraries appear only where they do without this patch (various directories in the build tree). In this case, I'm not sure how a clang-dedicated directory can be useful given that whatever clang you're using doesn't know where to find the openmp build directory.
  1. openmp is built standalone and either OPENMP_CLANG_VERSION or OPENMP_CLANG_TARGET_TRIPLE is not specified or computed: In this case, in both the build and install trees, openmp libraries appear only where they do without this patch (for the install tree, that's the main lib directory). We cannot create clang-dedicated directories when there's no clue what clang the standalone openmp build was intended for.

Diff Detail

Event Timeline

jdenny created this revision.Dec 14 2018, 2:57 PM
jdenny updated this revision to Diff 180193.Jan 3 2019, 7:58 PM

Rebased. Ping.

jdenny updated this revision to Diff 183004.Jan 22 2019, 6:06 PM
jdenny edited the summary of this revision. (Show Details)
jdenny set the repository for this revision to rOMP OpenMP.

Rebased. Ping.

If this patch is not the right thing to do, or if it conflicts with existing plans for openmp library organization, please let me know. I'd like to understand the best path forward.

In general, this makes sense to me.

openmp/CMakeLists.txt
69

The way this is computed looks different to me than what I believe to be the corresponding code in libc++'s file. It's this, right?

string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION
       ${PACKAGE_VERSION})

if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
  set(DEFAULT_INSTALL_PREFIX lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/)
  set(DEFAULT_INSTALL_HEADER_PREFIX lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/)
  set(LIBCXX_LIBRARY_DIR 
${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/lib${LIBCXX_LIBDIR_SUFFIX})
  set(LIBCXX_HEADER_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION})
elseif(LLVM_LIBRARY_OUTPUT_INTDIR)
  set(LIBCXX_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
  set(LIBCXX_HEADER_DIR  ${LLVM_BINARY_DIR})
else()
  set(LIBCXX_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LIBCXX_LIBDIR_SUFFIX})
endif()
jdenny added inline comments.Jan 23 2019, 7:35 PM
openmp/CMakeLists.txt
69

Thanks for reviewing. It seems like each subproject has a slightly different formula.

What this patch does here is similar to what's in compiler-rt/cmake/base-config-ix.cmake except this patch doesn't attempt to observe LLVM_ENABLE_PER_TARGET_RUNTIME_DIR. Perhaps it should?

Below is some of the relevant code from that script (I've omitted many seemingly irrelevant lines):

if (LLVM_TREE_AVAILABLE)
  set(COMPILER_RT_OUTPUT_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION})
  set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
else()
  set(COMPILER_RT_OUTPUT_DIR ${CMAKE_CURRENT_BINARY_DIR} CACHE PATH
    "Path where built compiler-rt libraries should be stored.")
  set(COMPILER_RT_INSTALL_PATH ${CMAKE_INSTALL_PREFIX} CACHE PATH
    "Path where built compiler-rt libraries should be installed.")
endif()

if(NOT DEFINED COMPILER_RT_OS_DIR)
  string(TOLOWER ${CMAKE_SYSTEM_NAME} COMPILER_RT_OS_DIR)
endif()
if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
  set(COMPILER_RT_LIBRARY_OUTPUT_DIR
    ${COMPILER_RT_OUTPUT_DIR})
  set(COMPILER_RT_LIBRARY_INSTALL_DIR
    ${COMPILER_RT_INSTALL_PATH})
else(LVM_ENABLE_PER_TARGET_RUNTIME_DIR)
  set(COMPILER_RT_LIBRARY_OUTPUT_DIR
    ${COMPILER_RT_OUTPUT_DIR}/lib/${COMPILER_RT_OS_DIR})
  set(COMPILER_RT_LIBRARY_INSTALL_DIR
    ${COMPILER_RT_INSTALL_PATH}/lib/${COMPILER_RT_OS_DIR})
endif()
hfinkel added inline comments.Jan 23 2019, 7:48 PM
openmp/CMakeLists.txt
69

... It seems like each subproject has a slightly different formula.

I can certainly believe that.

What this patch does here is similar to what's in compiler-rt/cmake/base-config-ix.cmake

SGTM.

except this patch doesn't attempt to observe LLVM_ENABLE_PER_TARGET_RUNTIME_DIR. Perhaps it should?

Yes, I think that it should.

I think this patch would effectively lose the ability to use a custom standalone build of the OpenMP runtime: When hacking on the runtime it's pretty convenient to only build and install the openmp repository and use environment variables to switch between different versions. When putting libomp.so into lib/clang/9.0.0/lib/linux/x86_64 [1] this path will be very first one that Clang instructs the linker to look at. As a result the "custom" location that I specify in LIBRARY_PATH won't be honored and I'll always end up using the version installed next to the compiler.

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

1: This path doesn't exist in a current installation of trunk. Is any other project using this path in its default settings? Sorry, I haven't been following LLVM activities pretty closely for the last months.

Thanks for reviewing.

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

LIBRARY_PATH during compilation has no effect for me.

First, -v reveals that -L/home/jdenny/llvm/bin/../lib is before -L/home/jdenny/llvm/lib, which is added by LIBRARY_PATH, which is thus redundant here.

Second, -v also reveals that -L/usr/lib/gcc/x86_64-linux-gnu/7.3.0/../../../x86_64-linux-gnu is before -L/home/jdenny/llvm/bin/../lib, and I have:

$ ls -l /usr/lib/x86_64-linux-gnu/libomp.so
lrwxrwxrwx 1 root root 11 Jan  7  2018 /usr/lib/x86_64-linux-gnu/libomp.so -> libomp.so.5

As a result:

$ readelf -d a.out | grep libomp.so
 0x0000000000000001 (NEEDED)             Shared library: [libomp.so.5]

Now LD_LIBRARY_PATH cannot force a.out to find Clang's libomp.so because a.out wants libomp.so.5.

My guess is that you don't have such a libomp.so.5 sitting in a system library directory, and so environment variables seem to work for you. If you instead have a libomp.so in a system library directory, Clang is probably incorrectly pointing to it for linking, but, linking manages to succeed anyway, so LD_LIBRARY_PATH can then locate the right one.

Is there something in my setup that seems uncommon? Surely many people who wish to build Clang locally could find themselves in this situation.

I think this patch would effectively lose the ability to use a custom standalone build of the OpenMP runtime: When hacking on the runtime it's pretty convenient to only build and install the openmp repository and use environment variables to switch between different versions. When putting libomp.so into lib/clang/9.0.0/lib/linux/x86_64 [1] this path will be very first one that Clang instructs the linker to look at. As a result the "custom" location that I specify in LIBRARY_PATH won't be honored and I'll always end up using the version installed next to the compiler.

You want a Clang that has its own libomp.so to be told to use a different libomp.so? And you want to tell Clang that via LIBRARY_PATH not -L? Is that correct?

First, I think your use case can suffer from the same problem with system libraries that I just described. In that situation, LIBRARY_PATH doesn't seem to help locate the desired libomp.so, regardless of whether it's installed alongside Clang or installed from a standalone openmp repository. However, I think you're not seeing that problem because your system libraries happen not to conflict.

Second, if you have a script, could you use something like -L$MY_ENV_VAR? Unlike LIBRARY_PATH, -L does override system library directories for me, so I think -L is the right solution for your use case.

In summary, it seems this patch fixes confusing behavior for a use case common to LLVM users: building Clang in their home directories. This patch breaks a use case that is more common to LLVM developers: using LIBRARY_PATH to mix versions of Clang and the OpenMP runtime. However, that use case was broken anyway, and switching from LIBRARY_PATH to -L fixes that use case in both regards.

Does all that sound right?

1: This path doesn't exist in a current installation of trunk. Is any other project using this path in its default settings? Sorry, I haven't been following LLVM activities pretty closely for the last months.

I thought I had a decent solution because Clang already looks in that directory. However, each subproject seems to do something a little different, and LLVM_ENABLE_PER_TARGET_RUNTIME_DIR affects the result. I'm trying to better understand this now.

I think this patch would effectively lose the ability to use a custom standalone build of the OpenMP runtime: When hacking on the runtime it's pretty convenient to only build and install the openmp repository and use environment variables to switch between different versions. When putting libomp.so into lib/clang/9.0.0/lib/linux/x86_64 [1] this path will be very first one that Clang instructs the linker to look at. As a result the "custom" location that I specify in LIBRARY_PATH won't be honored and I'll always end up using the version installed next to the compiler.

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

1: This path doesn't exist in a current installation of trunk. Is any other project using this path in its default settings? Sorry, I haven't been following LLVM activities pretty closely for the last months.

I don't believe that this is a concern. User-provided link paths, with -L, are added before the toolchain-provided paths. If you look at ToolChains/Gnu.cpp the ordering is:

Args.AddAllArgs(CmdArgs, options::OPT_L);
Args.AddAllArgs(CmdArgs, options::OPT_u);

ToolChain.AddFilePathLibArgs(Args, CmdArgs);

so the -L things are added first, then the ToolChain-provided paths.

Of course, we should regardless prioritize the ease of our users vs. our own convenience (and you could just not have an in-tree build in your development environment). However, I think you're okay either way.

I'm trying to better understand how various LLVM subprojects currently (without this patch) install to Clang-dedicated library directories. I'm building with the following:

-DLLVM_ENABLE_PROJECTS='clang;openmp;libcxx;libcxxabi;lldb;compiler-rt;lld;polly;libunwind'

I then see the following effects from -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True:

  • Rename the Clang-dedicated directory from lib/clang/9.0.0/lib/linux to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib.
  • For the libclang_rt.*-x86_64.* there, drop the now redundant -x86_64. components from their names.
  • Move libc++* and libunwind* from lib to the Clang-dedicated directory.

The OpenMP libraries to consider are:

  • libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?
  • libomp.so
  • libomptarget.rtl.x86_64.so, libomptarget.so
  • libomptarget-nvptx.a, libomptarget.rtl.cuda.so

My plan for the non-symlinks is:

  • Always install to lib in case that matters for backward compatibility.
  • With -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True, additionally install to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib.
  • Without -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True, where should they be additionally installed?
    • Just lib is bad for users because the problem this patch tries to solve then remains in the default case.
    • lib/clang/9.0.0/lib/linux currently appears to be specific to libclang_rt.*. Unless I missed something, we'd have to modify Clang to get it to find other libraries there.
    • lib/clang/9.0.0/lib/linux/x86_64 is what this patch currently chooses as it's another place Clang looks. But is that any better or worse than the next option?
    • lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib would mean -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True is effectively always set for OpenMP. Is that bad? I'm planning to choose this option unless there are objections.

I then see the following effects from -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True:

  • Rename the Clang-dedicated directory from lib/clang/9.0.0/lib/linux to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib.
  • For the libclang_rt.*-x86_64.* there, drop the now redundant -x86_64. components from their names.
  • Move libc++* and libunwind* from lib to the Clang-dedicated directory.

I meant to mention that, without -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True, I find only libclang_rt.*-x86_64.* in the Clang-dedicated library directory.

libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

No, I see no reason to change that.

lib/clang/9.0.0/lib/linux/x86_64 is what this patch currently chooses as it's another place Clang looks. But is that any better or worse than the next option?

So Clang always searches this directory but we never put anything there? I vaguely recall that this is where the compiler-rt libs used to be.

Move libc++* and libunwind* from lib to the Clang-dedicated directory.

So we have the same problem with libc++ and friends as well? I'd prefer that whatever solution we come up with here can be uniformly applied across subprojects. Any reason of which you can think for why libc++ and libomp should have different behaviors in this regard?

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

LIBRARY_PATH during compilation has no effect for me.

First, -v reveals that -L/home/jdenny/llvm/bin/../lib is before -L/home/jdenny/llvm/lib, which is added by LIBRARY_PATH, which is thus redundant here.

Are you saying a manual -L trumps the environment variable LIBRARY_PATH? Yes, that makes sense, I guess.

Second, -v also reveals that -L/usr/lib/gcc/x86_64-linux-gnu/7.3.0/../../../x86_64-linux-gnu is before -L/home/jdenny/llvm/bin/../lib, and I have:

$ ls -l /usr/lib/x86_64-linux-gnu/libomp.so
lrwxrwxrwx 1 root root 11 Jan  7  2018 /usr/lib/x86_64-linux-gnu/libomp.so -> libomp.so.5

I think now I'm getting what all of this is about, sorry for the misunderstanding! No, I don't have a "global" installation of Clang, I only have multiple custom installations of Clang in various places.

As a result:

$ readelf -d a.out | grep libomp.so
 0x0000000000000001 (NEEDED)             Shared library: [libomp.so.5]

Now LD_LIBRARY_PATH cannot force a.out to find Clang's libomp.so because a.out wants libomp.so.5.

Please note that libomp.so.5 is an invention of Debian. None of upstream's CMake files will ever create a symlink with that name!
(We could think about adding such symlink, there was a complaint sometime ago that we don't have versioned shared libraries. But that wouldn't solve all problems, the linker would still use the "wrong" file.)

My guess is that you don't have such a libomp.so.5 sitting in a system library directory, and so environment variables seem to work for you. If you instead have a libomp.so in a system library directory, Clang is probably incorrectly pointing to it for linking, but, linking manages to succeed anyway, so LD_LIBRARY_PATH can then locate the right one.

Is there something in my setup that seems uncommon? Surely many people who wish to build Clang locally could find themselves in this situation.

I guess it will become more common that distros package Clang with all its libraries.

I think this patch would effectively lose the ability to use a custom standalone build of the OpenMP runtime: When hacking on the runtime it's pretty convenient to only build and install the openmp repository and use environment variables to switch between different versions. When putting libomp.so into lib/clang/9.0.0/lib/linux/x86_64 [1] this path will be very first one that Clang instructs the linker to look at. As a result the "custom" location that I specify in LIBRARY_PATH won't be honored and I'll always end up using the version installed next to the compiler.

You want a Clang that has its own libomp.so to be told to use a different libomp.so? And you want to tell Clang that via LIBRARY_PATH not -L? Is that correct?

First, I think your use case can suffer from the same problem with system libraries that I just described. In that situation, LIBRARY_PATH doesn't seem to help locate the desired libomp.so, regardless of whether it's installed alongside Clang or installed from a standalone openmp repository. However, I think you're not seeing that problem because your system libraries happen not to conflict.

Correct, it worked for the past years without problems.

Second, if you have a script, could you use something like -L$MY_ENV_VAR? Unlike LIBRARY_PATH, -L does override system library directories for me, so I think -L is the right solution for your use case.

In summary, it seems this patch fixes confusing behavior for a use case common to LLVM users: building Clang in their home directories. This patch breaks a use case that is more common to LLVM developers: using LIBRARY_PATH to mix versions of Clang and the OpenMP runtime. However, that use case was broken anyway, and switching from LIBRARY_PATH to -L fixes that use case in both regards.

(It works in cases without a global installation of libomp.so, but yes, it's broken in these scenarios.)

  • libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

Agreed.

My plan for the non-symlinks is:

  • Always install to lib in case that matters for backward compatibility.
  • With -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True, additionally install to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib.

SGTM. Did you consider a symlink to the shared library in lib to avoid duplication?

  • Without -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True, where should they be additionally installed?
    • Just lib is bad for users because the problem this patch tries to solve then remains in the default case.
    • lib/clang/9.0.0/lib/linux currently appears to be specific to libclang_rt.*. Unless I missed something, we'd have to modify Clang to get it to find other libraries there.
    • lib/clang/9.0.0/lib/linux/x86_64 is what this patch currently chooses as it's another place Clang looks. But is that any better or worse than the next option?
    • lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib would mean -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True is effectively always set for OpenMP. Is that bad? I'm planning to choose this option unless there are objections.

IMO adding a default for a subproject that is different from all others needs consideration.

Move libc++* and libunwind* from lib to the Clang-dedicated directory.

So we have the same problem with libc++ and friends as well? I'd prefer that whatever solution we come up with here can be uniformly applied across subprojects. Any reason of which you can think for why libc++ and libomp should have different behaviors in this regard?

Seconded. Once distros start packaging libc++ they'll hit the same problem, right?

  • libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

From my perspective these names are not for backward compatibility, but to trick GNU and Intel compiler to use the LLVM/OpenMP runtime, when it is found before their own OpenMP runtime. By adding the path of libomp.so to the LIBRARY_PATH, gcc/icc will pick up the LLVM/OpenMP runtime during compilation.

As long as libgomp does not implement OMPT and LLVM has no Fortran frontend, I would welcome the possibility to easily link this runtime into Fortran applications compiled with the GNU compiler.

lib/clang/9.0.0/lib/linux/x86_64 is what this patch currently chooses as it's another place Clang looks. But is that any better or worse than the next option?

So Clang always searches this directory

ToolChain::ToolChain in lib/Driver/ToolChain.cpp always looks for it (it's the last of three directories), so I think that means yes.

but we never put anything there?

Not that I've found in the configurations I've tried.

I vaguely recall that this is where the compiler-rt libs used to be.

I don't know.

Move libc++* and libunwind* from lib to the Clang-dedicated directory.

So we have the same problem with libc++ and friends as well?

Theoretically, I think so, but I haven't experienced it, perhaps because I'm just looking much more closely at openmp in my work.

I'd prefer that whatever solution we come up with here can be uniformly applied across subprojects.

Agreed, but it might be worthwhile to choose a guinea pig (openmp) first. I find it difficult to think of all the possible consequences of changes like this.

Any reason of which you can think for why libc++ and libomp should have different behaviors in this regard?

No, but I don't know why they didn't already choose the behavior I'm proposing for openmp. A few guesses:

  1. For some reason, it was decided to move libcxx and libunwind libraries into the Clang-dedicated directories. My guess has been that move could potentially break backward compatibility or maybe even some permanent and reasonable use case, so, for libcxx and libunwind, moving them is opt-in using -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True. This patch addresses that issue for openmp by, instead, duplicating the libraries (probably should instead use a symlink as Jonas suggested) rather than moving them.
  1. Still, my proposal is that placement of Clang's libraries early in the search path should be the default. I've shown why that's good. Is it ever bad? Is there ever a case where the user expects system libraries to have precedence over Clang's own libraries by default? I would guess a user should specify -L for that, but maybe some user installs alternative libraries ahead in the current path and expects that to be sufficient.

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

LIBRARY_PATH during compilation has no effect for me.

First, -v reveals that -L/home/jdenny/llvm/bin/../lib is before -L/home/jdenny/llvm/lib, which is added by LIBRARY_PATH, which is thus redundant here.

Are you saying a manual -L trumps the environment variable LIBRARY_PATH? Yes, that makes sense, I guess.

While that's true, that's not the point I was making here. You suggested I set LIBRARY_PATH to find Clang's libomp.so, so I set it to my installed library directory, /home/jdenny/llvm/lib. My point here is that Clang reads LIBRARY_PATH and adds -L/home/jdenny/llvm/lib to the linker command line after -L/home/jdenny/llvm/bin/../lib, which it adds regardless, so LIBRARY_PATH proves redundant.

In other words, LIBRARY_PATH generally doesn't seem helpful for Clang's own lib directory, even when there's no conflicting system library. Sorry, maybe that's not helpful to point out.

Now LD_LIBRARY_PATH cannot force a.out to find Clang's libomp.so because a.out wants libomp.so.5.

Please note that libomp.so.5 is an invention of Debian. None of upstream's CMake files will ever create a symlink with that name!
(We could think about adding such symlink, there was a complaint sometime ago that we don't have versioned shared libraries. But that wouldn't solve all problems, the linker would still use the "wrong" file.)

Let me see if I understand this. Sorry if I have it all mixed up. For some N, if Clang looks for libomp.so.N instead of libomp.so, and if we could be sure that all copies of libomp.so.N that might be seen are ABI-compatible, and if no such libomp.so.N is yet another symlink, then it shouldn't matter if the linker uses the wrong file, and all that matters is that LD_LIBRARY_PATH chooses the right one. Is that right? I feel like that ABI-compatibility assumption is shaky, but I don't have enough practical experience here.

My plan for the non-symlinks is:

  • Always install to lib in case that matters for backward compatibility.
  • With -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True, additionally install to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib.

SGTM. Did you consider a symlink to the shared library in lib to avoid duplication?

Good idea.

  • libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

From my perspective these names are not for backward compatibility, but to trick GNU and Intel compiler to use the LLVM/OpenMP runtime, when it is found before their own OpenMP runtime. By adding the path of libomp.so to the LIBRARY_PATH, gcc/icc will pick up the LLVM/OpenMP runtime during compilation.

As long as libgomp does not implement OMPT and LLVM has no Fortran frontend, I would welcome the possibility to easily link this runtime into Fortran applications compiled with the GNU compiler.

On the one hand, it sounds like you're saying that putting these symlinks into Clang-dedicated directories would help your use case... because then you could easily raise their precedence for other compilers without raising the precedence of lots of other system libraries that happen to be installed in the same lib directory. Is that right? However, might it be harmful that you're also raising the precedence of other unrelated Clang libraries that also happen to be installed in the Clang-dedicated directory?

On the other hand, because this patch raises the precedence of Clang's libraries, this patch makes it harder to reverse your use case: to make Clang use another compiler's libraries. Do we care? Generally, making one compiler use another compiler's libraries probably ought to be harder than making the compiler use its own libraries correctly, so my guess is no.

Agreed, but it might be worthwhile to choose a guinea pig (openmp) first. I find it difficult to think of all the possible consequences of changes like this.

I feel it's likely there are some things we're not considering, but as a result, the prudent thing to do is to propose the change for libc++ also. We'll, potentially, get a much-wider set of feedback and, based on that, be able to make a better-informed decision. That's the path I prefer that we take. I recommend that you write an RFC for cfe-dev proposing to apply this change to all runtime libraries (openmp, libc++, etc.). It might turn out that there are technical reasons why this would not work for some libraries and would work for others, but I think that we should understand what those reasons are before we proceed. Once we've collected this feedback, the ordering in which we try applying the change to specific projects can be determined. libomp can then be the guinea pig if that's the logical thing to do.

Agreed, but it might be worthwhile to choose a guinea pig (openmp) first. I find it difficult to think of all the possible consequences of changes like this.

I feel it's likely there are some things we're not considering, but as a result, the prudent thing to do is to propose the change for libc++ also. We'll, potentially, get a much-wider set of feedback and, based on that, be able to make a better-informed decision. That's the path I prefer that we take. I recommend that you write an RFC for cfe-dev proposing to apply this change to all runtime libraries (openmp, libc++, etc.). It might turn out that there are technical reasons why this would not work for some libraries and would work for others, but I think that we should understand what those reasons are before we proceed. Once we've collected this feedback, the ordering in which we try applying the change to specific projects can be determined. libomp can then be the guinea pig if that's the logical thing to do.

Sounds reasonable.

I'd like to bake this patch a little longer to apply some of the ideas we're discussing. I'll then post the RFC and point here before working on patches for other runtimes.

cfe-dev is sufficient for the RFC?

Agreed, but it might be worthwhile to choose a guinea pig (openmp) first. I find it difficult to think of all the possible consequences of changes like this.

I feel it's likely there are some things we're not considering, but as a result, the prudent thing to do is to propose the change for libc++ also. We'll, potentially, get a much-wider set of feedback and, based on that, be able to make a better-informed decision. That's the path I prefer that we take. I recommend that you write an RFC for cfe-dev proposing to apply this change to all runtime libraries (openmp, libc++, etc.). It might turn out that there are technical reasons why this would not work for some libraries and would work for others, but I think that we should understand what those reasons are before we proceed. Once we've collected this feedback, the ordering in which we try applying the change to specific projects can be determined. libomp can then be the guinea pig if that's the logical thing to do.

Sounds reasonable.

I'd like to bake this patch a little longer to apply some of the ideas we're discussing. I'll then post the RFC and point here before working on patches for other runtimes.

cfe-dev is sufficient for the RFC?

Thanks, and sounds good. Yes, cfe-dev should be sufficient (and cross-posting between dev lists is generally discouraged, except for announcements for which replies are not expected, because not everyone is subscribed to all lists).

jdenny updated this revision to Diff 183577.Jan 25 2019, 11:13 AM
jdenny edited the summary of this revision. (Show Details)
  • Rename openmp's Clang-dedicated directory from lib/clang/9.0.0/lib/linux/x86_64 to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib because the latter is what other subprojects we've looked at use.
  • When openmp and clang are built together, expand 9.0.0 and x86_64-unknown-linux-gnu from PACKAGE_VERSION and LLVM_DEFAULT_TARGET_TRIPLE.
  • When openmp is built standalone but the C++ compiler is a recent Clang, use Clang to get 9.0.0 and x86_64-unknown-linux-gnu (clang --print-target-triple tells us unless it's an old Clang). This is helpful when building openmp after clang in order to build bitcode libraries as in step 4 in the following instructions:

    https://www.hahnjo.de/blog/2018/10/08/clang-7.0-openmp-offloading-nvidia.html
  • Otherwise, OPENMP_CLANG_VERSION and OPENMP_CLANG_TARGET_TRIPLE can be specified as cmake variables.
  • As discussed, this patch still does not observe the cmake variable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR, which other subprojects observe. Instead, it always installs both to lib and to the Clang-dedicated directory normally selected by that option. An RFC will hopefully determine if this is really the better choice.
  • This patch still does not change things for the libiomp5.so and libgomp.so sym links . That discussion is ongoing.
  • This patch still copies rather than sym links between lib and the Clang-dedicated directory. The cmake create_symlink is UNIX-specific. I'm not a windows developer, so I'm not sure if the best formula is to skip these for windows or create copies instead. I'll research this, but maybe someone else already knows?

lib/clang/9.0.0/lib/linux/x86_64 is what this patch currently chooses as it's another place Clang looks. But is that any better or worse than the next option?

So Clang always searches this directory

ToolChain::ToolChain in lib/Driver/ToolChain.cpp always looks for it (it's the last of three directories), so I think that means yes.

but we never put anything there?

Not that I've found in the configurations I've tried.

I vaguely recall that this is where the compiler-rt libs used to be.

I don't know.

Suddenly, some of the story here came back to me. I apologize for forgetting this earlier.

Clang's -frtlib-add-rpath checks for lib/clang/9.0.0/lib/linux/x86_64 and then sets -rpath to it so that you don't have to set LD_LIBRARY_PATH for your executable. It doesn't check for the other Clang-dedicated directories. That was my original motivation to install libraries there rather than in lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib.

In clang/lib/Driver/ToolChains/CommonArgs.cpp, tools::addArchSpecificRPath is what performs that check, and it's called by tools::addOpenMPRuntime and addSanitizerRuntime. However, none of my configurations ever install any library (including a sanitizer or openmp library) in that directory.

I just checked my notes from a #llvm conversation, and I looked at D30015, D30700, and D45145, where this functionality was added. I was told all this might have been created for an android toolchain, so maybe most people don't see its benefit.

Should I change this patch back to using that directory so that -frtlib-add-rpath is useful for openmp (outside of android toolchains)? I don't know. I've noticed it does indeed locate the correct libomp.so, but offloading to my GPU doesn't happen (based on watching nvidia-smi to see what runs on it). Based on some googling I did back when I was writing this patch, it seems it might be something that's changed in the behavior of -rpath for transitive or indirect dependencies. Indeed, if I use -Wl,-rpath=lib -Wl,--disable-new-dtags to get the old -rpath behavior, offloading works fine. But old behavior is obviously not the path forward.

I think it's a good idea to have a proper RFC on cfe-dev about how to handle runtime libraries in case they are already installed in system directory. More pointers on historic discussions below.

  • libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

From my perspective these names are not for backward compatibility, but to trick GNU and Intel compiler to use the LLVM/OpenMP runtime, when it is found before their own OpenMP runtime. By adding the path of libomp.so to the LIBRARY_PATH, gcc/icc will pick up the LLVM/OpenMP runtime during compilation.

As long as libgomp does not implement OMPT and LLVM has no Fortran frontend, I would welcome the possibility to easily link this runtime into Fortran applications compiled with the GNU compiler.

I also value this use-case, and I think that should continue to work with this patch: The intention is to put libraries that Clang cares about early in the library search hierarchy. As other Compiler (Intel Compiler, GCC) don't know about and don't look into "Clang-dedicated" directories, it should be sufficient to have the symlinks in lib/? (which is not going to change)

  1. Still, my proposal is that placement of Clang's libraries early in the search path should be the default. I've shown why that's good. Is it ever bad? Is there ever a case where the user expects system libraries to have precedence over Clang's own libraries by default? I would guess a user should specify -L for that, but maybe some user installs alternative libraries ahead in the current path and expects that to be sufficient.

My memories are also coming back here: Searching for abandoned revisions resulted in quite some interesting patches that I authored two years ago:

In the following two revisions I actually proposed something similar for libc++, maybe some points are relevant (can't remember all the details, but the conclusions seems to be that the libc++ maintainers didn't like installing to a different directory):

Are you saying a manual -L trumps the environment variable LIBRARY_PATH? Yes, that makes sense, I guess.

While that's true, that's not the point I was making here. You suggested I set LIBRARY_PATH to find Clang's libomp.so, so I set it to my installed library directory, /home/jdenny/llvm/lib. My point here is that Clang reads LIBRARY_PATH and adds -L/home/jdenny/llvm/lib to the linker command line after -L/home/jdenny/llvm/bin/../lib, which it adds regardless, so LIBRARY_PATH proves redundant.

In other words, LIBRARY_PATH generally doesn't seem helpful for Clang's own lib directory, even when there's no conflicting system library. Sorry, maybe that's not helpful to point out.

Okay, got it this time. Thanks for the explanation, I missed what the ordering implies here.

Please note that libomp.so.5 is an invention of Debian. None of upstream's CMake files will ever create a symlink with that name!
(We could think about adding such symlink, there was a complaint sometime ago that we don't have versioned shared libraries. But that wouldn't solve all problems, the linker would still use the "wrong" file.)

Let me see if I understand this. Sorry if I have it all mixed up. For some N, if Clang looks for libomp.so.N instead of libomp.so, and if we could be sure that all copies of libomp.so.N that might be seen are ABI-compatible, and if no such libomp.so.N is yet another symlink, then it shouldn't matter if the linker uses the wrong file, and all that matters is that LD_LIBRARY_PATH chooses the right one. Is that right? I feel like that ABI-compatibility assumption is shaky, but I don't have enough practical experience here.

From my understanding the general idea is to have libomp.so -> libomp.so.N. When linking an application the linker resolves the link chain and adds libomp.so.N as a dependency. The assumption is that these versioned libraries will stay ABI-compatible for all times.
The point is that the OpenMP runtime libraries have a much stronger guarantee: The goal is to never break the ABI, we're only adding new functions and utilize additional bits that were left unused when introducing a new interface.
I don't really know why the Debian maintainers decided to add such symlink on their own, but that's part of the problem.

However, only using LD_LIBRARY_PATH during runtime might not be sufficient. Imagine there is a new version of the OpenMP standard which introduces a new feature. A new function is added to libomp and Clang is taught to generate code, calling the new function.
If linking ends up using the old libomp installed in a system directory, ld cannot find the new symbols and will fail. So I think ABI-compatibility is not equivalent to "the right library for linking" in all cases. Does that make sense?

I think it's a good idea to have a proper RFC on cfe-dev about how to handle runtime libraries in case they are already installed in system directory. More pointers on historic discussions below.

Agreed. And thanks for the pointers!

  • libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

From my perspective these names are not for backward compatibility, but to trick GNU and Intel compiler to use the LLVM/OpenMP runtime, when it is found before their own OpenMP runtime. By adding the path of libomp.so to the LIBRARY_PATH, gcc/icc will pick up the LLVM/OpenMP runtime during compilation.

As long as libgomp does not implement OMPT and LLVM has no Fortran frontend, I would welcome the possibility to easily link this runtime into Fortran applications compiled with the GNU compiler.

I also value this use-case, and I think that should continue to work with this patch: The intention is to put libraries that Clang cares about early in the library search hierarchy. As other Compiler (Intel Compiler, GCC) don't know about and don't look into "Clang-dedicated" directories, it should be sufficient to have the symlinks in lib/? (which is not going to change)

But, if my patch were to install those symlinks into a Clang-dedicated directory, wouldn't it improve that use case? That is, if people use LIBRARY_PATH (or -L) to help gcc/icc locate Clang's libgomp.so, could my patch permit them to avoid inadvertently elevating other system libraries that happen to be in lib/ because they can now point to a Clang-dedicated directory instead? Then again, it does not help them to avoid elevating other Clang libraries in that Clang-dedicated directory, so hopefully there's nothing else there that matters.

  1. Still, my proposal is that placement of Clang's libraries early in the search path should be the default. I've shown why that's good. Is it ever bad? Is there ever a case where the user expects system libraries to have precedence over Clang's own libraries by default? I would guess a user should specify -L for that, but maybe some user installs alternative libraries ahead in the current path and expects that to be sufficient.

My memories are also coming back here: Searching for abandoned revisions resulted in quite some interesting patches that I authored two years ago:

I agree that, by using a Clang-dedicated directory, my patch helps avoid the problem Chandler mentioned. However, my patch still changes the search path from what people are used to. That could affect, for example, the reverse of the use case we just discussed above for libgomp.so. That is, what happens when people want to make Clang not use its own libraries? The discussion below suggests that's a practical concern, at least for some libraries.

In the following two revisions I actually proposed something similar for libc++, maybe some points are relevant (can't remember all the details, but the conclusions seems to be that the libc++ maintainers didn't like installing to a different directory):

So, lib/clang/9.0.0 is the Clang "resource directory". A point in D30733 is that libcxx and libunwind libraries should not be "locked" to a particular Clang version and so shouldn't be installed in the Clang resource directory. However, people there were not opposed to installing those to a Clang-dedicated directory that isn't version-locked, and there's even a proposal for what that directory structure might look like.

Assuming installation to that non-version-locked Clang-dedicated directory, one comment there is, "Now if you've built things against those libs, and upgrade your clang version, you are not tied to the new libc++ that comes with it, as you would be with libc++ placed in the resource dir." At first, that sounded nonsensical to me: if every Clang upgrade clobbers the previous Clang's libc++ because the installation directory isn't version-locked, you're always tied to the new libc++. Or do they assume that the resource directory appears early in the library search path and their proposed non-version-locked Clang-dedicated directory does not? If that's right, then their proposal doesn't elevate libraries within the search patch. (And I don't think they solve the problem my patch is trying to solve, but I don't think that problem was their focus.)

Interestingly, from that same discussion, "The only things that belong in clang's resource directory are the builtins, sanitizer libs, and maybe the OpenMP libs (assuming they're tied to a particular compiler version, and don't have a stable API/ABI)." So they consider openmp to be different than libcxx and libunwind.

In summary, assuming I understood that discussion, Clang-version-independent libraries, such as libc++ and libunwind, should not be installed to the Clang resource directory unless the user requests it (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is a current way to request that), or else the expected library search path for such libraries would not be obeyed. However, Clang-version-locked libraries can be installed there always. For example, compiler-rt libraries are installed there now (and LLVM_ENABLE_PER_TARGET_RUNTIME_DIR changes the organization some). OpenMP libraries perhaps should be too, and that's what my patch is trying to do.

Does that make sense?

Please note that libomp.so.5 is an invention of Debian. None of upstream's CMake files will ever create a symlink with that name!
(We could think about adding such symlink, there was a complaint sometime ago that we don't have versioned shared libraries. But that wouldn't solve all problems, the linker would still use the "wrong" file.)

Let me see if I understand this. Sorry if I have it all mixed up. For some N, if Clang looks for libomp.so.N instead of libomp.so, and if we could be sure that all copies of libomp.so.N that might be seen are ABI-compatible, and if no such libomp.so.N is yet another symlink, then it shouldn't matter if the linker uses the wrong file, and all that matters is that LD_LIBRARY_PATH chooses the right one. Is that right? I feel like that ABI-compatibility assumption is shaky, but I don't have enough practical experience here.

From my understanding the general idea is to have libomp.so -> libomp.so.N. When linking an application the linker resolves the link chain and adds libomp.so.N as a dependency. The assumption is that these versioned libraries will stay ABI-compatible for all times.
The point is that the OpenMP runtime libraries have a much stronger guarantee: The goal is to never break the ABI, we're only adding new functions and utilize additional bits that were left unused when introducing a new interface.
I don't really know why the Debian maintainers decided to add such symlink on their own, but that's part of the problem.

One good effect is that, if linking and loading don't locate the same version of the library (that is, -L, etc. doesn't agree with LD_LIBRARY_PATH, etc.), execution will fail. That seems like a good thing.

Thus, .so version numbers seem especially helpful for libraries (such as libc++ and libunwind) for which a particular Clang might not select its own versions because they're installed to lib, which is late in the search patch.

If we're not going to use .so version numbers for openmp libraries, then maybe those libraries don't belong somewhere like lib. Maybe, like compiler-rt libraries, which also don't have .so version numbers, they only belong in Clang's resource directory so that Clang always prefers its own versions so that linking/loading discrepancies are less likely.

However, only using LD_LIBRARY_PATH during runtime might not be sufficient. Imagine there is a new version of the OpenMP standard which introduces a new feature. A new function is added to libomp and Clang is taught to generate code, calling the new function.

Shouldn't the .so version number be incremented for this?

If linking ends up using the old libomp installed in a system directory, ld cannot find the new symbols and will fail

Makes sense. Conversely, if linking did find the new libomp but loading found the old one, loading would fail because the version number would be wrong.

So I think ABI-compatibility is not equivalent to "the right library for linking" in all cases. Does that make sense?

I don't understand. I think the lack of ABI-compatibility in your example is precisely why the library was not right for linking. What did I miss?

Assuming installation to that non-version-locked Clang-dedicated directory, one comment there is, "Now if you've built things against those libs, and upgrade your clang version, you are not tied to the new libc++ that comes with it, as you would be with libc++ placed in the resource dir." At first, that sounded nonsensical to me: if every Clang upgrade clobbers the previous Clang's libc++ because the installation directory isn't version-locked, you're always tied to the new libc++.

Ah, I just realized that doesn't make sense because of the .so version numbers for libc++. So, the idea there is that, if Clang looks in a directory specific to its own version, it won't find old .so versions of libraries. Right?

Or do they assume that the resource directory appears early in the library search path and their proposed non-version-locked Clang-dedicated directory does not? If that's right, then their proposal doesn't elevate libraries within the search patch.

So, I still don't know if it's ok to elevate libc++ earlier in the search path.

Eugene.Zelenko set the repository for this revision to rOMP OpenMP.Feb 7 2019, 10:09 AM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 10:09 AM