Fair enough; I'm convinced :) LGTM

In D146666#4214806, @mstorsjo wrote:

In D146666#4214769, @smeenai wrote:

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

Yeah, in one sense, that’s the correct thing to do. But on the other hand, even if I’m doing a simple native build, if I’m going out of my way to specify the path to these tools, perhaps we should respect it. We already do it somewhat inconsistently - AFAIK the variables do take effect for the tablegen family of tools without checking the LLVM_USE_HOST_TOOLS variables.

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

D146227 is a fix diff that's up and waiting for CI.

LGTM

Note that we aren't using toString(InputFile*) here because it

includes the install name for dylibs in its output, and ld64's map file
does not contain those.

LGTM

Heh, "target" being an extremely overloaded term strikes again :)

Okay, -print-before-all did the trick, and the local variables it's complaining about are from https://github.com/llvm/llvm-project/blob/08b65c5c9e58a4f9e79a52a0fccd70e4c4ebd43b/libcxx/include/__memory/uninitialized_algorithms.h#L520. I guess we have some ODR issue with the specific types that template is being instantiated with (reverse_iterator<reverse_iterator<internal_type*>>) that's being exposed now?

I'm still seeing LTO errors after this patch:

fragment covers entire variable
  call void @llvm.dbg.value(metadata i64 poison, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %5, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
fragment covers entire variable
  call void @llvm.dbg.value(metadata i64 poison, metadata !798697, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !798698
!798697 = !DILocalVariable(name: "__last", arg: 3, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %7, metadata !798697, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798697 = !DILocalVariable(name: "__last", arg: 3, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %9, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %12, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
LLVM ERROR: Broken module found, compilation aborted!

One other alternative for Rust to consider would be to support building as part of an LLVM build using LLVM_EXTERNAL_PROJECTS and LLVM_EXTERNAL_<PROJECT>_SOURCE_DIR, so that Rust becomes part of the LLVM build graph and can easily access its targets. See https://github.com/apple/swift/blob/c95e11c011e5e6ee19f9c21332ff04df29dd49db/cmake/modules/SwiftSharedCMakeConfig.cmake#L269 for how Swift does it.

Adding the usual CMake reviewers.

LGTM

LGTM

In D117852#4164315, @vvavrychuk wrote:

In case of Visual Studio 2019/2022, Windows SDK are installed outside of MSVS directory

C:\Program Files (x86)\Windows Kits\10

In D144603#4162192, @haowei wrote:

In D144603#4162048, @smeenai wrote:

Hmm, what cache key does ccache use for compilers? Is it the --version output string, the path, or some combination? If the path is involved then I don't see any value in using ccache for configuring anything past stage1, since the compiler will (presumably) be installed somewhere else afterwards and not used directly from the build tree. If it's just --version, what are the cache pollution concerns? The compiler binaries output by all the stages should produce the same outputs given the same inputs (and presumably have the same --version), right?

I am not sure about ccache. We use an internal distributed build cache system when we build LLVM. The compiler binary hash and the --version output will both be used to calculate the cache key so there is little value to use caching past stage1 as the runtime build and stage2+ will be built from a fresh from source compiler and it will always result in cache miss.

I don't believe ccache only uses --version output as cache key since two compiler binary with the same version string can have different output (e.g. 2 WIP source tree but track the same git revision).

So think it would be beneficial to add a flag to avoid using compiler launcher past stage1.

Hmm, what cache key does ccache use for compilers? Is it the --version output string, the path, or some combination? If the path is involved then I don't see any value in using ccache for configuring anything past stage1, since the compiler will (presumably) be installed somewhere else afterwards and not used directly from the build tree. If it's just --version, what are the cache pollution concerns? The compiler binaries output by all the stages should produce the same outputs given the same inputs (and presumably have the same --version), right?

Rename per review comment

In D124490#4151517, @MaskRay wrote:

In D124490#4146219, @smeenai wrote:

Ping @MaskRay. Have your blocking concerns been addressed?

Very sorry for missing these notifications! I'll read this patch soon...

Ping @ldionne, would you be able to take a look at this soon (or are you okay waiving the libc++ blocking requirement for it)? This is really useful for Android armv7, where the triple is traditionally spelled armv7-none-linux-androideabi but normalized to arvm7-none-linux-android, and this patch would resolve that discrepancy.

Ping @MaskRay

it's just a matter of replacing -I with -isystem to get the correct behaviour

In D139254#4045730, @xiangzhangllvm wrote:

! In D139254#4036649, @smeenai wrote:
I'm going on an extended leave soon, and I won't have time to implement it before then. I've asked some coworkers to take a look, but if you have the time to add the option it'd be hugely appreciated :)

Hi @smeenai I first implement at an llvm option for your requirement, you can use -mllvm -option-name to quick solve your code size problem.
PLS refer D141556

@tamas' suggestion would be a good change to make IMO, but I think it's outside the scope of this patch, and the patch as-is improves the status quo, so LGTM.

@MaskRay, were you still planning to review this/do you have any unaddressed concerns?

In D141184#4033243, @philnik wrote:

LGTM assuming D139147 is being worked on.

Ah, I didn't consider the case where you were using LLVM_RUNTIME_TARGETS but not LLVM_BUILTIN_TARGETS. Thanks.

In D139254#4034785, @xiangzhangllvm wrote:

Hi @smeenai , do you have plan to implement it ? If not, I will do it, but may some days later (due to some other jobs in my hands).

LGTM, thank you!

In D138196#4029676, @arichardson wrote:

In D138196#4029503, @smeenai wrote:

In D138196#4019550, @thakis wrote:

cstdlib only has the following using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;, so I assume it would also fail with std::aligned_alloc(). I don't have a chromium checkout, but maybe the C library that your build is using doesn't provide aligned_alloc() ? Or maybe it requires an additional macro to expose it? Otherwise you could add another opt-out case to libcxx/include/__config (like the macos < 10.15 one from this commit).

It's broken on android (bionic) and chromeos (some glibc, I think?).

libcxx/test/std/depr/depr.c.headers/stdlib_h.aligned_alloc.compile.pass.cpp claims "::aligned_alloc is available starting with Android P (API 28)". We support API level 24, so we can't use that yet.

So I think this breaks most people doing Android builds, so it'd probably be good if _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC was set correctly automatically for at least Android.

(But we'll just define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC ourselves for now.)

CC @danalbert and @rprichard. (We hit the same breakage as well.)

If you give me instructions to test this and the right macro to check I can try to submit a fix, but it will probably be a lot quicker if someone with an existing development setup can add the right line to __config.

In D127812#4031849, @ilinpv wrote:

In D127812#4031313, @smeenai wrote:

We can use -mno-fmv to avoid that dependency, right? We're interested in using that for our own code (where we don't make use of function multi-versioning), and want to prevent the compiler-rt support from being pulled in from the archive unnecessarily. It'd still be available for users who needed it.

Right, you will need to explicitly provide '-mno-fmv` then. Currently __aarch64_cpu_features cpu_model.c stuff located in bultins (libclang_rt.builtins-aarch64.a). Did I understand you correctly that your apps linked agains libclang_rt.builtins-aarch64.a and if we move function multiversioning part to new library, lets say libclang_rt.cpu_features-aarch64.a, that will resolve your concern ? As a sidenote, builtins/cpu_model.c contains X86 CPU features used in function multiversioning on that target as well.

In D127812#4031101, @ilinpv wrote:

In D127812#4030881, @smeenai wrote:

We're not actually using multi-versioning anywhere, but we're still paying the size cost for it as a result. Would we consider moving the newly added functions into their own file (or perhaps moving the outlined atomics functions into a different file), so that you can use outlined atomics without also paying the size cost of function multiversioning if you don't need it?

Function multiversioning expects compiler-rt has __aarch64_cpu_features, it will be broken if compiler-rt miss that ( clang/lib/Driver/ToolChains/Clang.cpp:7231 ). I believe function multiversioning will be used in Android as outline atomics already did.

In D139254#4030325, @xiangzhangllvm wrote:

In D139254#4029517, @smeenai wrote:

Would there be any objections to adding a flag to restore the previous behavior? This is a fairly hefty size increase for us, and we don't think the additional checking is worth the size increase in our case. We also intend to look into the size increases more to determine how much is from guarding calls to e.g. __cxa_throw, where you will reuse the stack frame, vs. calls to e.g. abort or _Unwind_Resume, where the stack frame should never be reused and checking the stack cookie doesn't seem worthwhile (if my logic is sound, which I'm not 100% sure of).

I agree to add a option to go in previous behavior. (e.g -fstack-protector-weak or -fstack-protector-return)
But this enhance for no-return function is necessary for a security. We meet such cases in user code. That is why I did it.

I'm investigating a size increase we observed after this change for Meta's Android apps. This increases the size of compiler-rt by 1.6 KB, which is small by itself, but then compiler-rt is statically linked into each SO, and some of our apps have hundreds of SOs, so the increase adds up to a sizeable total. (We would ideally have far fewer SOs, but that's a pretty involved change.)

Would there be any objections to adding a flag to restore the previous behavior? This is a fairly hefty size increase for us, and we don't think the additional checking is worth the size increase in our case. We also intend to look into the size increases more to determine how much is from guarding calls to e.g. __cxa_throw, where you will reuse the stack frame, vs. calls to e.g. abort or _Unwind_Resume, where the stack frame should never be reused and checking the stack cookie doesn't seem worthwhile (if my logic is sound, which I'm not 100% sure of).

In D138196#4019550, @thakis wrote:

cstdlib only has the following using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;, so I assume it would also fail with std::aligned_alloc(). I don't have a chromium checkout, but maybe the C library that your build is using doesn't provide aligned_alloc() ? Or maybe it requires an additional macro to expose it? Otherwise you could add another opt-out case to libcxx/include/__config (like the macos < 10.15 one from this commit).

It's broken on android (bionic) and chromeos (some glibc, I think?).

libcxx/test/std/depr/depr.c.headers/stdlib_h.aligned_alloc.compile.pass.cpp claims "::aligned_alloc is available starting with Android P (API 28)". We support API level 24, so we can't use that yet.

So I think this breaks most people doing Android builds, so it'd probably be good if _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC was set correctly automatically for at least Android.

(But we'll just define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC ourselves for now.)

Address comment

Rebase to see if CI is happier

Pinging the libc++abi reviewer group.

The size regressions from this are acceptably small for us, so we have no problems with that. Thanks for flagging it.

In D122780#3978947, @nilayvaish wrote:

In D122780#3978826, @smeenai wrote:

I'm not able to apply the latest version of the patch, and it looks like CI isn't either. Which revision is this patch based on top of?

Your stack still has D122858 in it, which is making arc patch unhappy locally, but even taking the raw patch and using git apply is failing for me.

@smeenai , can you try again? I think I don't understand the 'arc' tool that well.

I'm not able to apply the latest version of the patch, and it looks like CI isn't either. Which revision is this patch based on top of?

I'll be going on an extended vacation soon. If anyone needs this sooner, they should feel free to pick it up, otherwise I'll get to it when I'm back in February. (Similarly, there's no rush on our end for you to figure out the platform issues you observed.)

LGTM

Ah, I was hoping I'd be able to rebase the current version onto the 15.x release branch cleanly, but I'm running into conflicts that I'm not sure how to resolve. I'll wait for the rebased version and then try that.

Thanks for looking into the APK size impact. Could you give me a couple of days (I'll try my best to get to this tomorrow) to evaluate this on Meta's Android apps as well? I maintain the libc++ used by those, and we have some apps that are incredibly sensitive to APK size (increases of even a few KB are flagged), so I'd appreciate the chance to evaluate this before it lands. (If I haven't been able to get to this by Wednesday of next week, feel free to go ahead regardless.)

I think that'd be pretty nice. Right now there's a bunch of different and inconsistent ways to control which parts of compiler-rt you build (e.g. the sanitizers have COMPILER_RT_SANITIZERS_TO_BUILD, but then things like XRay and libfuzzer have dedicated COMPILER_RT_BUILD_* variables), and it'd be great to have a consistent interface instead. I know @compnerd and some others have been interested in reorganizing compiler-rt, and this could be part of that.

CI is happy now. Could someone from the libc++abi group please take a look?

Pausing this while D138461 is sorted out.

Hopefully make CI happy this time

LGTM

Might be worth a mention in either https://llvm.org/docs/AdvancedBuilds.html#multi-stage-pgo, to make it a bit more discoverable.

Rebase and synchronize threads to avoid duplicates

This seems like a pretty simple and non-intrusive method to be able to tune the outliner. LGTM if you add a test case.

Thanks for the revert.

It's not. I'm out for a few days; could you revert this for me please?

Rebase