Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Miscompilation on armv7 with NDK r25c #1860

Closed
ngg opened this issue Apr 7, 2023 · 3 comments
Closed

[Bug]: Miscompilation on armv7 with NDK r25c #1860

ngg opened this issue Apr 7, 2023 · 3 comments
Assignees
Labels

Comments

@ngg
Copy link

ngg commented Apr 7, 2023

Description

When we tried to switch from NDK r24 to r25c, our application has started to crash when compiled to armv7.

Unfortunately, I couldn't easily minimize our code to submit it here, but I managed to reproduce the same crash on armv7 Linux using upstream clang 15.0.7. Updating clang to 16.0.1 solved the crash, so I bisected the llvm-project repo to find that the necessary commit for the fix is llvm/llvm-project@8a09875. The fix itself is not armv7 specific, I assume it can cause problems on any platform.

I found a discussion here https://discourse.llvm.org/t/instcombine-struct-w-padding-aggregate-loads-stores-as-scalars/67035/5 that describes the issue and has small testcases that can reproduce the miscompilation, but on CUDA target. I don't have a small isolated testcase for Android, I hope the information I'm providing here is enough.

I tried to cherry-pick the commit onto llvm/llvm-project@282c83c which clang-r450784d1 is based on which is included in NDK r25c, there is a conflict in the test case that this fix modifies but the fix itself in llvm/lib/Analysis/InstructionSimplify.cpp is ok and works, it no longer crashes.

Please cherry-pick the llvm/llvm-project@8a09875 commit into the next NDK release.

Upstream bug

llvm/llvm-project#59156

Commit to cherry-pick

llvm/llvm-project@8a09875

Affected versions

r25

Canary version

No response

Host OS

Linux

Host OS version

Debian 11

Affected ABIs

armeabi-v7a

@ngg ngg added the bug label Apr 7, 2023
@DanAlbert DanAlbert added this to NDK r26 Apr 7, 2023
@github-project-automation github-project-automation bot moved this to Triaged in NDK r26 Apr 7, 2023
@DanAlbert
Copy link
Member

I don't think we're going to be doing an r25d, so we'll get this "for free" when we update the compiler in r26. Leaving this open since we don't actually have a newer working compiler for r26 yet.

@DanAlbert DanAlbert moved this from Triaged to Needs prebuilt update in NDK r26 Apr 7, 2023
@DanAlbert
Copy link
Member

Oh, and thanks for the detailed investigation :) That saves us a ton of time.

@ngg
Copy link
Author

ngg commented Apr 8, 2023

Ok, we'll stay on r24 until r26 is released.

I've also bisected which change between r24 and r25 caused the crash which lead to llvm/llvm-project@2caf85a (https://reviews.llvm.org/D112811).
I think this commit just revealed the already existing bug with insertvalue that was later fixed by llvm/llvm-project@8a09875.

@github-project-automation github-project-automation bot moved this from Needs prebuilt update to Merged in NDK r26 Apr 27, 2023
foxsen pushed a commit to android-la64/platform-ndk that referenced this issue Oct 10, 2024
Includes a bunch of test `#include` fixes for the reason explained in
the changelog.

Also updates the ASan test. The ASan failure message has been rephrased
so the old test no longer matched the death message correctly. As best
as I can tell the test we have is a copy of the upstream test with all
the supporting functions from the various sanitizer test support headers
copied into it. I've replaced the guts of the test and copied the new
required supporting functions.

This also disables the TSan smoke test. It hangs indefinitely, and we
don't have timeout support (`timeout` is not available on old devices).
The test was already an xfail, so we can just disable it for now. The
test will still be built, so we can be sure that it won't suffer build-
time regressions.

A few libc++ tests were disabled because the tests (either the failing
asserts or whole tests) were removed upstream.

Bug: android/ndk#1298
Bug: android/ndk#1677
Bug: android/ndk#1834
Bug: android/ndk#1860
Bug: android/ndk#1862
Test: checkbuild.py && run_tests.py
Change-Id: Id467537487908cad99099bfeb2cfacf4787d875c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Merged
Development

No branches or pull requests

3 participants