This was mostly to illustrate usage of the matching LLVM commit. Unlikely to break something. Post-commit review should be sufficient.

Yes, scopes are per directory; macros operate on the caller's scope and functions add one.

In D60862#1471755, @friss wrote:

Actually, thinking more about this, how do you use it? LLVM_EXTERNALIZE_DEBUGINFO_EXTENSION is a global but this would be different per target.

Enables https://reviews.llvm.org/D60863

Ok, so far this appears to be a rare issue that we don't know much about. I think keeping LLVM and its subprojects in sync (which means CMake version 3.4.3) should be the prio for now.

Can you please explain in more detail how this is supposed to fix the issue?

+1 Following the local coding style was a pain when adding functions in these files. Good step forward.
This covers the majority of occurrences. Leaving unittests unchanged is probably intentional. There's a few more left in tools and other corners for a follow-up.

you should be using the same sysroot to build LLVM and LLDB. In my specific problem, when you build any of the lldb tools (e.g. lldb-server) it fails to link because you are building the tool against the freshly built libc++ but the llvm libraries you had previously built were linked against the NDK's libc++.

No, everything is being built for android. Cross-compiling lldb-server involves cross-compiling llvm libraries, clang libraries, and if you've got swift in the picture, swift host libraries. LLVM and clang libraries are built against the libc++ from the android NDK, but in standalone builds, LLDB will try to link against the freshly built libc++ from LLVM. You get loads of undefined references to symbols from the NDK's libc++.

[LLVM_LIBRARY_DIR is] not a cache variable when it's an in-tree build

From the looks of it, clang needs to set it manually because they rely on llvm-config instead of using find_package.

Major concern from my side is that clang does something like this too:

clang/CMakeLists.txt
86:  set(LLVM_LIBRARY_DIR ${LIBRARY_DIR} CACHE PATH "Path to llvm/lib")
125:  link_directories("${LLVM_LIBRARY_DIR}")

@serge-sans-paille After all, it appears to me that your issue is caused by something else, but if this change fixes it, I am not against taking it for now. It shouldn't do any harm. Please get in touch with @hans if this is really relevant for release_80.

Good initiative to fix potential nullptr assignments to std::string.

In D58193#1401137, @serge-sans-paille wrote:

@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora, because it wasn't working automagically otherwise. Reading the discussion, I don't think I missed some configuration stuff, or what did I miss when configuring the build of lldb in standalone mode?

Yap, will try and come up with a proposal soon.

In D58193#1398376, @labath wrote:

BTW, what's the reason for having super-high-fidelity list of dependencies in a standalone build?

I don't want to play the nitpicker here, but did you test this? If so, please provide a sample configuration command line.

@xiaobai && @compnerd Do you know if find_package can tell us anything about it?

All these targets are exported from the LLVM build-tree since D56606 and from the install-tree since D57383.

Ping

In D57989#1395232, @xiaobai wrote:

we are also doing a CMake version check in LLDBConfig for the same purpose. Seems like you could consolidate those?

Submit proposal from Diff 186517

Looking at the code more closely, it turns out this is not the only issue:

• apparently generator expressions are not supported in the BUILD_RPATH target property
• BUILD_RPATH is only supported in 3.8+ https://cliutils.gitlab.io/modern-cmake/chapters/intro/newcmake.html
• LLDB_FRAMEWORK_INSTALL_DIR should not overwrite, but rather add an install RPATH (and it should be the first)

Tested it for the lldb driver and you are right, that the genex ends up in the binary -- interesting. I then also tested with your change, but it gave me the same:

Load command 17
          cmd LC_RPATH
      cmdsize 40
         path $<TARGET_FILE:liblldb> (offset 12)

A chunk of code referred to a generator expression wrapped in quotations and then referred to later again in quotations which caused cmake to think it was something to be referred to as a string.

Actually, both use cases already work without this change: passing LLVM_DIR && Clang_DIR or passing LLDB_PATH_TO_LLVM_BUILD. IMHO this patch needs good reason to land.

Can't speak for all the subproject, but with D57995 this should work for LLDB.

In D57995#1394412, @mgorny wrote:

Indeed it is a problem. However, it seems to be a bug that LLVM_INCLUDE_DIR contains two directories.

Indeed, these variables do not exist when running against a LLVM/Clang install-tree and this should be fixed. Did you give this a try running against a LLVM/Clang build-tree? Dumping the values here locally, gives me:

LLVM_BUILD_MAIN_INCLUDE_DIR /path/to/llvm/include
LLVM_BUILD_LIBRARY_DIR      /path/to/llvm-build/./lib
LLVM_BUILD_BINARY_DIR       /path/to/llvm-build

Polish && error out also on empty string

Remove useless newline

it is pretty reasonable to ask that the user tell us where LLVM and Clang are built

In D57402#1385718, @xiaobai wrote:

• For us it's a good match for our downstream code https://github.com/apple/swift-lldb/blob/upstream-with-swift/cmake/modules/LLDBStandalone.cmake

I don't think it's too difficult to change how it works downstream in lldb-swift. I actually already have an open PR trying to make swift-lldb closer to upstream in this particular file: https://github.com/apple/swift-lldb/pull/1252
This change would be a cherry-pick on top of that, I think.

Hey sorry for the late reply, didn't see this earlier. Personally, I think the move away from the llvm-config approach is good, but I have no strong opinion about the solution.

Not sure if we want to change the behavior this way. The purpose of this code was to make the dependency to libc++ explicit because it's specific to macOS and it's missed quite often. In my experience warnings like the proposed one are hard to recognize in the load of CMake output.

In D57521#1379837, @beanz wrote:

@phosek, does LLVM’s runtime directory do this correctly?

At some point we should try and deprecate and remove this option in favor of the LLVM directory because the LLVM approach is much more complete and robust.

In case compiler-rt abandons llvm-config in favor of find_package(LLVM) one day, we could drop the COMPILER_RT_HAS_LLVMTESTINGSUPPORT logic here and use imported target properties instead. It seems a cleaner solution to me and avoids issues like D57521.

FYI: Seems to be happening since D55891

Polishing

Tested this with LLDB library target.

LGTM

Thanks -- and one more :) https://reviews.llvm.org/rL352058

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

Yes this sounds very reasonable.

In D56531#inline-500764, @labath wrote:

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

In D56531#1352298, @mgorny wrote:

Please don't risk merging this before the branching.

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

Closing this in favor of a more comprehensive fix.

Simple cleanup parts of the previous reviews D56400 and D56440

I will split off the "dead code elimination" part and close both reviews in favor of a more comprehensive fix.

In D56440#1351033, @sgraenitz wrote:

In D56440#1349892, @JDevlieghere wrote:

It looks like LLDB_TEST_COMPILER_IS_DEFAULT is set but never read. Why do we need it exactly?

[...] I will check why dotest doesn't need them and either fix it (which adds a use case) or remove it.

Move BUILD_SHARED_LIBS and LLDB_HAVE_LLD to a separate commit

Fix typo

In D56440#1349892, @JDevlieghere wrote:

It looks like LLDB_TEST_COMPILER_IS_DEFAULT is set but never read. Why do we need it exactly?

I think this is going to be really nice! Here's a few details I found.

Use LLVM_BINARY_DIR when checking for existance of libc++ include directory

Update documentation: libc++ should be checked out when building for macOS

Remove remove unused LLDB_HAVE_LLD and ENABLE_SHARED in lit/CMakeLists.txt

In test/CMakeLists.txt pass on LLDB_TEST_COMPILER_USED instead of LLDB_TEST_COMPILER

Act on FIXME's in subsequent commits and remove comments here