In D103906#2806084, @rengolin wrote:

Right, ok, this is true for compiler-rt, but not necessarily other runtime libraries.

There is an open discussion if we should assume compiler-rt is the default and use what we have, but this is a different issue.

If the call to __multi3 is there by accident (leaked from other target's default), then it's ok to remove it. If not, we may need some more complex solution.

@compnerd @joerg @t.p.northover @srhines, do you know of any Arm32 runtime library that implements 128-bit maths with Clang?

I think Android does, so perhaps we can add a condition there on AndroidEABI?

In D101473#2723435, @hctim wrote:

@srhines, @mcgrathr, @phosek, @echristo - please raise your hand if you foresee any problems with bumping compiler-rt to c++17. The rest of LLVM seems to use it just fine.

Adding Dan as an FYI. While this doesn't impact Android platform or regular NDK users, I suppose that someone could have some esoteric build rules that are relying on this, but they should probably be more explicit about what they're running in those cases anyways.

In D96403#2639180, @srhines wrote:

In D96403#2639137, @smeenai wrote:

In D96403#2638932, @rprichard wrote:

In D96403#2638883, @smeenai wrote:

With NDK r22, I only see libunwind.a for armv7. Will it be provided for other architectures in future NDK versions?

NDK r23 should have a libunwind.a for all architectures. This change isn't in the r23 beta 2 that was just released, but it is in the current NDK canary build at https://ci.android.com/builds/branches/aosp-master-ndk/grid. (e.g. build 7220482).

Got it, thanks.

Is there a public repository for viewing the libunwind source state that the NDK r23 libunwind is built from? I have the AOSP source tree checked out (following the instructions in https://source.android.com/setup/build/downloading) on the llvm-toolchain manifest branch. toolchain/llvm-project inside that checkout (https://android.googlesource.com/toolchain/llvm-project) has a branch for ndk-release-r23, but the merge base of that branch and upstream LLVM main is from April 29 2020, and I don't see many newer changes to libunwind after that, so I wasn't sure I was looking at the right place. I know you've done a bunch more libunwind work since then (e.g. D87750, D87881, and D90898), so I was curious if those were gonna be part of r23's libunwind.

So libunwind is now produced as a prebuilt with the rest of the toolchain build, and not built from that copy of toolchain/llvm_project from the NDK packaging/release branch. Thus if you want to rebuild/examine the sources, you need to look at the toolchain that was included for NDK r23, which is clang-r399163b (from https://android.googlesource.com/platform/ndk/+/refs/tags/ndk-r23-beta2/ndk/toolchains.py#25). Given that, you can get the manifest for the toolchain directly here. You can use that manifest to check out the exact sources that were used to build everything (following instructions from here).

^ PROCESS is correct, but I picked the wrong version (not beta2). I'll update and post a better reply in a few minutes.

In D96403#2639137, @smeenai wrote:

In D96403#2638932, @rprichard wrote:

In D96403#2638883, @smeenai wrote:

With NDK r22, I only see libunwind.a for armv7. Will it be provided for other architectures in future NDK versions?

NDK r23 should have a libunwind.a for all architectures. This change isn't in the r23 beta 2 that was just released, but it is in the current NDK canary build at https://ci.android.com/builds/branches/aosp-master-ndk/grid. (e.g. build 7220482).

Got it, thanks.

Is there a public repository for viewing the libunwind source state that the NDK r23 libunwind is built from? I have the AOSP source tree checked out (following the instructions in https://source.android.com/setup/build/downloading) on the llvm-toolchain manifest branch. toolchain/llvm-project inside that checkout (https://android.googlesource.com/toolchain/llvm-project) has a branch for ndk-release-r23, but the merge base of that branch and upstream LLVM main is from April 29 2020, and I don't see many newer changes to libunwind after that, so I wasn't sure I was looking at the right place. I know you've done a bunch more libunwind work since then (e.g. D87750, D87881, and D90898), so I was curious if those were gonna be part of r23's libunwind.

+2 for relanding this.

In D46429#2547020, @MaskRay wrote:

aarch64_elf64_le_vec is a variable name in BFD (extern const bfd_target aarch64_elf64_le_vec;), instead of a BFD emulation.
@srhines Can this workaround be dropped now?

% aarch64-linux-gnu-ld -m aarch64_elf64_le_vec a.o 
aarch64-linux-gnu-ld: unrecognised emulation mode: aarch64_elf64_le_vec
Supported emulations: aarch64linux aarch64elf aarch64elf32 aarch64elf32b aarch64elfb armelf armelfb aarch64linuxb aarch64linux32 aarch64linux32b armelfb_linux_eabi armelf_linux_eabi

Would it be better/easier if we just change the default in the Clang driver at this point to use LLD for Android triples? I believe Dan has patches for doing that which we hadn't submitted yet (because we only recently made it the NDK default).

In D89615#2345350, @jyknight wrote:

In D89615#2342857, @vitalybuka wrote:

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

I think this is ok, but I'd like @eugenis to confirm that he is fine with it too. Thanks for explaining the complexity across the different API levels.

In D83144#2268760, @fmayer wrote:

Thanks for the review! Should I wait for Alex to take a look, or is it fine like this?

Thanks for creating the new test, and for making this more flexible. Everything else looks good here.

Sorry, Jiyong, I had forgotten to subscribe llvm-commits when I first uploaded this (because it's part of compiler-rt and doesn't seem to have a default rule to add them). In any case, this patch LGTM, and I am going to go ahead and approve it since this only affects Android. Let's give it 24 hours for anyone else to comment before committing though (especially since I messed up with the original upload/review).

Yes, let's just submit this now. If it breaks further, then we can consider other options. I'm not sure why there is a separate libc++abi reviewer listed here.

@asmith, are you still working on this patch? The gap-fill feature is definitely needed for parity with existing objcopy implementations.

Thanks, Ryan!

Thanks, Ryan, for diagnosing and addressing this bug.

Thanks, Nick, for cleaning this up and always striving to make things more compatible.

I'm also satisfied with this from the Android side. Thank you for discussing this with Ryan.

LGTM on the Android side. Thanks Dan for handling this.

In D77330#1996255, @grimar wrote:

In D77330#1995415, @nathanchance wrote:

In D77330#1994737, @psmith wrote:

In D77330#1994680, @nickdesaulniers wrote:

Hmm...seems to have regressed ARMv5 Linux kernel boots according to a bisection from @nathanchance (https://groups.google.com/g/clang-built-linux/c/aKDK-N6JN4g)

I don't think that URL is publicly visible.

I don't know why that started happening :/

I think we saw such issue before. That time I think the URL of a page from incognito mode worked (i.e. a different URL to the same page).

In D77168#1988122, @jfb wrote:

In D77168#1988049, @srhines wrote:

pragma clang attribute is interesting, but how do you apply that in a selective fashion to local variables (especially in a way that can be automated)? At first, I didn't think the goal for this should be to create a frequently used option for most end users, but I do think that it could be quite useful for more folks when debugging, especially if it is easy to automate (which optimization-fuel approaches are, while pragmas are not).

__attribute__((uninitialized)) for more selectiveness :)

Of course, not automated. In general we don't really automate compiler things of this sort, say UBSan. OptRemarks is the best we really have to dig into these things. One other option would be to truly randomize the init pattern: emit a handful of different byte patterns, and then see the crash caused by a different pattern, and track it back to which local variable got that byte pattern.

pragma clang attribute is interesting, but how do you apply that in a selective fashion to local variables (especially in a way that can be automated)? At first, I didn't think the goal for this should be to create a frequently used option for most end users, but I do think that it could be quite useful for more folks when debugging, especially if it is easy to automate (which optimization-fuel approaches are, while pragmas are not).

Thank you for making this clear.

I agree with Ryan. Please update the Clang driver for this to pass this same flag for 32-bit ARM as we already pass for AArch64.

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.