Page MenuHomePhabricator

build: remove use of llvm-config
Needs RevisionPublic

Authored by compnerd on Jan 29 2019, 11:26 AM.

Details

Summary

Update the build to use the standard CMake package search mechanism (-DLLVM_DIR=). This reduces the use of llvm-config which will eventually allow us to remove the tool. It also enhances the support to cross-compile lld in standalone mode.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

compnerd created this revision.Jan 29 2019, 11:26 AM
ruiu added a comment.Jan 29 2019, 11:29 AM

I'm not CMake expert so I'm probably not the right person to review this, but if you are making the same change to other tools under the LLVM umbrella, I'm fine with this change.

I made a similar change to clang: https://reviews.llvm.org/rL346732, and I think this is the right direction to go.

ruiu added a comment.Jan 29 2019, 1:47 PM

Tom,

Feel free to approve this patch once your patch to clang has approved.

tstellar accepted this revision.Jan 29 2019, 1:59 PM

The clang patch was merged a few months ago. LGTM.

This revision is now accepted and ready to land.Jan 29 2019, 1:59 PM
mgorny requested changes to this revision.Jan 29 2019, 2:15 PM

This change breaks building against installed LLVM with externally provided unconfigured sources. LLVM_MAIN_SRC_DIR was made a cache variable originally for this purpose.

This revision now requires changes to proceed.Jan 29 2019, 2:15 PM

This doesn't break the build, we check that the path exists, and find_program will not error out. You can still specify LLVM_LIT_EXE to specify the lit.py to use. Is that not sufficient for your needs?

This doesn't break the build, we check that the path exists, and find_program will not error out. You can still specify LLVM_LIT_EXE to specify the lit.py to use. Is that not sufficient for your needs?

The unittests won't be enabled unless ${LLVM_BUILD_MAIN_SRC_DIR} exists and points to an LLVM source tree, which is one reason to have it as a cache variable.

I wrote a patch similar to this in LLDB, so I'll chime in a bit.

@mgorny: This doesn't actually *break* the build per se. This change just means you need to do a bit more work to be able to test the lld you just built. Some things that could be done to improve the situation:

  • Users who want to test lld should specify LLVM_BUILD_MAIN_SRC_DIR (and possibly LLVM_LIT_EXE) manually.
  • Introduce some new variable like PATH_TO_LLVM_SRC and replace LLVM_BUILD_MAIN_SRC_DIR. This could be optional variable which would need to be set if you want to test.

I don't see this as a hard blocker for putting this patch in. It simplifies the build logic because we don't have to call out to some other process and parse its output, and it de-duplicates work because the LLVM CMake package provides a lot of the variables that we were filling out.

tstellar added a comment.EditedJan 30 2019, 2:01 PM

I wrote a patch similar to this in LLDB, so I'll chime in a bit.

@mgorny: This doesn't actually *break* the build per se. This change just means you need to do a bit more work to be able to test the lld you just built. Some things that could be done to improve the situation:

  • Users who want to test lld should specify LLVM_BUILD_MAIN_SRC_DIR (and possibly LLVM_LIT_EXE) manually.

This isn't actually different from what you had to do before this patch. The only difference is that LLVM_MAIN_SRC_DIR was a CACHE variable, but now LLVM_BUILD_MAIN_SRC_DIR is not.
The request was to have a CACHE variable.

  • Introduce some new variable like PATH_TO_LLVM_SRC and replace LLVM_BUILD_MAIN_SRC_DIR. This could be optional variable which would need to be set if you want to test.

I don't see this as a hard blocker for putting this patch in. It simplifies the build logic because we don't have to call out to some other process and parse its output, and it de-duplicates work because the LLVM CMake package provides a lot of the variables that we were filling out.

I wrote a patch similar to this in LLDB, so I'll chime in a bit.

@mgorny: This doesn't actually *break* the build per se. This change just means you need to do a bit more work to be able to test the lld you just built. Some things that could be done to improve the situation:

  • Users who want to test lld should specify LLVM_BUILD_MAIN_SRC_DIR (and possibly LLVM_LIT_EXE) manually.

