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

Sat Jan 26 18:38:57 PST 2019

jdenny added a comment.

In D55725#1372480 <https://reviews.llvm.org/D55725#1372480>, @Hahnfeld wrote:

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

> In D55725#1371344 <https://reviews.llvm.org/D55725#1371344>, @protze.joachim wrote:
> 
>> 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?
>>
>>
>> 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.

> In D55725#1371349 <https://reviews.llvm.org/D55725#1371349>, @jdenny wrote:
> 
>> 2. 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:
> 
> - https://reviews.llvm.org/D26244: Basically the idea was to prefer Clang's libraries in `lib/` over the system default. Here's what I think was the most important point raised by Chandler: https://reviews.llvm.org/D26244#641099. I don't think this applies to the current approach because no other project should be installing to `lib/clang/`...

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):
> 
> - https://reviews.llvm.org/D30214
> - https://reviews.llvm.org/D30733

So, `lib/clang/9.0.0` is the Clang "resource directory".  A point in D30733 <https://reviews.llvm.org/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?

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

https://reviews.llvm.org/D55725