Repository navigation
Closed
Labels
A-spuriousArea: Spurious failures in builds (spuriously == for no apparent reason)Area: Spurious failures in builds (spuriously == for no apparent reason)O-androidOperating system: AndroidOperating system: AndroidT-releaseRelevant to the release subteam, which will review and decide on the PR/issue.Relevant to the release subteam, which will review and decide on the PR/issue.
Description
The same test has failed on Travis in multiple PRs, including at least:
- Rollup of 11 pull requests #49675 (comment)
- Rollup of 8 pull requests #49696 (comment)
- Add GlobalAlloc trait + tweaks for initial stabilization #49669 (comment)
- Merge the std_unicode crate into the core crate #49698 (comment)
I can reproduce on some branches (but not master) by running RUST_TEST_THREADS=1 src/ci/docker/run.sh arm-android. The end of the output looks like:
test atomic::int_nand ... ok
test atomic::int_or ... ok
test atomic::int_xor ... ok
test atomic::static_init ... died due to signal 11
error: test failed, to rerun pass '--test coretests'
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "arm-linux-androideabi" "-j" "8" "--release" "--locked" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--"
expected success, got: exit code: 3
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target arm-linux-androideabi
Which means the test process dies with “SIGSEGV Invalid memory reference” while executing this test:
static S_FALSE: AtomicBool = AtomicBool::new(false);
static S_TRUE: AtomicBool = AtomicBool::new(true);
static S_INT: AtomicIsize = AtomicIsize::new(0);
static S_UINT: AtomicUsize = AtomicUsize::new(0);
#[test]
fn static_init() {
assert!(!S_FALSE.load(SeqCst));
assert!(S_TRUE.load(SeqCst));
assert!(S_INT.load(SeqCst) == 0);
assert!(S_UINT.load(SeqCst) == 0);
}I haven’t figured out how to use gdb inside the Android emulator inside Docker to debug this further.
CC @alexcrichton, @rust-lang/release
Metadata
Metadata
Assignees
Labels
A-spuriousArea: Spurious failures in builds (spuriously == for no apparent reason)Area: Spurious failures in builds (spuriously == for no apparent reason)O-androidOperating system: AndroidOperating system: AndroidT-releaseRelevant to the release subteam, which will review and decide on the PR/issue.Relevant to the release subteam, which will review and decide on the PR/issue.
Activity
kennytm commentedon Apr 8, 2018
Assume the error is first discovered in #49675 (based on 74abffe). The final PRs involved in that rollup are:
It was reported that the error still appears locally, so one of the above would be the cause.
From #49669 (comment), we knew the cause should be already contained by d919eb671b5c6d3c68221831a8d1186f492ea4d4 (i.e. 48fa6f9 on master). 74abffe...48fa6f9 includes the following PRs:
PRs in both lists are:
alexcrichton commentedon Apr 9, 2018
Yes I've been able to reproduce this locally and it seems to be unrelated to changes that we're landing and otherwise just related to how libcore or libstd itself is optimized. I've attempted to debug this to get an actual cause but to no avail so far.
alexcrichton commentedon Apr 9, 2018
I'm pretty certain though that this isn't spurious in the sense that
@bors: retrywould do much, it seems deterministic based on changes in libstd/libcore. The changes, however, don't feel at all related to the bug still...alexcrichton commentedon Apr 9, 2018
Ok I think I've found the cause of this. I've got no idea how this ever worked before! (as is par for the course on these kinds of bugs)
So a program like this:
generates roughly this IR:
Now the crucial thing here is that we don't actually modify
@FOOin LLVM, so when run through optimizations it determines:Which sounds reasonable! As a result, our constant was promoted from BSS to read-only data. We can see this:
Here the
bmeans bss andrmeans rodata.Now the tricky part comes later during the actual codegen. Here if you disassemble the function we get:
So it turns out we're not using atomic instructions at all, but rather we're using the
__sync_val_compare_and_swap_4intrinsic function. On Android this is loaded from libgcc, and the definition bottoms out in the__kuser_cmpxchgfunction provided by the kernel. This kernel helper presumably does syscall stuff if necessary, but on the emulator the kernel apparently discovered that the hardware supports atomic instructions so it's using atomic instructions! Notably gdb says that the faulting instruction is:Turns out
r2is the same as the address to our static which is in rodata. Now that we're in rodata this ends up failing with a fault because we can't actually modify rodata.So it looks like tl;dr; LLVM will aggressively flag unmodified internal globals as
constant(yay!) which allocates them in rodata instead of bss. In this case, however, the intrinsic operation that we lowered down to ended up actually doing a write which LLVM didn't know about making that an invalid optimization.Given that we don't actually have many instances of atomics without modifying them I'm just gonna fix the test and call it a day...
alexcrichton commentedon Apr 9, 2018
As to how this ever worked I'm sort of msytified. I'm just gonna assume that all rodata doesn't actually end up being read-only for various reasons in the loader and we got lucky before where our constants here just happened to wind up in a read/write section of memory
alexcrichton commentedon Apr 9, 2018
I've posted a workaround at #49811
cuviper commentedon Apr 9, 2018
This sounds like something that still needs to be fixed!
BatmanAoD commentedon Apr 9, 2018
Since we have no control over Android's libgcc, is it possible that the implementation on an actual Android device might be updated in the future to use real atomic instructions, thus triggering the bug on that device as well as in the emulator?
std: Be sure to modify atomics in tests
alexcrichton commentedon Apr 9, 2018
@cuviper indeed! I've opened https://bugs.llvm.org/show_bug.cgi?id=37061
alexcrichton commentedon Apr 9, 2018
@BatmanAoD hm I'm not sure I quite follow? It looks like our vanilla
arm-linux-androideabiis targeting such an old version of ARM/Android that it requires usage of intrinsics for atomic operations. The implementation of the intrinsics are behaving differently than what LLVM assumes, aka loading from an atomic is actually doing a cmpxchg to learn the value. The actual write happens deep within the intrinsic inside of the kernel-provided__kuser_cmpxchgfunction, and I'm sort of just assuming that the kernel is doing runtime detection to see if atomics are actually available and filling in an appropriate implementaiton. If a kernel implementation of the cmpxchg intrinsic were used I'm not sure if it'd also deliver a fault.If the code is recompiled with support for atomic instructions then I don't think any fault will happen as LLVM's assumption that the static is never modified will actually be true (as it's generating the atomic instructions).
Does that answer your question?
Auto merge of #49811 - alexcrichton:fix-android, r=SimonSapin
BatmanAoD commentedon Apr 10, 2018
@alexcrichton Yes; I misunderstood or misread and somehow got the impression that libgcc was responsible for making the determination of whether to invoke the
strexeqinstruction.