- User Since
- Aug 20 2014, 4:35 PM (135 w, 6 d)
Thanks for catching this. I didn't even notice that I had written such bad tests!
LLVM has a CMake module "LLVMProcessSources.cmake" which verifies that all source files are referenced in the CMakeLists file. Since it is part of the LLVM distributed modules, you can re-use it in libcxx.
The one downside to this approach is that when you use an absolute path as the DESTINATION argument to install you can't change the install root without reconfiguring. When you use relative paths you can.
Mon, Mar 27
Added a note to the unit test CMake file about why the tests are where they are. Generally we isolate debugserver from the rest of LLDB, and this comment explains the breach of isolation.
@jingham I put the unit tests at the top because they depend on LLDB's Host library (at least the current tests do). I'm attempting to write tests which cover the socket communication between LLDB and debugserver by sending data between the two using each side of the API.
Fleshed out the unit test logic to be more meaningful.
Fri, Mar 24
Xcode is pretty magic to me. I don't know how to do much of anything in it other than build. I think the right solution would be to take most of the source files out of the debugserver target and generate a static archive from them, then have debugserver and the debugserverTest target link the static archive.
Wed, Mar 22
Tue, Mar 21
One minor style comment, otherwise looks good to me.
Cool! Seems to me like this patch addresses everyones issues, and I think the patch looks good. Thanks for iterating on this!
Mon, Mar 20
@inglorion I believe the reason we had to set the policy to OLD is because the auto-passed variable masks the explicitly passed one in try_compile. If that is the case, we need the same change in the Darwin code path too.
This patch looks fine to me, however I'd like to see the resolution on D31098 before landing this. If we change the CMake policy CMP0056 to NEW, I think that will automatically address this issue resulting in us not needing this patch.
@davidxl, do you recall how the failure that caused you to make the policy change exhibited itself? It seems to me like we were working around the behavior of the old policy setting by passing the linker options through manually. I wonder if the reason for the error was CMake masking those liker flags under the new policy.
This looks like a great cleanup. Thanks!
Fri, Mar 17
@mgorny, we should only support this on shared libraries that we intend as vended libraries, not all libraries. For example we don't really want to support the CMake BUILD_SHARED_LIBS option's libraries as vended libraries on any platform.
Tue, Mar 14
This is great, thanks!
Ugh... I really don't like how LLVM_UTILS_PROVIDED is implemented, but I guess it is fine. This patch is good.
Mon, Mar 13
Forgot a semi-colon...
Updates based on feedback from Jason and Zachary.
Removing some extra changes that accidentally came along for the ride in my initial upload.
Tue, Mar 7
This patch is fine, just be warned that eventually I really want to kill the projects subdirectory entirely.
Mon, Mar 6
Fri, Mar 3
Updating to address Paul's feedback about DWARF32/DWARF64.
Updated based on feedback from Adrian.
Thu, Mar 2
Updates based on feedback from Adrian.
Wed, Mar 1
- Reducing the patch to just the NFC changes
Feb 24 2017
This is for LLVM. Will land shortly.
This all looks pretty great to me.
Feb 22 2017
Feb 17 2017
Don't really have much to add here, but I really like what this is doing, and I think it will be a huge win for LLDB. Thanks @zturner!
Mostly looks good, a few inline comments below.
LGTM. This is really cool, thanks for putting it together.
Feb 15 2017
Thanks for all your work on these!
Feb 14 2017
Feb 8 2017
This looks reasonable to me.
A few small comments below. Otherwise LGTM.
A few comments inline. Since these variables are used by the LLVM CMake modules, they also need to be added to LLVMConfig.cmake.in. Otherwise they will break out-of-tree builds.
Feb 7 2017
Ok. I think this patch is fully ready to go now. I added the missing dependencies in the ObjectFileELF tests in rL294372.
Adding Info and Utils handling.
Feb 6 2017
Feb 1 2017
Unit tests are updated in rL293821. This update makes one last change to the unit tests to build with the explicit dependencies.
Ah! I forgot to update the unittest directory. I'll get a patch in today that updates the unittests following the pattern from the libraries.
Jan 31 2017
Fixing the issue labath reported in the review. Generally speaking we shouldn't be configuring or compiling plugins that can't be used.
@labath, we'll have to see if CMake's handling is good enough. What CMake actually does is repeat the full chain of the cycle, so it would be something like adding -lX -lY -lX -lY in your example. This doesn't work for certain pathological cases, but CMake actually has mechanisms to help deal with that too if we need them:
I expect this patch and the next one to be pretty safe because I'm not actually removing the existing dependency specifications, just adding new explicit ones.
Thankfully CMake will not complain about circular dependencies in static archive targets. If it did, we'd really be in trouble because the LLDB dependencies graph has *lots* of circular dependencies. Actually, I think CMake even handles them properly on Linux, which should eliminate our need to add --start-group and --end-group to the linker command lines.
@zturner That mechanism does work, however I generally find that it is cleaner to pass named parameters into CMake functions which provides a bit of a self-documenting API, as opposed to relying on known named variables. The named-parameter was introduced for LLD, and I think is the more-modern way of handling this. Eventually I think LLVM should be updated to do the same.
Updates based on post commit feedback from labath.
Jan 26 2017
Updates based on review by bryant
Jan 25 2017
Jan 24 2017
Updating patch with a few new things.
Jan 23 2017
Jan 20 2017
Jan 18 2017
Updates based on feedback from dblaikie.
Jan 17 2017
@joerg I completely disagree with you. Since libgcc provides those files for some platforms compiler-rt should as well.
Ah... don't know if we have one with shared libs enabled, but adding a test is still good. You probably need a new available feature added to lit for checking the dylib. In case you're unsure how to do that I wrote it up real quick and put it in a paste (https://reviews.llvm.org/P7962).
Looping in Brad King and Stephen Kelly for CMake advice.
I'm pretty confident we have buildbots testing mingw. You should be able to add a test case under tests/tools/llvm-config and have REQUIRES: mingw32 in it.
@bryant and I spoke over IRC in a PM, for posterity I'll recap here.
Many thanks for the quick cleanup! LGTM!