Page MenuHomePhabricator

[cmake] Respect LLVM_RUNTIMES_LIBDIR_SUFFIX
AbandonedPublic

Authored by mgorny on Aug 21 2016, 12:42 AM.

Details

Summary

Respect the newly-introduced LLVM_RUNTIMES_LIBDIR_SUFFIX variable that
can be used to override the libdir used to hold LLVM runtimes. Install
clang runtimes (headers & compiler-rt) there, and use it when looking
them up.

Diff Detail

Event Timeline

mgorny updated this revision to Diff 68800.Aug 21 2016, 12:42 AM
mgorny retitled this revision from to cmake: Supporting overriding runtime libdir via CLANG_LIBDIR_SUFFIX.
mgorny updated this object.
mgorny added reviewers: chandlerc, chapuni, samsonov.
mgorny added a subscriber: cfe-commits.
beanz added a reviewer: rnk.Sep 23 2016, 1:56 PM

This looks reasonable to me, but I'm not super familiar with multi-lib conventions. I think @chandlerc is more familiar with how that stuff works.

A gentle ping.

beanz added a comment.Oct 17 2016, 9:47 PM

Looking more closely at this, there is a problem that I see.

The clang runtime directory only supports building compiler-rt, and is going to be replaced by the llvm runtimes directory in the (hopefully near) future.

Maybe a better way to go is to define RUNTIMES_LIBDIR_SUFFIX in LLVM and connect that into both the clang runtime and llvm runtimes directories.

Do you think I should just focus on the suffix, or maybe try to make the whole path configurable?

I think doing just the suffix is the right starting point.

mgorny updated this revision to Diff 75996.Oct 27 2016, 2:20 AM
mgorny retitled this revision from cmake: Supporting overriding runtime libdir via CLANG_LIBDIR_SUFFIX to [cmake] Respect LLVM_RUNTIMES_LIBDIR_SUFFIX.
mgorny updated this object.
mgorny edited edge metadata.

Here's the initial patch for the suggested approach. I'm still testing it though.

mgorny added inline comments.
lib/Driver/Tools.cpp
2021

Note: I don't know if it's better to use LLVM_RUNTIMES_LIBDIR_SUFFIX here, or just #define LLVM_LIBDIR_SUFFIX in addition to that.

beanz added a comment.Oct 27 2016, 1:41 PM

This looks like it is in the direction I would expect. Please let me know once the final patch is ready.

lib/Driver/Tools.cpp
2021

This should be LLVM_LIBDIR_SUFFIX as per Chandler's comments.

chandlerc edited edge metadata.Oct 27 2016, 2:25 PM

I don't get it.

There is nothing "RUNTIMES" about it. And this variable is actually used to produce real libraries: libclang, etc.

It's just the LIBDIR_SUFFIX. That's it. The reason it is called CLANG_LIBDIR_SUFFIX and set from LLVM_LIBDIR_SUFFIX is to support using the CMake build of Clang in a standalone mode where there *isn't* a LLVM_LIBDIR_SUFFIX and instead the CLANG_LIBDIR_SUFFIX needs to be set directly.

So I'm really not understanding what benefit this change is bringing.

And the other comment in the CMake file that the CLANG_LIBDIR_SUFFIX must exactly match whatever LLVM_LIBDIR_SUFFIX is remains, AFAICT, 100% correct.

I don't get it.

There is nothing "RUNTIMES" about it. And this variable is actually used to produce real libraries: libclang, etc.

It's just the LIBDIR_SUFFIX. That's it. The reason it is called CLANG_LIBDIR_SUFFIX and set from LLVM_LIBDIR_SUFFIX is to support using the CMake build of Clang in a standalone mode where there *isn't* a LLVM_LIBDIR_SUFFIX and instead the CLANG_LIBDIR_SUFFIX needs to be set directly.

So I'm really not understanding what benefit this change is bringing.

And the other comment in the CMake file that the CLANG_LIBDIR_SUFFIX must exactly match whatever LLVM_LIBDIR_SUFFIX is remains, AFAICT, 100% correct.

That is 100% incorrect. LLVM_LIBDIR_SUFFIX is used almost everywhere, mostly because it goes implicitly through LLVM's CMake macros. CLANG_LIBDIR_SUFFIX is truly only used to set the paths in the sources.

I don't get it.

There is nothing "RUNTIMES" about it. And this variable is actually used to produce real libraries: libclang, etc.

It's just the LIBDIR_SUFFIX. That's it. The reason it is called CLANG_LIBDIR_SUFFIX and set from LLVM_LIBDIR_SUFFIX is to support using the CMake build of Clang in a standalone mode where there *isn't* a LLVM_LIBDIR_SUFFIX and instead the CLANG_LIBDIR_SUFFIX needs to be set directly.

So I'm really not understanding what benefit this change is bringing.

And the other comment in the CMake file that the CLANG_LIBDIR_SUFFIX must exactly match whatever LLVM_LIBDIR_SUFFIX is remains, AFAICT, 100% correct.

That is 100% incorrect. LLVM_LIBDIR_SUFFIX is used almost everywhere, mostly because it goes implicitly through LLVM's CMake macros. CLANG_LIBDIR_SUFFIX is truly only used to set the paths in the sources.

OK, it used to be used in more exciting places.

If you want to go that route, the fix is just to s/CLANG_LIBDIR_SUFFIX/LLVM_LIBDIR_SUFFIX/.

This really doesn't solve the problems we have. We don't want to build and install two almost-identical copies of compiler-rt and clang headers, when we can just build one and make clang use it.

This really doesn't solve the problems we have. We don't want to build and install two almost-identical copies of compiler-rt and clang headers, when we can just build one and make clang use it.

However, as I said before, I don't really think that this approach makes sense. I don't think we should be building Clang to re-use parts of some other Clang installation. I understand that is exactly what you want to do, but I disagree that it is a good idea. I think it is brittle and failure prone. I suggested ways you could make it work for your use cases however, but I don't think changing the CMake in this way is valid.

Hahnfeld resigned from this revision.Oct 27 2016, 11:49 PM
Hahnfeld removed a reviewer: Hahnfeld.
mgorny abandoned this revision.Oct 27 2016, 11:59 PM

Very well. I'll look into (ab)using the existing options to achieve the same result. Having stand-alone build systems, this should become easier these days, at least until some major change in runtime handling occurs.