In D110720#3109821, @probinson wrote:

The lines where CHECK is split into CHECK-LINUX and CHECK-NOLINUX are also enabling the test for nvptx.

It's true that the nvptx case is now checking more than it used to, but not everything; making it check everything could probably be done, but is it worthwhile? I experimented with passing CHECK to the nvptx case, and it doesn't pass; there would have to be a CHECK-NVPTX line paralleling each CHECK: line, pretty much. Seems like a lot of effort, but is nvptx really that special?
I'll do it if someone says Yes, it's worth the trouble; but unless someone can say that, I'd rather not.

... but it seems much easier to put the -fno-outline-atomics flag into the linker's .bp file instead of changing the behavior of __system_property_get.

In D114523#3165542, @bc-lee wrote:

From what I understand, for dynamic executables, si->call_constructors() [1] calls the constructor of each loaded shared object. Right?

I put the LD_DEBUG=2 environment variable and found that libc.so's constructor is called before the main executable or any other shared object's constructor. And libc.so calls __libc_preinit_impl -> __libc_init_common -> __system_properties_init.
So the point is that __system_properties_init is initialized before this compiler-rt code is called. But I don't know if this is guaranteed behavior.

Of course, I think this code should work fine even if __system_properties_init is not yet called.

[1] https://cs.android.com/android/platform/superproject/+/9aafec59ccad4b3851269e4119561efd8a2f5e65:bionic/linker/linker_main.cpp;l=508

Also: is it OK to use a mix of LSE and non-LSE atomic operations to access the same memory concurrently? I would think that should be fine, but it seems worth mentioning.

I noticed that using __system_property_get from a constructor function probably doesn't work within the Bionic loader (linker[64]), but maybe Bionic ought to be fixed (and it shouldn't matter much in practice):

• The Bionic dynamic loader calls the constructor functions before calling __system_properties_init. (see tmp_linker_so.call_constructors();)
• However, a static executable calls __system_properties_init before the constructors, so static executables should be OK.
• Maybe the dynamic loader ought to call __system_properties_init before constructors.
• Calling __system_property_get before __system_properties_init is probably OK? It looks like SystemProperties::Find would return nullptr, and __system_property_get would return 0.
• In any case, it wouldn't actually matter unless someone ran a new loader on an affected device+kernel.

The motivating issue happened with Android (D107509).

Adding some Android folks to the subscriber list.

The dereferenceable.ll and dereferenceable-inseltpoison.ll files are also confusing to me. It *sounds* like they're trying to test inferred attributes, but its CHECK* lines would allow any or no attributes on the functions, so it's not actually testing anything attribute-related.

Follow up from D107527 review.

I noticed this inconsistency while reviewing D107509.

Update test.

Yes, Android did add stpcpy and stpncpy in API 21:
https://android.googlesource.com/platform/bionic/+/refs/heads/android11-dev/libc/libc.map.txt

In D107127#2917667, @eugenis wrote:

It resolves everything to the first definition in the lookup order. The only special handling of weak symbols that I've found is that missing definition is not an error.

In D107127#2917555, @eugenis wrote:

Weak symbol is a purely link-time concept, AFAIK. It is ignored in the dynamic loader, so, unless we link multiple copies of this file into the same DSO, "weak" attribute here should not have any effect.

Yeah, this looks fine to me.

Update another place.

error: [FE_UPWARD] in __compiler_rt_scalbn(-0x1p+0 [BFF0000000000000], 10000) = -0x1.fffffffffffffp+1023 [FFEFFFFFFFFFFFFF] != -inf [FFF0000000000000]

In D96403#2639210, @srhines wrote:

It is actually using clang-r416183 from toolchains.py. From there, you can go to the manifest for building those prebuilts. This line shows you the SHA for the toolchain/llvm-project used to build the prebuilts. Finally, you can look here to see what was merged. Note that there's a typo for the revision number in the commit message, but the upstream LLVM SHA mentioned there is correct. Here is where this toolchain was built in case that is useful/interesting.

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?

Rebase this revision.

I think this is OK for Android.

MSVC's scalbn appears to ignore the current rounding mode, so the value returned from __compiler_rt_scalbn[f] disagreed with the value returned from scalbn[f]. I think it's sufficient to disable the non-default-rounding-mode tests for MSVC. With this change, the __divdc3 and __divsc3 builtins, when compiled with MSVC, should produce the same results as on Linux systems (or as with MinGW and the other Unix systems I tested).

Disable non-default-rounding-mode scalbn[f] tests when using the MSVC libraries.

In D97356#2586381, @vitalybuka wrote:

This happens only once after clean build, so it did not annoy us, but I don't mind for even bigger value e.g. 10m

The new unit tests failed on sanitizer-windows. I reverted it for now. https://lab.llvm.org/buildbot/#builders/127/builds/6620

Account for both x86_64,arm64,x86,arm and x86_64,x86,arm64,arm orderings.

