LGTM

In D63745#1556941, @labath wrote:
This code should go to tools/debugserver/source/CMakeLists.txt

+1

Thanks for the initiative! Three inline comments for cosmetics.

Polishing

Unfortunately lit is a little more complicated

Remove Xcode restriction and polish

Add CMAKE_EXECUTABLE_SUFFIX when searching for llvm-tblgen

That will be fine with post-commit review. Will submit tomorrow.

After figuring out D62879 that came up on the way, this wasn't too complicated indeed. I put the patch for exporting LLVM_CONFIGURATION_TYPES here: D62878

Patch the replacement logic also in lit/CMakeLists.txt

IIUC Clang uses the same mechanism. Moving append_configuration_directories() to AddLLVM.cmake could help there too. We may consider it for the future once D62859 is through.

Chose the replacement for remaining matches of CMAKE_CFG_INTDIR depdending on LLVM_CONFIGURATION_TYPES

Thanks for your feedback. I should have mentioned that this is pretty much a draft. For now I was happy it works at all.

Fixed debugserver; excluded changes in add_lldb_library() for now; result of this test looks as good as before:

Thanks for having a look. I'd to focus on the mechanism here and keep the contents of the framework bundle unchanged. Happy to discuss this in separate reviews like D62474.

Fix copy/paste mistake for darwin-debug

lldb-vscode is no fremwork tool, but instead it needs the RPATHs

Closing this in favor of the reduced proposal

Ok thanks. I will be OOO next week, so no hurries.

In D61952#1503551, @JDevlieghere wrote:

How does this cleanup affect dependency tracking? Does the build dir become unusable after running ninja install?

In D61952#1504942, @sgraenitz wrote:
Actually, why not make the copy operations PRE_BUILD actions of the test suite instead of POST_BUILD actions of their executables?

In D61952#1505019, @sgraenitz wrote:
The solution to the "install -> test" issue would be having both I guess.

In D61956#1505144, @xiaobai wrote:

I noticed you have lots of comments that say "Release has different values for these variables".

Fix default install locations and add comments on how to use DESTDIR

Sure, ideally CMake defined a global install order and this order would handle overlap. I think that's unlikely to happen. It took quite some time to find and fix an overlap case downstream and I think it's worth avoiding it in general in the future. OTOH I see that the solution shouldn't be too intrusive. For the swift resources: I am not familiar with the details; all I know is that tests fail if they are missing.

Add back CACHE STRING "" for CMAKE variables

Yet another option: we could install the framework tools to a fake location and copy them to their final destination (overwriting their build-tree pendants) in a manual install step at the very end of the install process. The problem here is, that there may be more things in the build-tree framework than we overwrite, and thus remain in the install-tree (thinking about Swift resources in swift-lldb for example).

I thought about one more option, but I don't think it's better than the current proposal: instead of deleting the build-tree resources in the very first install step, I could install the framework manually at this point and skip its regular install target. We would loose implicit stripping and RPATH replacement for the dylib though. Should be possible to do it manually, but if it breaks, we won't notice. So, it's not quite appealing.

Actually, why not make the copy operations PRE_BUILD actions of the test suite instead of POST_BUILD actions of their executables?

In D61952#1504346, @labath wrote:

I'm not very familiar with frameworks and complexities involved in creating them, but the fact that we need to delete stuff from the build tree in order to install properly makes me think that there is something fishy going on. Is there no way to arrange things so that this can be avoided?

Thanks for your input.

Rename macOS specific cache file.
Add settings and comments.
Fix install destinations by appending Developer and SharedFrameworks respectively.

Yes, sorry for that. I realized it after the commit was in. The commit I got from arc patch did have the original author information. It must have changed in git llvm push, probably because it's still going to SVN and then gets mirrored back to git. I missed this last detail.

Sure, will commit this on your behalf tomorrow. (If nothing else comes up.)

LGTM

Thanks for adding this. Would it make sense to use LLVM_ENABLE_PROJECTS_USED? https://github.com/llvm/llvm-project/blob/a568222d/llvm/CMakeLists.txt#L128

It looks like this test is flaky [...] what would you say to just deleting the "# CHECK: 1 location added to breakpoint 1" line from the test?

In D61611#1496782, @stella.stamenova wrote:

I can update config.py to create the right features, but we can also use UNSUPPORTED: system-windows, so I'll look into the best way to do this.

Perfect, thanks

Add back test requirement target-x86_64

In D61611#1496644, @probinson wrote:

Sorry for the drive-by... what is this REQUIRES: nowindows? I don't see where lit generates this property. I grepped all of llvm-project.git and I see it used in two tests, but not where it's produced. Which suggests that those tests don't actually run *anywhere*.

1. XFAIL: system-windows

@stella.stamenova Can you have a look at the lit test please? It works on macOS and Linux, but I didn't test Windows. Should I add something like # REQUIRES: nowindows or is it fine like this?

Generalize lit test

Ok, I went with MCJIT for now. Will leave this here for future reference.

Thanks for your reply and thoughts about that.

Add lit test

For illustration: pass ownership to ObjectLinkingLayer and hand out a MemBufferRef in NotifyLoaded

Ok I see, as long as the original MemoryBuffer exists, we can recreate an identical ObjectFile on the client side. Currently it gets deleted together with JITLinkContext when returning from jitLink().

Maybe not the most elegant solution, but this seems to work: https://reviews.llvm.org/D61065

One crucial part that I found missing here is a way to connect JITEventListener (like GDBJITRegistration, PerfJITEvents, etc.), because the new ObjectLinkingLayer::NotifyLoaded function only provides the module key but no ObjectFile so we cannot create and pass over a LoadedObjectInfo as with RuntimeDyld. Please see my inline comments for some pointers.

LGTM too. Did this land?

This was mostly to illustrate usage of the matching LLVM commit. Unlikely to break something. Post-commit review should be sufficient.

Yes, scopes are per directory; macros operate on the caller's scope and functions add one.