Page MenuHomePhabricator

[CMake] Replace use of llvm-config with LLVM and Clang CMake packages
ClosedPublic

Authored by xiaobai on Jan 9 2019, 6:51 PM.

Details

Summary

I did this for two reasons:

  • Using the CMake packages simplifies building LLDB Standalone. This is for two reasons: 1) We were doing a decent amount of work that is already done in the LLVMConfig.cmake that we want to import, 2) We had to do some manual work to call llvm-config, parse its output, and populate variables that the build system uses.
  • As far as I understand, using llvm-config makes it difficult if not impossible to cross-compile LLDB standalone.

Event Timeline

xiaobai created this revision.Jan 9 2019, 6:51 PM
mgorny added a comment.Jan 9 2019, 7:37 PM

Please don't risk merging this before the branching.

Seems like I nice cleanup. I just have a question about the search paths.

(Also, I agree with @mgorny about not doing this so close to the branch point.)

cmake/modules/LLDBStandalone.cmake
9

Why do you put NO_DEFAULT_PATH here? IIUC, the user will now have to specify LLDB_PATH_TO_LLVM_BUILD in order to build this, whereas previously llvm-config would be found on the path if it happened to be there (e.g. because it was already installed).

Would it not make sense to keep this behavior?

compnerd requested changes to this revision.Jan 10 2019, 8:48 AM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
cmake/modules/LLDBStandalone.cmake
7

Please add the following here:

