[Openmp-commits] [PATCH] D55725: [OpenMP] Add libs to clang-dedicated directories

Fri Jan 25 00:17:36 PST 2019

Hahnfeld added a comment.

In D55725#1369528 <https://reviews.llvm.org/D55725#1369528>, @jdenny wrote:

> In D55725#1369006 <https://reviews.llvm.org/D55725#1369006>, @Hahnfeld wrote:
>
> > 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.)

In D55725#1370148 <https://reviews.llvm.org/D55725#1370148>, @jdenny wrote:

> - `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.

In D55725#1370622 <https://reviews.llvm.org/D55725#1370622>, @hfinkel wrote:

> > 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?

Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D55725