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
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