file(TO_CMAKE_PATH LLDB_PATH_TO_LLVM_BUILD "${LLDB_PATH_TO_LLVM_BUILD}")
file(TO_CMAKE_PATH LLDB_PATH_TO_CLANG_BUILD" "${LLDB_PATH_TO_CLANG_BUILD}")

This ensures that you can use platform specific paths when they do not match CMake's view (i.e. Windows)

This revision now requires changes to proceed.Jan 10 2019, 8:48 AM
xiaobai marked 2 inline comments as done.Jan 10 2019, 11:00 AM
xiaobai added inline comments.
cmake/modules/LLDBStandalone.cmake
7

Will do, thanks!

9

In situations where you have multiple LLVM builds where each might be a different version of LLVM, I think it is better to force the user to specify which LLVM build they want to use instead of having them implicitly rely on whichever llvm-config happens to be in their path.

That being said, I would be willing to remove NO_DEFAULT_PATH here. I can understand if people find this behavior valuable or if the scenario I described is not very common.

xiaobai updated this revision to Diff 181135.Jan 10 2019, 12:56 PM

compnerd's comment

labath added inline comments.Jan 11 2019, 12:19 AM
cmake/modules/LLDBStandalone.cmake
9

I don't actually use the standalone build, so I don't care about this too much. I just mentioned this because this is the default behavior when searching for packages (as well as the previous behavior when we searched for llvm-config). However, it is true that we are version-locked much more tightly with llvm than with any of the other packages we search for with find_package.

The other thing that bugs me about the LLDB_PATH_TO_(LLVM|CLANG)_BUILD variables is that they seem to imply that you should point them to the build tree of llvm/clang. However, it should be possible to build lldb against an already-installed llvm, right (in which case they will likely have the same value)? In either case, I think it would be nice to explicitly declare these as a cache variable, if only so we can provide a docstring for them.

mgorny added inline comments.Jan 11 2019, 12:23 AM
cmake/modules/LLDBStandalone.cmake
9

In situations when you have multiple versions of LLVM in PATH, you usually set PATH in the order you want it to be used. And you really don't like when projects try to second-guess you.

xiaobai marked an inline comment as done.Jan 11 2019, 11:23 AM
xiaobai added inline comments.
cmake/modules/LLDBStandalone.cmake
9

@labath: When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example, LLVM_MAIN_INCLUDE_DIR should be set to the include directory in the LLVM source tree. TableGen.cmake adds the path in this variable to the include path when it invokes llvm-tablegen. This is exposed in the LLVM CMake package as LLVM_BUILD_MAIN_INCLUDE_DIR but only when you're using an LLVM build tree. The reason you need this is tools/driver/Options.td includes "llvm/Option/OptParser.td", which does not get put into the include directory of your LLVM build/install.

Maybe I should rewrite part of this to make it clearer that you need a build tree and not an llvm install? Declaring the variables as cache variables is a good idea nonetheless.

@mgorny: It seems like you find the behavior valuable so I will remove NO_DEFAULT_PATH. CMake processes the HINTS before searching your PATH regardles, so if you set LLDB_PATH_TO_${PROJECT}_BUILD then it will use that instead of using whatever it finds in your PATH.

xiaobai updated this revision to Diff 181352.Jan 11 2019, 12:36 PM

Removed use of NO_DEFAULT_PATH
Made LLDB_PATH_TO_{LLVM,CLANG}_BUILD cache variables

Thanks for the initiative! Would be great to have this part cleaned up as well.

cmake/modules/LLDBStandalone.cmake
9

Is it that instead of -DLLVM_CONFIG=/path/to/llvm-build/bin/llvm-config we would now pass -DLLDB_PATH_TO_LLVM_BUILD=/path/to/llvm-build?

LLDB_PATH_TO_(LLVM|CLANG)_BUILD variables [...] they will likely have the same value?

In my understanding this was always required. Did you try pointing LLDB_PATH_TO_CLANG_BUILD to a standalone build of Clang? Is there a good use-case for it? How do we detect that this Clang builds against the same LLVM build-tree?

When using LLVM/Clang/etc. as a dependency in external projects I usually followed the advice in https://www.llvm.org/docs/CMake.html#embedding-llvm-in-your-project:

If LLVM is not installed or you wish to build directly against the LLVM build tree you can use LLVM_DIR as previously mentioned.

That would be quite simple and find_package(LLVM) accepts LLVM_DIR as an input. Did you check how the other subproject do that?

xiaobai marked an inline comment as done.Jan 11 2019, 1:32 PM
xiaobai added inline comments.
cmake/modules/LLDBStandalone.cmake
9

Is it that instead of -DLLVM_CONFIG=/path/to/llvm-build/bin/llvm-config we would now pass -DLLDB_PATH_TO_LLVM_BUILD=/path/to/llvm-build?

Correct.

In my understanding this was always required. Did you try pointing LLDB_PATH_TO_CLANG_BUILD to a standalone build of Clang? Is there a good use-case for it? How do we detect that this Clang builds against the same LLVM build-tree?

I did not try doing it with a standalone build of Clang. I'm not sure if there is a good use-case for it, but if somebody knows then I'd like to hear about it. I am aware that it is possible to do a standalone build of Clang but I usually do integrated builds.

As for detecting whether or not the Clang builds against the same LLVM tree, I'm not sure if that's possible currently with the way LLVM and Clang's CMake packages are set up.

That would be quite simple and find_package(LLVM) accepts LLVM_DIR as an input. Did you check how the other subproject do that?

The other subprojects that I have looked at (clang, lld) have the current behavior of using llvm-config, which I think repeats a lot of work. Ideally the other projects would use the LLVM CMake package as well imo.

As for assigning LLVM_DIR directly, that is something that you could do. Even if I submitted this patch today as-is, setting LLVM_DIR directly would ignore LLDB_PATH_TO_LLVM_BUILD entirely. I would prefer still having the HINTS mechanism because it can take a list of directories. This is especially useful on build machines where the LLVM build can be found in one of a few possible locations. I also believe that the variable LLDB_PATH_TO_LLVM_BUILD is a little easier to understand than LLVM_DIR for people trying to figure out how to build LLDB. I see these two solutions as the same in every other regard though.

Please don't risk merging this before the branching.

+1
The official branch goal is Wednesday this week.

cmake/modules/LLDBStandalone.cmake
9

Sounds good to me. It would be nice if LLDB_PATH_TO_LLVM_BUILD is sufficient for the regular case of having LLVM & Clang in one build tree.

it should be possible to build lldb against an already-installed llvm

When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example [...]

IIUC all LLVM subprojects are made for *testing* against the build-tree. LLDB tests depend on LLVMTestingSupport and others that are not part of the installed package (see LLVM_EXPORTS_BUILDTREE_ONLY).
While the ability to *build* against an installed LLVM sounds like a good goal in principle, it's questionable what benefit it has without the ability to *test* the result. Thus, I wouldn't consider it a high prio for now.

it should be possible to build lldb against an already-installed llvm

When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example [...]

IIUC all LLVM subprojects are made for *testing* against the build-tree. LLDB tests depend on LLVMTestingSupport and others that are not part of the installed package (see LLVM_EXPORTS_BUILDTREE_ONLY).
While the ability to *build* against an installed LLVM sounds like a good goal in principle, it's questionable what benefit it has without the ability to *test* the result. Thus, I wouldn't consider it a high prio for now.

The fact that in is not possible to test lldb when using an installed llvm is unfortunate, but I think it would be nice to preserve the ability to build it at least, as this is what (some? i am not sure about other distros, but this is at least true for gentoo) linux distros use to provide modular llvm packages. If you still wanted to provide modular packages from a monolithic build you'd have to go through the install tree manually and sort the artefacts into respective packages, defeating the purpose of the install tree.

(This is also the use case for having separated llvm and clang and then building lldb against those.)

Thanks all for the feedback and discussion!

I won't commit this until after the branch cut. I'm thinking it should be fine to do early next week, but I'll wait if needed. Does anyone have any objects to this patch in its current form? I will rebase it later this week to handle any conflicts.

compnerd accepted this revision.Jan 15 2019, 2:42 PM
This revision is now accepted and ready to land.Jan 15 2019, 2:42 PM
sgraenitz accepted this revision.Jan 18 2019, 8:49 AM
This revision was automatically updated to reflect the committed changes.

FYI Fixed two small details with https://reviews.llvm.org/rL351879, hope that's ok:

  • LLDB_PATH_TO_CLANG_BUILD defaults to LLDB_PATH_TO_LLVM_BUILD
  • switch args for file(TO_CMAKE_PATH "<path>" <variable>)

@sgraenitz: I don't mind at all, I view those changes as improvements. Thanks for that!

mgorny added a comment.Feb 9 2019, 3:57 AM

This definitely broke stand-alone builds:

FAILED: tools/driver/Options.inc 
cd /var/tmp/portage/dev-util/lldb-9999/work/lldb-9999_build && /usr/lib/llvm/9/bin/llvm-tblgen -gen-opt-parser-defs -I /var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver /var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver/Options.td -o tools/driver/Options.inc -d tools/driver/Options.inc.d
/var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver/Options.td:1:9: error: Could not find include file 'llvm/Option/OptParser.td'
include "llvm/Option/OptParser.td"
        ^
/var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver/Options.td:1:9: error: Unexpected input at top level
include "llvm/Option/OptParser.td"
        ^
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2019, 3:57 AM
mgorny added inline comments.Feb 9 2019, 4:28 AM
lldb/trunk/cmake/modules/LLDBStandalone.cmake
20 ↗(On Diff #182948)

Apparently the idea of using LLVM_BUILD_MAIN_INCLUDE_DIR is plain wrong here. The old code used the equivalent of LLVM_INCLUDE_DIR (i.e. the include dir of *installed* LLVM) and I think the new code should use the same. Given how long the old code was there, I suppose there shouldn't be any use case broken by using that directory.

mgorny added a comment.Feb 9 2019, 4:53 AM

Submitted a fix as D57995.

sgraenitz added inline comments.Feb 11 2019, 12:25 PM
lldb/trunk/cmake/modules/LLDBStandalone.cmake
20 ↗(On Diff #182948)

Please note that it works well when running against a LLVM/Clang build-tree, which is a very common (but indeed not the only) use case this code should support.