Optimizer bitshift bug for ARM v7

[llvmbot]

Bugzilla Link	44747
Resolution	WORKSFORME
Resolved on	Feb 11, 2020 12:54
Version	8.0
OS	Windows NT
Attachments	Android Studio example project, screenshots, assembly review
Reporter	LLVM Bugzilla Contributor
CC	@efriedma-quic,@nickdesaulniers,@zygoloid

Extended Description

During our work with the pixman graphics library, we've discovered an optimizer bug that results in incorrect bit shift math. There are no compile time errors, but the output result is completely incorrect at runtime.

My development environment is using Windows 10, Android Studio v3.5.3, Android NDK v20.0.5594570, with Clang v8.0.7. This bug appears to only happen when targeting armeabi-v7a for 32-bit Android with -O1 or higher, and not for arm64-v8a or any x86 variant.

I've attached an Android Studio sample project demonstrating the error, as well as screenshots shots showing the invalid math calculation. Looking at android.c, lines 8 and 10 cause the issue, where lines 17 and 19 do not (since they are not optimized due to the optnone attribute). The data ends up in the wrong byte position.

After looking through the generated assembly, it appears to be generating an orr instruction with an incorrect shift value. I've attached a short review showing some of the assembly differences between optimized and unoptimized versions.

[llvmbot]

Hi Brian,
From the upstream clang/llvm we have the 8.0.1 branch I am not able to reproduce the error. Are you able to obtain the exact clang command used? And version, with clang -v ?

I tried reproducing with this command:
clang --target=arm-linux-androideabi -O3 -S -o - -x c - < /tmp/8/ClangBugTest/app/jni/android.c -I/tmp/8/ClangBugTest/app/jni