This isn't actually different from what you had to do before this patch. The only difference is that LLVM_MAIN_SRC_DIR was a CACHE variable, but now LLVM_BUILD_MAIN_SRC_DIR is not.
The request was to have a CACHE variable

  • Introduce some new variable like PATH_TO_LLVM_SRC and replace LLVM_BUILD_MAIN_SRC_DIR. This could be optional variable which would need to be set if you want to test.

I don't see this as a hard blocker for putting this patch in. It simplifies the build logic because we don't have to call out to some other process and parse its output, and it de-duplicates work because the LLVM CMake package provides a lot of the variables that we were filling out.

The issue here is that the old variable LLVM_MAIN_SRC_DIR was a CACHE variable and LLVM_

Ah, I misunderstood what was being said. I should have read more carefully. :)
Adding back LLVM_MAIN_SRC_DIR as a cache var and having it default to LLVM_BUILD_MAIN_SRC_DIR should be easy enough.

compnerd updated this revision to Diff 184368.Jan 30 2019, 2:21 PM

Restore LLVM_MAIN_SRC_DIR cache variable

xiaobai requested changes to this revision.Jan 31 2019, 12:30 AM

After taking the time to go through this more thoroughly I realized there were several issues that need to be addressed before this can go in.

I also found that testing is broken with LLD standalone builds. Specifically, I ran ninja check-lld and that doesn't work because it tries to use llvm-lit from the lld build tree. This patch is not at fault, as it fails the same way with or without this patch. I think this could be addressed in a later patch.

CMakeLists.txt
12–13

This should be changed to LLVM_CMAKE_DIR or LLVM_DIR (Both of these directories will have the LLVM CMake modules). As-is, LLVM_CMAKE_PATH is not set.

13

You also will need to set LLVM_MAIN_INCLUDE_DIR here or else TableGen won't include that path by default. Something like this should work:
set(LLVM_MAIN_INCLUDE_DIR ${LLVM_BUILD_MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")

35

find_program -> set

40

find_program -> set

This revision now requires changes to proceed.Jan 31 2019, 12:30 AM
compnerd updated this revision to Diff 184545.Jan 31 2019, 10:33 AM
compnerd marked 4 inline comments as done.

Fixes

mgorny accepted this revision.Feb 1 2019, 12:00 AM

Seems not to break anything for me ;-).

Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 12:00 AM
mgorny requested changes to this revision.Feb 7 2019, 12:48 PM

I'm sorry, I must've tested wrong before. It actually breaks the build for me:

[29/143] cd /var/tmp/portage/sys-devel/lld-9999/work/lld-9999_build && /usr/lib/llvm/9/bin/llvm-tblgen -gen-opt-parser-defs -I /var/tmp/portage/sys
-devel/lld-9999/work/lld-9999/lib/Driver /var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td -o lib/Driver/DarwinLdOpti
ons.inc -d lib/Driver/DarwinLdOptions.inc.d
FAILED: lib/Driver/DarwinLdOptions.inc
cd /var/tmp/portage/sys-devel/lld-9999/work/lld-9999_build && /usr/lib/llvm/9/bin/llvm-tblgen -gen-opt-parser-defs -I /var/tmp/portage/sys-devel/ll
d-9999/work/lld-9999/lib/Driver /var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td -o lib/Driver/DarwinLdOptions.inc -
d lib/Driver/DarwinLdOptions.inc.d
/var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td:1:9: error: Could not find include file 'llvm/Option/OptParser.td'
include "llvm/Option/OptParser.td"
        ^
/var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td:1:9: error: Unexpected input at top level
include "llvm/Option/OptParser.td"
        ^
ninja: build stopped: subcommand failed.
This revision now requires changes to proceed.Feb 7 2019, 12:48 PM

Which version of the patch did you use? L13 in the current version sets LLVM_MAIN_INCLUDE_DIR specifically for that case.

Yep, that one. However, something else might have changed in the background since LLDB doesn't want to build anymore either. I'll try a bit of pseudo-bisect just in case.

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

Ok, I've researched this in detail and LLDB is broken by the same change. The concept used for tablegen here is apparently wrong, and causes it not to find installed .td files.

mgorny added inline comments.Feb 9 2019, 4:30 AM
CMakeLists.txt
13

So I've looked into the failures more closely, and it looks that ${LLVM_BUILD_MAIN_INCLUDE_DIR} is wrong here. It should be ${LLVM_INCLUDE_DIR}, i.e. the directory with installed headers. This is also what the old code was doing.