OTOH, my internal master Cuttlefish VM sets ro.product.cpu.abilist to x86_64,arm64-v8a,x86,armeabi-v7a,armeabi.

FWIW: The sanitizer tests passed on an x86_64 R emulator (RSR1.200819.001.A1).

I also tested a version of this change where I instead added --rtlib=libgcc to the flags in configure_android. It was necessary to add --rtlib=libgcc both for compiling and linking, because at compile-time, Clang can use outline-atomics on AArch64. On other architectures, though, the --rtlib=libgcc has no effect at compile-time, and Clang printed a warning. It looks like this stage2 compiler is only used to target Android, though, so it seemed simpler to just override the default.

The coff-dwarf.test buildbot failures were fixed by https://github.com/llvm/llvm-project/commit/0fd7c31a098efdfaa5a57dbac6e9c0921b00a999 (on Feb 11).

For reference, I know of four ways that LLVM's libunwind is currently linked into programs:

• Fuchsia/Windows (i.e. Clang's default behavior): it's part of libc++, e.g.:
  • Fuchsia:
    • fuchsia/prebuilt/third_party/clang/linux-x64/lib/x86_64-unknown-fuchsia/c++/libc++.so is INPUT(libc++.so.2 -lunwind -lc++abi).
    • fuchsia/prebuilt/third_party/clang/linux-x64/lib/x86_64-unknown-fuchsia/c++/libc++.a is an archive that contains the libunwind object files.
  • In https://github.com/mstorsjo/llvm-mingw/releases/tag/20201020:
    • llvm-mingw\x86_64-w64-mingw32\bin\libc++.dll uses _Unwind_Resume from libunwind.dll
    • llvm-mingw\x86_64-w64-mingw32\lib\libc++.dll.a also seems to import _Unwind_Resume from libunwind.dll
    • llvm-mingw\x86_64-w64-mingw32\lib\libc++.a has the libunwind object files in it
• FreeBSD: libclang_rt.builtins and libunwind are repackaged into libraries that look like libgcc, linked implicitly.
• Darwin: it's part of libSystem, linked implicitly.
• crosstool in google3: I think this build system is passing --unwindlib=libunwind to the driver.

Ping.

Maybe this is blocked on someone from Apple reviewing the Mach-O parts?

Rebase and fix merge conflict in gn file.

Update libunwind/src/BUILD.gn for macro name change.

The Android part of this change looks fine to me.

In D96560#2558737, @pcc wrote:

In D96560#2558596, @rprichard wrote:

It seems OK to me. Does it make sense to also skip (pc == 0) in UnwindCursor<A, R>::setInfoForSigReturn?

Hmm, that seems redundant with the check that I'm adding here. Let's start with one and we can reconsider if we find another bug here.

It seems OK to me. Does it make sense to also skip (pc == 0) in UnwindCursor<A, R>::setInfoForSigReturn?

Adding glandium and thakis for Firefox and Chrome. e.g. I suspect this change would cause the same sort of breakage seen in D95166.

Adding a couple of other people as possible reviewers.

emaste: Just a heads up in case FreeBSD is affected. FWIW, I noticed that libgcc_eh.a on FreeBSD 12.1 doesn't hide the _Unwind_* or unw_* symbols. The build system is defining VISIBILITY_HIDDEN but libunwind doesn't respond to that name. FreeBSD libgcc_eh.a also defines hidden symbols named logAPIs, logUnwinding, and logDWARF. libunwind defines these internal names if NDEBUG isn't defined. They can break statically-linked programs on FreeBSD (e.g. duplicate symbol linker errors).

Restore Mach-O alias fixes.

After renaming the CMake option, CMake prints a warning if I set the old option:

CMake Warning:
  Manually-specified variables were not used by the project:

Just fix ELF and leave Mach-O as-is.

In D91841#2428875, @MaskRay wrote:

Implementing the relevant functions called by divxc3 is one choice, another choice is to emulate libgcc - don't use scalbn.

In D90898#2496725, @compnerd wrote:

In D90898#2384775, @rprichard wrote:

In D90898#2379387, @compnerd wrote:

Could you add a bit of a comment explaining why the XOM case is not protected against?

I think I can expand on the comment.

libunwind can be used for generating backtraces during regular execution (e.g. profiling) and crashing if the XOM is in use seems unfortunate.

For profiling, I assume the unwind info needs to be accurate at every instruction? I know that's definitely not the case for AArch64, but I thought it's still only mostly-accurate on other archs? I don't *really* know that unwinding through a signal handler is important for Android, but I don't know how to rule it out either, and we do have a test for it. I think the test predated Android's libunwindstack unwinder. Maybe someone could be using _Unwind_Backtrace in a segfault handler, and that probably avoids the async-unwind-info problem most of the time. But I also don't think that's how third-party Android crash dumpers work.

Sure, though, libunwind is generic and is used outside of android as well, including other AArch64 targets, so we do need to consider those use cases while making changes.