I do get the correct result for both functions:
test_set_optimized:
.fnstart
@ %bb.0:
ldrb r12, [r1, #​3]
ldrb r3, [r1, #​7]
orr r3, r12, r3, lsl #​16
str r3, [r0]
ldrb r3, [r1, #​1]
ldrb r1, [r1, #​5]
orr r1, r1, r3, lsl #​16
str r1, [r0, #​4]
ldrb r1, [r2, #​3]
ldrb r3, [r2, #​7]
orr r1, r1, r3, lsl #​16
str r1, [r0, #​8]
ldrb r1, [r2, #​1]
ldrb r2, [r2, #​5]
orr r1, r2, r1, lsl #​16
str r1, [r0, #​12]
bx lr

test_set_unop:
.fnstart
@ %bb.0:
.pad #​12
sub sp, sp, #​12
str r0, [sp, #​8]
str r1, [sp, #​4]
str r2, [sp]
ldr r0, [sp, #​4]
ldrb r1, [r0, #​3]
ldrb r0, [r0, #​7]
lsl r0, r0, #​8
orr r0, r1, r0, lsl #​8
ldr r1, [sp, #​8]
str r0, [r1]
ldr r0, [sp, #​4]
ldrb r1, [r0, #​1]
ldrb r0, [r0, #​5]
orr r0, r0, r1, lsl #​16
ldr r1, [sp, #​8]
str r0, [r1, #​4]
ldr r0, [sp]
ldrb r1, [r0, #​3]
ldrb r0, [r0, #​7]
lsl r0, r0, #​8
orr r0, r1, r0, lsl #​8
ldr r1, [sp, #​8]
str r0, [r1, #​8]
ldr r0, [sp]
ldrb r1, [r0, #​1]
ldrb r0, [r0, #​5]
orr r0, r0, r1, lsl #​16
ldr r1, [sp, #​8]
str r0, [r1, #​12]
add sp, sp, #​12
bx lr

[llvmbot]

It reproduces for me using this exact command line:

clang -target armv7-none-linux-androideabi19 -fdata-sections -ffunction-sections -fstack-protector-strong -funwind-tables -no-canonical-prefixes --sysroot C:/Users/Work/AppData/Local/Android/Sdk/ndk/20.0.5594570/build//../toolchains/llvm/prebuilt/windows-x86_64/sysroot -g -Wno-invalid-command-line-argument -Wno-unused-command-line-argument -fno-addrsig -fpic -march=armv7-a -DNDEBUG -mfpu=neon -S -DANDROID -fvisibility=hidden -DLLONG_MAX -DUSHRT_MAX=65535 -isystem /usr/include -DHAVE_STDINT_H -DHAVE_UINT64_T -DXP_UNIX -nostdinc++ -Wa,--noexecstack -Wformat -Werror=format-security -O3 -c android.c

And running clang -v gives me the following output:

Android (5220042 based on r346389c) clang version 8.0.7 (https://android.googlesource.com/toolchain/clang b55f2d4ebfd35bf643d27dbca1bb228957008617) (https://android.googlesource.com/toolchain/llvm 3c393fe7a7e13b0fba4ac75a01aa683d7a5b11cd) (based on LLVM 8.0.7svn)
Target: x86_64-w64-windows-gnu
Thread model: posix
InstalledDir: C:\Users\Work\AppData\Local\Android\Sdk\ndk\20.0.5594570\toolchains\llvm\prebuilt\windows-x86_64\bin

If I run using your command line, I still get the incorrect output as follows (I notice some shift differences in the unoptimized version here as well, but I'm not very well versed in ARM assembly and couldn't say if they are an issue):

Command line: clang --target=arm-linux-androideabi -O3 -S -o - -x c - < android.c

Assembly:

test_set_optimized:
.fnstart
@ %bb.0:
ldrb r12, [r1, #​3]
ldrb r3, [r1, #​7]
orr r3, r12, r3, lsl #​16
str r3, [r0]
ldrb r3, [r1, #​1]
ldrb r1, [r1, #​5]
orr r1, r1, r3, lsl #​8
str r1, [r0, #​4]
ldrb r1, [r2, #​3]
ldrb r3, [r2, #​7]
orr r1, r1, r3, lsl #​16
str r1, [r0, #​8]
ldrb r1, [r2, #​1]
ldrb r2, [r2, #​5]
orr r1, r2, r1, lsl #​8
str r1, [r0, #​12]
bx lr
.Lfunc_end0:
test_set_unop:
.fnstart
@ %bb.0:
.pad #​12
sub sp, sp, #​12
str r0, [sp, #​8]
str r1, [sp, #​4]
str r2, [sp]
ldr r0, [sp, #​4]
ldrb r1, [r0, #​3]
ldrb r0, [r0, #​7]
orr r0, r1, r0, lsl #​8
ldr r1, [sp, #​8]
str r0, [r1]
ldr r0, [sp, #​4]
ldrb r1, [r0, #​1]
ldrb r0, [r0, #​5]
orr r0, r0, r1, lsl #​8
ldr r1, [sp, #​8]
str r0, [r1, #​4]
ldr r0, [sp]
ldrb r1, [r0, #​3]
ldrb r0, [r0, #​7]
orr r0, r1, r0, lsl #​8
ldr r1, [sp, #​8]
str r0, [r1, #​8]
ldr r0, [sp]
ldrb r1, [r0, #​1]
ldrb r0, [r0, #​5]
orr r0, r0, r1, lsl #​8
ldr r1, [sp, #​8]
str r0, [r1, #​12]
add sp, sp, #​12
bx lr
.Lfunc_end1:

[llvmbot]

Hi Brian,
from
https://android.googlesource.com/toolchain/llvm/+/3c393fe7a7e13b0fba4ac75a01aa683d7a5b11cd

I figured out that the actual llvm commit was:
commit 0184c53
Author: Evgeniy Stepanov eugeni.stepanov@gmail.com
Date: Sat Jan 5 00:44:58 2019 +0000

Revert "Revert "[hwasan] Android: Switch from TLS_SLOT_TSAN(8) to TLS_SLOT_SANITIZER(6)""

This reapplies commit r348983.

llvm-svn: 350448

Indeed I confirm that in this version it existed such bug. But it is not present in clang 9.0 or in trunk.

Cheers

[efriedma-quic]

Closing based on comment 3.

(There's a separate bugtracker for the Android ndk at https://github.com/android/ndk/issues, if you want to report an issue specifically with that version of clang.)