Thanks for the quick review!

Update test

@sdesmalen How does the new test case look?

Fix test

Use -run-pass=prologepilog in test

In D69018#1711129, @sdesmalen wrote:

Thanks for fixing this @smeenai!

In D69018#1710443, @smeenai wrote:

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

You can create a MIR test with a single instruction accessing a pre-allocated stackslot with a very large offset, and check that the offset is generated correctly.

e.g. the following MIR

---
 name: D69018
 tracksRegLiveness: true
 fixedStack:
   - { id: 0, offset: 2147483648, size: 1}
 body: |
   bb.0:
     $x0 = LDRXui %fixed-stack.0, 0
     RET_ReallyLR
 ...

compiles with your patch, but fails to complete (i.e. it keeps running) without it.

Add test

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

Is there any way to add a test for this?

libc++ also has a version of this check (https://github.com/llvm/llvm-project/blob/master/libcxx/cmake/Modules/CheckLibcxxAtomic.cmake). Does that need to be adjusted as well?

Makes sense to me, particularly since this is generalizing what we're already doing for compiler-rt.

This only works because of a specific detail of LLVM's build system. The variable will get set in the directory scope, so different directories including this module will still duplicate the check. Here's a simple test I did, using cmake version 3.12.1 (run this as a script):

LGTM

Do we not build libc++abi with -fvisibility=hidden? That would also have prevented this, right?

LGTM

In D67512#1674846, @sanjoy wrote:

This patch individually is fine, but why are you setting scalar-evolution-max-iterations in the first place? It is a debug option that end-users should ideally never have to change.

Add test

Added test, as suggested by @jdoerfert on IRC.

In D65924#1620382, @jfb wrote:

In D65924#1620379, @smeenai wrote:

I take it LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN will be going away entirely? If so, LGTM.

I intend to leave it there so it's easy to do the same dance for the next compiler bump. It won't be active for a bit, but we also won't have to re-agree on how to bump the version next time around :)

I take it LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN will be going away entirely? If so, LGTM.

In D65831#1619712, @compnerd wrote:

Hmm, yes, it is for use from the build tree. I don't see how INTERFACE_INCLUDE_DIRECTORIES can do a -include though. It has to be INTERFACE_COMPILE_OPTIONS or INTERFACE_COMPILE_DEFINITIONS.

When libc++ is installed, the __config_site and __config are concatenated to make a single __config. I'm imagining this is specific to using libc++ from the build tree? If so, you should use INTERFACE_INCLUDE_DIRECTORIES and $<BUILD_INTERFACE:...> to limit it to builds.

Output to stdout

I'm not tied to the name -###-verbatim and am open to suggestions if anyone can think of something better.

Abandoning in favor of D65839

LGTM

Adding @rnk as someone familiar with the driver/frontend :)

LGTM

LGTM, thanks!

LGTM

In D61479#1586987, @sberg wrote:

eh, the summary here doesn't get updated from the actual git commit message :(

thanks for the reviews!

LGTM

Sorry, I missed this. Thanks, LGTM.

LGTM

In D64846#1590169, @lanza wrote:

You can use CROSS_TOOLCHAIN_FLAGS_<PROJECT_NAME> (in this case CROSS_TOOLCHAIN_FLAGS_LLDB) to pass additional variables to the cross-compiling configure. That seems more appropriate for setting these options instead of hardcoding them in LLDB's CMake.

I think LLDB_PATH_TO_NATIVE_CLANG_BUILD along with the error that would occur makes it much clearer to the user what's happening. CROSS_TOOLCHAIN_FLAGS is a hell of a lot more ambiguous as to it's purpose than the former. If you've never heard of the NATIVE build then this would point it out to you relatively easily.

In D64837#1589983, @lanza wrote:

In D64837#1588826, @smeenai wrote:

This just seems like it's replacing all instances of LLVM with ${CMAKE_PROJECT_NAME}?

I think it'd be cleaner and more explicit to pass an additional project name parameter to llvm_create_cross_target instead of implicitly using ${CMAKE_PROJECT_NAME}. There's some prior art for that, e.g. in https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;366290$1009?as=source&blame=off

That's what I did originally. However, the users of build_natve_tool wouldn't have the proper context to know what to use. It's only non-llvm specific usage is within add_tablegen which would then have to learn how to decide what project_name variable to pass in. e.g. if lldb is part of ENABLE_PROJECTS then add_tablegen should call llvm_native_build(LLVM NATIVE ...) else llvm_native_build(LLDB NATIVE ...). So we'd have to propagate that to all users of add_tablegen where we'd have to figure out whether the build is standalone or not via checking CMAKE_PROJECT_NAME anyways. e.g.

 if (${CMAKE_PROJECT_NAME} MATCHES lldb)
    add_tablegen(lldb lldb lldb-tblgen)
else()
    add_tablegen(llvm lldb lldb-tblgen)
endif()

I'm not convinced that adding the granularity to choose your llvm_create_cross_target project is worth propagating the configuration outwards given that there are no users that don't satisfy CMAKE_PROJECT_NAME == project_name.

An alternative would be to have some global variable llvm_cross_targets list that build_native_tool iterates over. But this is making an already clumsy solution more clumsy.

In D64846#1588823, @smeenai wrote:

Having Swift-specific options in here is a non-starter, since Swift support isn't in mainline LLDB yet.

You can use CROSS_TOOLCHAIN_FLAGS_<PROJECT_NAME> (in this case CROSS_TOOLCHAIN_FLAGS_LLDB) to pass additional variables to the cross-compiling configure. That seems more appropriate for setting these options instead of hardcoding them in LLDB's CMake.

This just seems like it's replacing all instances of LLVM with ${CMAKE_PROJECT_NAME}?

Having Swift-specific options in here is a non-starter, since Swift support isn't in mainline LLDB yet.

In D63735#1587999, @steven_wu wrote:

This adds an unnecessary static initializer to the MachOObject.cpp. Can you move the array of validArchs into filescope? You can declare it in getValidArchs() and use getValidArchs() in isValidArchs.

Ping.

Distributed ThinLTO doesn't support archives: http://lists.llvm.org/pipermail/llvm-dev/2019-June/133145.html. @tejohnson suggested -start-lib -end-lib as a workaround, but that's not a thing for COFF. Do you have plans for archive support, or is that not something you need to deal with for your use cases?