I should have looked more closely at the AOSP sources, because we already hard-code the expected 4KB max-page-size. We do have some crufty flags that can be cleaned up, so I'll do that instead (https://android-review.googlesource.com/c/platform/build/soong/+/1278815). :) I'll also go ahead and create/test a CL to revert the Clang driver change forcing 4KB max-page-size next week.

So it turns out that the ld shipped in the NDK actually did have the 64KB max-page-size patch, so not taking this patch would change current user behavior. Thanks to @MaskRay for sharing the exact binutils patch we were looking for to verify that it has it. For that reason, this patch should be fine for our 32-bit ARM NDK users. For the platform, we'll see if we should just set the max-page-size back to 4KB explicitly in the build rules, but that shouldn't affect this CL.

We (Android Google folks) have an internal thread going on about what implications this might have, but I can say that I am concerned about making this kind of change for ARM Android targets. Even though we manually set a 4KB max page size in our platform builds for 32-bit, we also have to consider the needs of NDK developers (i.e. those folks making native applications/games/etc.). Some of their build configurations might behave very differently with this change, so my current thinking is that 64KB should not be the default for ARM Android targets.

In D77168#1955312, @jfb wrote:

Do you not think pragma is a more general approach? That's what's used in a bunch of other cases, and I'd like to see it attempted here.
If not, I agree with John that just counting up isn't a good bisection experience. I'd rather see a begin / end bound.

In D77168#1953635, @jfb wrote:

I'm not sure this is a good idea at all. We want to keep the codepath as simple as possible to avoid introducing bugs. If a codebase sees a crash then it's easier to bisect one function at a time than doing something like this. I'd much rather see bisection using pragma to apply the uninitialized attribute to multiple declarations.

In D76452#1945029, @MaskRay wrote:

In D76452#1944786, @srhines wrote:

In D76452#1944733, @MaskRay wrote:

Another approach will be -DCLANG_DEFAULT_LINKER=lld. It provides a default value for -fuse-ld=.

How does that work for Darwin builds? Is lld fully supported for MachO at this point, or will Clang still choose to use the preferred Apple linker?

I am not clear about your use case. lld can be used as ELF/Mach-O/COFF/WebAssembly linker. If you want to build ELF objects, -fuse-ld=lld (via clangDriver) or configure clang with -DCLANG_DEFAULT_LINKER=lld.
If you want to build Mach-O objects, lld's current Mach-O port lacks features (and will be removed). The new Mach-O port is still under development.

In D76452#1944733, @MaskRay wrote:

Another approach will be -DCLANG_DEFAULT_LINKER=lld. It provides a default value for -fuse-ld=.

Thanks, Dan, for setting this up.

In D69925#1736434, @efriedma wrote:

I think the reason it's written this way is that CLANG_VERSION_STRING might not correspond to an LLVM version? For example, Apple has its own versioning scheme.

In D68269#1693496, @danalbert wrote:

If the Android and FreeBSD folks are ok with this, I'm fine with it

Fine for Android for at least the platform and the NDK (we don't actually build with CMake in those cases). +srhines just in case there's another case I don't know about.

Looks really nice. I am sure the NDK developers will be happy to see support for static OpenMP. Do you want to add the public NDK github issue link in the commit message?

LGTM, but please wait for someone more familiar with LangRef policies to review that part.

In D64666#1650325, @sylvestre.ledru wrote:

@ziangwan maybe you should add this improvement to the release notes, wdyt?

Thanks for improving this for LLDB and Android.

You have patch.patch in the diff above. You should drop that.

s/integrates/integrated in the title, but otherwise this looks good.

Craig, can you confirm that this is acceptable? I don't think there are any chips with SSE4.2 but without cx16, so this just seemed like an oversight. It might be a good idea to really audit the list of possible CPU features for other missing inclusions. Another idea would be to set a baseline minimum CPU for Android, which would cut down on having to specify so many features separately.

FYI, https://reviews.llvm.org/D56571 is the review where this was accepted.

Everything looks great. Thanks for adding these improvements. While it's probably safe to commit this, perhaps you should give it 24 hours in case some of the other clang-tidy folks have different style or testing concerns.

Thanks for picking this up and finishing it.

The revert is fine with me, although I still would like Ryan to follow up on verifying that the suggested fix for Bionic will indeed work for all cases. I think it is fine for you to merge now to unbreak things for everyone else though. Thanks for working through the problem.

This LGTM if Ryan is happy with it. Thanks for taking care of getting a workaround implemented until this can be fixed.

I had just a few small suggestions to make things more consistent. Feel free to ignore my fallthrough comment, but I feel like -Wimplicit-fallthrough has been quite valuable in Android.

I added Peter and Oliver since they might have some suggestions here too.

Thanks for taking a look at this.

Thanks for finding this. Note that this is the exact code in comparesf2.c.

In D60577#1463469, @smeenai wrote:

I have zero context on this change specifically, but "we should prefer functions in a private namespace" seems to be the exact opposite of the advice in https://llvm.org/docs/CodingStandards.html#static

Thank you for fixing this quickly!

In addition to -fno-everything, don't forget about -fnothing and -fno-nothing, which seem like they would also be nice to have.

@jmorse can I get your approval on this CL before we submit it? Thanks.

https://reviews.llvm.org/D59807 has the fix for the nondeterminism. Thanks for the hints about the reverse iteration order. That helps make it more likely to fail deterministically, but it still runs a small chance of "passing" if the fix isn't there.

Switching it to a MapVector makes it deterministic. I was just struggling late last night to try to reduce the .ll file down to something worth checking in! If you want to take over to minimize the test case, that would be much appreciated. Let me know so I can continue doing that if needed. Thanks.

This patch appears to generate non-reproducible builds in some cases. I can craft a more minimal test case, but the following link (https://godbolt.org/z/sWucUZ) is what I have been using. If I run Clang multiple times, the output eventually swaps the order of some undef DEBUG_VALUE's. I am just passing "-O1 -g" with that .ii file. It isn't obvious to me yet what is causing this to be unordered/non-deterministic.

Dan, this seems pretty important for the NDK. If you submit this, would you want it cherry-picked ASAP?

I asked Chris to comment here about libunwindstack. Hopefully we can get the problem fixed, but I think this workaround might be necessary for code running on older devices in any case (i.e. NDK apps).

Very cool, thank you for coming up with a great way to improve this case.

Would it be reasonable to have a test for this with perhaps an invalid GCC installation? There is some mock GCC/sysroot testing in https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-gcc-toolchain.c and https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-ndk-standalone.cpp. I am not sure that it will be easy to trip this same bug that way, but I think it is possible.

LGTM for removing the broken Android builders.

In D36806#1385936, @lhames wrote:

Looks like this was LGTM'd but never applied. Stephen -- do you have commit access?

Sorry about the delay.

Craig, does this look ok now?

In D55029#1312167, @ruiu wrote:

I wouldn't rush to submit this now, given that this issue is not new at all. Maybe we can just wait for Peter's response?

Thanks Zhizhou! I'm curious about Peter's answer to the 64KiB default as well, but I think we should move forward with getting this patch submitted.

I just wanted to follow up here.

Yabin did some of the work to bring TSan to Android. Perhaps he knows whether this is has significant use. Considering that it is meant as a debugging aide, I would actually be comfortable with moving it. You are also correct that we never officially marked it as a feature of the NDK (or platform). That is something we want to do though.