- User Since
- Jul 10 2018, 11:23 AM (32 w, 3 d)
Wed, Feb 20
@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.
Tue, Feb 19
Good initiative to fix potential nullptr assignments to std::string.
Mon, Feb 18
Yap, will try and come up with a proposal soon.
Fri, Feb 15
Thu, Feb 14
I don't want to play the nitpicker here, but did you test this? If so, please provide a sample configuration command line.
Wed, Feb 13
Submit proposal from Diff 186517
Tue, Feb 12
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.
Mon, Feb 11
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
Fri, Feb 8
Remove useless newline
Thu, Feb 7
it is pretty reasonable to ask that the user tell us where LLVM and Clang are built
Wed, Feb 6
Tue, Feb 5
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.
Fri, Feb 1
Thu, Jan 31
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
Tue, Jan 29
Tested this with LLDB library target.
Mon, Jan 28
Fri, Jan 25
Thu, Jan 24
Thanks -- and one more :) https://reviews.llvm.org/rL352058
Jan 22 2019
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>)
Jan 18 2019
Jan 16 2019
Yes this sounds very reasonable.
Jan 14 2019
Jan 13 2019
Jan 11 2019
Thanks for the initiative! Would be great to have this part cleaned up as well.
Closing this in favor of a more comprehensive fix.
I will split off the "dead code elimination" part and close both reviews in favor of a more comprehensive fix.
Jan 10 2019
Jan 9 2019
Move BUILD_SHARED_LIBS and LLDB_HAVE_LLD to a separate commit
I think this is going to be really nice! Here's a few details I found.
Jan 8 2019
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
Jan 7 2019
Jan 4 2019
Jan 3 2019
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
Dec 18 2018
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
Dec 13 2018
Fair enough. Will create a ticket for it. At least this works for now.
Dec 12 2018
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
Dec 10 2018
Turn LLDB_CODESIGN_IDENTITY_USED variable from STRING into INTERNAL