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?

LGTM

In D64579#1581557, @beanz wrote:

Yea, this makes sense even if it is a bit sad. We do use the --whole-archive approach for libLLVM, and it works. I was hoping to avoid it here in part to free up the ability to link libclang_shared before or in parallel to archiving the libclang*.a archives.

LGTM

Nice.

In D64579#1581194, @smeenai wrote:

In D64579#1581172, @phosek wrote:

While upgrading to newer CMake would be nice, I think it's unlikely that we could move all the way to 3.12 since that version was only released a year ago and still isn't available in most distributions, so this LGTM. Maybe leave a TODO that this should be revisited after we bump minimum CMake requirement to 3.12 or newer?

FWIW, CMake 3.4.3 was released Jan 25 2016 (https://cmake.org/pipermail/cmake/2016-January/062616.html) and we started requiring it on May 31 2016 (https://reviews.llvm.org/D20822), so we do have precedence for requiring a pretty recent CMake version. I guess we should figure out what other compelling reasons we'd have for moving to that version though.

In D64579#1581172, @phosek wrote:

While upgrading to newer CMake would be nice, I think it's unlikely that we could move all the way to 3.12 since that version was only released a year ago and still isn't available in most distributions, so this LGTM. Maybe leave a TODO that this should be revisited after we bump minimum CMake requirement to 3.12 or newer?

@beanz, what do you think of this? I think it's kinda awful, but I can't think of a better alternative short of upgrading to CMake 3.12 (as detailed in my comment).

In D64538#1579610, @rnk wrote:

In D64538#1579573, @smeenai wrote:

In D64538#1579561, @rnk wrote:

An extremely convenient feature of the current escaping pattern is that it magically works for both the cmd shell and various bash implementations. You can simply copy paste the commands and run them. This matters, for example, for the .sh crash reproducer script. On Windows today, you can just run it with bash, and it works. So, I'd prefer it if we didn't do this.

If you want -### output to be more FileCheck friendly, then I would recommend adding a new driver flag for testing that dumps the commands without doing any escaping. This came up before:
https://reviews.llvm.org/D62493

As long as the backslashes aren't followed by a few special characters (as detailed in my comment), the backslash should still continue to work. I've been testing this with git bash and I'm still able to copy-paste the ouptut command.

For the use case of crash reproduction, we often get these .sh files from users, and then try to reproduce and reduce the crash on Linux. Right now it's just a matter of adjusting the clang binary being run to reproduce a crash, but with this change, it will be harder. The current escaping is kind of the universal currency, a quoting style that is accepted everywhere.

In D64538#1579561, @rnk wrote:

An extremely convenient feature of the current escaping pattern is that it magically works for both the cmd shell and various bash implementations. You can simply copy paste the commands and run them. This matters, for example, for the .sh crash reproducer script. On Windows today, you can just run it with bash, and it works. So, I'd prefer it if we didn't do this.

If you want -### output to be more FileCheck friendly, then I would recommend adding a new driver flag for testing that dumps the commands without doing any escaping. This came up before:
https://reviews.llvm.org/D62493

CC @phosek, since I believe you had issues writing tests referencing the installation directory in the past.

LGTM with the comment; the use case makes sense to me.

In D64538#1579510, @smeenai wrote:

Hmm, this only fixes -###. Looking into -v now.

Hmm, this only fixes -###. Looking into -v now.

In D64191#1577566, @thakis wrote:

FYIe, you can also prevent this symbol to be linked in by putting a cxa_demangle function in an obj file that gets linked first, like so: https://cs.chromium.org/chromium/src/third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp?type=cs&q=cxa_demangle+file:android&sq=package:chromium&g=0&l=54 – but I agree that since cxa_demangle is so large, it'd be nice if the upstream version was behind some kind of define. Then we could set that and remove that hack in chromium too. I don't have an opinion on if that should be a new define or if _LIBCXXABI_NO_EXCEPTIONS should be reused. Actually, I think we build libcxxabi with exceptions enabled because some of our test code can in theory throw exceptions, but we have an external dumper so we don't need the stack from terminate() – so it'd be somewhat more convenient for us if we could turn off the stack from terminate() (and the pulling in of __cxa_demangle) while still being able to build without setting _LIBCXXABI_NO_EXCEPTIONS.

In D58418#1577349, @jkorous wrote:

Thanks for the revert.

There's actually one more problem - seems like ninja doesn't like the generated build.ninja file on Linux.

ninja: error: build.ninja:52390: bad $-escape (literal $ must be written as $$)

http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/13617/steps/build-stage1-compiler/logs/stdio

In D64089#1576902, @ldionne wrote:

Background question: I'm familiar with the behavior on Darwin, where we prefer C++ headers in the toolchain (alongside the driver), however do other platforms behave the same? I thought this was something specific to Darwin.

I think it's worth advertising the required configuration change (to explicitly say -DLIBCXX_CXX_ABI=libcxxabi) in the documentation and/or the release notes, otherwise the behavior change will be surprising.

LGTM

Ping.

Nice!

Given that we're following an existing name here, and we'd have to change that existing name as well were we to decide on a new name, I'm going to go ahead and commit this on @anushabasana's behalf. We can figure out the best name to use here and then change all of them at once post-commit.