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.

LGTM!

This LGTM.

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.

LGTM.

LGTM.

Since the dependent patch landed, this LGTM too.

LGTM!

LGTM!

This also looks good to me.

This looks pretty cool to me.

LGTM.

@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.

@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.

LGTM!

LGTM!

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.

LGTM!

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?

LGTM

LGTM!

This LGTM.

LGTM

LGTM.

LGTM!

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!

LGTM!

LGTM!

I think it is better to XFAIL Windows and have test coverage on non-windows systems than to have the tests not running anywhere.

This looks great! Thanks for doing this.

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.

In D34322#783532, @joerg wrote:

My suggestion would be to just use the YAML writer for now and leave a comment to make it clear that this is really JSON only.

One question inline below.

LGTM

LGTM

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!

LGTM.

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.

LGTM!

Is this really something we should be supporting? Building and testing clang with potentially out-of-sync lit or gtest seems undesirable to me.

LGTM!

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.

Yep, this seems straight forward.

What is the problem you're trying to solve here? This seems odd to me.

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?

This is awesome! Thanks!

This looks good to me.

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.

Updating to use MainLoop class, and refactor MainLoop class to operate on Windows.

Adding Greg to the reviewers list.

Adding Greg to the reviewers.

LGTM.

LGTM!

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

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!

Fixing a small bug parsing the command line arguments for IPv6 addresses.