I still don't understand the motivation behind this change. You already explicitly set CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY for Android so why do we need to change the default?

LGTM

I have just one comment related to efficiency, but if it turns out to be too difficult to address, I'd be also fine landing this change as is and addressing that issue in a follow up change.

LGTM

LGTM

LGTM

Ideally we would land D143052 but the 3.20 bump is getting pushed back due to buildbot issues so I'd like to land this in the meantime to unblock other cleanup.

I have D142957 that's trying to address the same underlying issue and with that change I can build runtimes with CMake 3.26. It needs to be broken up and landed piecemeal which I started (so far I have D143052 and D144440) but haven't finished yet due to a lack of time. I can try to prioritize it.

I agree with @smeenai, I think we should try to come with a more maintainable and less error prone approach.

LGTM

In D145449#4175864, @phosek wrote:

LLVM_ENABLE_PROJECTS is automatically forwarded from stage 1 builds to
stage 2 builds, so setting FUCHSIA_ENABLE_LLDB has no effect on
two-stage builds.

That shouldn't be the case, do you know where it's being passed through?

LLVM_ENABLE_PROJECTS is automatically forwarded from stage 1 builds to
stage 2 builds, so setting FUCHSIA_ENABLE_LLDB has no effect on
two-stage builds.

LGTM

In D144603#4165569, @fangism wrote:

In D144603#4163751, @phosek wrote:

In D144603#4162974, @haowei wrote:

I think remove the compiler launcher from default pass through flags are fine. What about the using compiler launcher in runtime builds? I don't think we should read CLANG_BOOTSTRAP_PASSTHROUGH to determine if we should pass through compiler launcher flags.

I'd suggest a similar approach, that is remove C_COMPILER_LAUNCHER and CXX_COMPILER_LAUNCHER from the default passthrough list and provide a new LLVM_EXTERNAL_PROJECT_PASSTHROUGH variable.

Will the PASSTHROUGH approach let us choose a different LAUNCHER for the external projects? For example, we could still distribute those compiles, albeit without expecting any caching.

In D144603#4162974, @haowei wrote:

I think remove the compiler launcher from default pass through flags are fine. What about the using compiler launcher in runtime builds? I don't think we should read CLANG_BOOTSTRAP_PASSTHROUGH to determine if we should pass through compiler launcher flags.

I agree, in that case let's remove C_COMPILER_LAUNCHER;CXX_COMPILER_LAUNCHER from the list of default passthrough variables which seems like a reasonable default, and provide CLANG_BOOTSTRAP_EXTRA_PASSTHROUGH so developers have a way to pass these through to the next stage if they want to.

In D144660#4157423, @MaskRay wrote:

It seems reasonable that COMPILER_RT_HAS_LLD is only set to true when we build lld in LLVM_ENABLE_PROJECTS builds or lld is available in a runtimes build.
I.e. don't consider COMPILER_RT_HAS_LLD true in LLVM_ENABLE_PROJECTS builds when lld comes from the system.

@beanz @smeenai do you have any preferences/other ideas?

A more generic version might be to have a list of variables to (not) passthrough.

LGTM

LGTM

LGTM

In D143052#4144613, @DavidSpickett wrote:

Is the intention that we handle this like CMakePolicy.cmake and pull it from the source dir when we build flang, or is it a mistake to move it to the top level?

Thanks!

In D143666#4143702, @michaelplatings wrote:

I'm happy to revert this and add --sysroot if you prefer. That would fit with keeping the tests hermetic.

I missed this change so I apologize for late response, but can you elaborate on why this is needed? Clang first checks the name without architecture and if the file doesn't exist then it'll return the one with architecture unconditionally, see https://github.com/llvm/llvm-project/blob/d61a863050bb4afd22d08bbe53af1e24c0657aba/clang/lib/Driver/ToolChain.cpp#L545. Since there are no files inside https://github.com/llvm/llvm-project/tree/main/clang/test/Driver/Inputs/resource_dir_with_arch_subdir/lib/linux/arm, it should always return the path with architecture as was the case for the previous version. LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON should play no role here since Clang tests are hermetic (as in they don't rely on runtimes being built).

LGTM

LGTM

LGTM

LGTM

LGTM

runtimes/cmake/Modules/WarningFlags.cmake uses target_add_compile_flags_if_supported which is defined in https://github.com/llvm/llvm-project/blob/2b2b8409e6848361577be404bd0ae47a097f0e0c/libcxx/cmake/Modules/HandleLibcxxFlags.cmake#L246. That function uses other functions defined in the same file. That file is also not explicitly included here.

In D143468#4128414, @thieta wrote:

@jrtc27 raises a good point - we have discussed deprecating the option but made it a hard error. Should we instead make it a soft error for now and a hard error later on?

Honestly, I was not expecting this option to be used as much as it seems like it is. So maybe it makes sense to make it a soft error to start.

Any thoughts on this so I can make the appropriate changes?

For the existing YAML files within LLVM, we most commonly used the PascalCase for field names and less commonly lisp-case. This change uses camelCase which is unusual and unless there's a particular reason for using this scheme such as compatibility with an existing format, I'd prefer keeping the format consistent with the rest of LLVM and using PascalCase or lisp-case.

LGTM

LGTM

LGTM

LGTM

Do you know if regular expressions are necessary to cover the existing use cases? In our experience, while regular expressions are powerful, they also tend to be error prone and more difficult to reason about. Would glob patterns that are implemented by https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/GlobPattern.h be sufficient?

LGTM

LGTM

LGTM

LGTM

LGTM

Another alternative would be to move these classes to the Demangle library.

LGTM but can you update the change title and description since the original one no longer matches the content of the patch.

LGTM

LGTM

LGTM