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.

LGTM!

Fixing variable naming conventions

I will fix up the naming conventions. Switching back and forth between LLVM and LLDB conventions has done a number on my brain.

Adding Jim as a reviewer.

Some cleanup to the test case:

This looks reasonable to me.

This seems like a reasonable workaround until we can figure out a better solution. Thanks Sean!

I think putting this into AddSphinxTarget.cmake would be a better way to handle it. Also I have one small inline comment.

One minor nitpick on the test case, but otherwise this looks fine to me.

@mgorny, because of differences in linker semantics between Darwin and ELF, I can't reproduce the failure you have locally. I think that the patch below works around it in a more-portable way.

Looks reasonable to me.

LGTM, sorry for the delay!

Please revert your patch. It is *not* the right solution and is masking underlying problems.

This is definitely not the right fix. Please revert.

Looks reasonable to me. Thanks!

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.

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.