@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

@aprantl Found a workaround for the lldb-cmake bot: https://reviews.llvm.org/rL350346

Hello lsaba, sorry for having to revert this after it broke http://green.lab.llvm.org/green/job/lldb-cmake/ again.
I tried to find the root cause, but so far didn't succeed. I will continue tomorrow and keep you in the loop so you can land this as soon as possible.

The indentations of section_type and other lines look like you accidentally committed tabs instead of spaces. Can we fix that? Thanks

Thanks everyone for your patience with the review. I will spare us the potential merge conflicts before Christmas and land this after the holidays. Cheers

Fair enough. Will create a ticket for it. At least this works for now.

Ninja builds work well without it. In order to catch actual double-signing problems here, I would prefer to pass it only in case of Xcode generator.

Add --force flag when using the Xcode generator to avoid double-signing errors

Turn LLDB_CODESIGN_IDENTITY_USED variable from STRING into INTERNAL