- User Since
- Aug 20 2014, 4:35 PM (160 w, 5 d)
Fri, Sep 8
I'm not sure this change is sufficient to do what you're intending. I think you also need to pass the library path of the cxx library into the bootstrap configuration otherwise it will use the one from the sysroot.
Thu, Aug 31
As a note: at some point in the future I'd like us to get rid of the practice of using ":" to create lists of lists, instead using list of list names, which is a bit cleaner.
Tue, Aug 29
Mon, Aug 28
Since the dependent patch landed, this LGTM too.
Tue, Aug 22
Aug 10 2017
This also looks good to me.
This looks pretty cool to me.
Aug 9 2017
Aug 4 2017
@mgorny I want to apologize here but I need to ask you not to commit this until @chapuni and I can come to an agreement between this solution and D36211. He ping'd me on IRC yesterday after I left for the day, I'll try and get in touch with him today to see if we can agree on the path forward.
Aug 3 2017
@chapuni, I disagree. LLDBExpression doesn't actually use RuntimeDyld directly, rather ExecutionEngine's headers do. I don't want downstream users of ExecutionEngine to know about the things it neccesitates pulling in.
@chapuni, the reason that I wanted to have this done with PUBLIC interface specifications is because lldbExpression does't actually use anything from the RuntimeDYLD library. The problem is LLVMExecutionEngine's headers are not clean, and they reference RuntimeDYLD directly and result in needing to bring it in.
Looks reasonable to me.
This all looks fine to me. The one thing I'd like you to consider is that someday (when lldb-server is supported on Darwin) we will want to run these tests against both debugserver and lldb-server. I'm not asking you to make those changes to this patch, but if there are things you would change about how you're doing things to make that easier to support I'd ask that you consider that.
Jul 26 2017
Jul 18 2017
I don't think this should be added as a new tool. We support generating LLVM-C.dylib on Darwin via the tools/llvm-shlib tool. It would be better if this were added there instead of completely separate.
So... Looking at the CMake in compiler-rt, COMPILER_RT_DEFAULT_TARGET_ONLY is only interpreted in a NOT APPLE block, so this would really not behave the way you intend. I think it is better to not support this on Darwin for the time being and provide a hard error rather than behaving incorrectly.
I'm suspicious that this would work as intended. What builtin libraries get built if your triple is Darwin?
Jul 10 2017
LGTM! Thanks for the cleanup!
LGTM! Thanks for updating the docs. I am really happy to be able to purge that broken behavior from my mind!
Jul 9 2017
I think it is better to XFAIL Windows and have test coverage on non-windows systems than to have the tests not running anywhere.
Jul 7 2017
Jun 21 2017
This looks great! Thanks for doing this.
Jun 19 2017
CMake stuff all looks good to me!
I get your perspective, but holding up this relatively small patch that fixes a bug in existing code on an architectural disagreement seems like excessive bike shedding. If we assume that JSON is required for the use case would you have Kuba write a full JSON parser in LLVM to satisfy your distaste over the fact that we have multiple JSON parsers already? That seems like an unreasonable request just to fix a few small bugs in existing code.
One question inline below.
Jun 16 2017
Jun 12 2017
May 31 2017
That looks like that should work to me. Seems good. @tstellar, does this resolve your issue?
Ah. I see what you're saying. I was thinking of the mismatch in a different way, but your solution makes sense. LGTM.
Ah. Makes sense. Can we maybe change the name from "StaticExports" to "ComponentExports"? As @chapuni pointed out this really applies to the component libraries, which can be static or shared based on configuration (although I keep hoping nobody would ever use BUILD_SHARED_LIBS in a distribution). Otherwise, I think this patch is good.
Unfortunately that doesn't really explain the need for these patches. From searching GitHub it doesn't seem like your project is overriding CMAKE_CFG_INTDIR. What is your CMake command line?
Ah! I see now. Probably the only thing that needs to be changed is handling LLVM_TOOLS_INSTALL_DIR being absolute. Otherwise the patch seems good to me.
How are you configuring? Unless I'm misunderstanding something when CMAKE_CONFIGURATION_TYPES is set CMAKE_CFG_INTDIR will never be ".".
I'm confused. In what situation is llvm-config installed into a directory that isn't the LLVM_TOOLS_INSTALL_DIR? It seems to me that if llvm-config is installed in LLVM_TOOLS_INSTALL_DIR this should work.
This all looks great. I'm really excited to have such a complete toolchain build example in-tree. Thank you!
Sorry for the delay, I've been out of town. I want to loop @phosek into this. I don't think this patch is correct. When I build on Darwin the sanitizer libraries are put in the build directory under lib/clang/$version/darwin/lib (which is correct), and the other runtimes are built under /lib/. This all seems right to me, so I don't understand the problem you're experiencing.
I want to echo @chapuni's question. What is the use case where a distribution would install the CMake modules and not install the LLVM component libraries? It doesn't seem to me that there is any reason to install the CMake modules to support find_package without also installing the static archives.
Is this really something we should be supporting? Building and testing clang with potentially out-of-sync lit or gtest seems undesirable to me.
May 30 2017
One small comment below. In general I agree with the thoughts here, and I think that this is a huge step forward for testing the debug server components.
May 10 2017
May 8 2017
Yep, this seems straight forward.
What is the problem you're trying to solve here? This seems odd to me.
Apr 27 2017
SWEET! this is great to see! One small comment inline, otherwise LGTM.
This looks like a nice improvement. There are some formatting inconsistencies, can you run clang-format?
Apr 21 2017
This is awesome! Thanks!
Apr 18 2017
This looks good to me.
Apr 17 2017
Can you please move this check into HandleLLVMOptions.cmake? By putting it into a module that is vended as part of LLVM's packaging then LLVM subprojects can have consistent settings when building out-of-tree.
Removing code I accidentally left in that was from debugging, and moving some duplicated code that @labath spotted out of the ifdef.
Updating patches to reflect feedback from zturner.
Apr 14 2017
Updating to use MainLoop class, and refactor MainLoop class to operate on Windows.
Apr 12 2017
Adding Greg to the reviewers list.
Adding Greg to the reviewers.
Fixing up the include guard as per feedback from zturner, and fixing up an install logic error that I spoted.
My intention in this patch is not in any way to adversely impact the Xcode project, which is the supported and documented way to build LLDB on OS X (http://lldb.llvm.org/build.html#BuildingLldbOnMacOSX).
Apr 11 2017
Updating the hard coded Config.h
Actually uploading the updates this time...
- Fixing installation to make sure CMake always installs the generated Config.h
- Removing LLDB_CONFIG_FCNTL_GETPATH_SUPPORTED becasue we really don't need it
@labath, I could adapt this into the MainLoop class, but I would actually want to change how that class hierarchy is implemented. Regardless of the event handling/polling model you use much of the code is identical between the classes. I'd rather get rid of MainLoopPosix and MainLoopBase and instead just implement a portable MainLoop class.
This looks good to me!
Apr 7 2017
Fixing a small bug parsing the command line arguments for IPv6 addresses.