skia_unittests StructTraitsTest.BitmapWithExtraRowBytes crashes on Android (debug-only)
Reported by
agd...@amazon.com,
Oct 30 2017
|
|||||||
Issue descriptionDevice name: Nexus 5X From "Settings > About Chrome" Application version: master branch @ 318723cc573ab5b141c2a40cf1874a8324f0d01e Operating system: Android 8.0.0 URLs (if applicable): N/A Steps to reproduce: (1) Build debug skia_unittests_apk target (2) [out debug Android directory]/bin/run_skia_unittests --num_retries 0 -f StructTraitsTest.BitmapWithExtraRowBytes Expected result: Test passes. Actual result: Test fails. C 27.110s Main ******************************************************************************** C 27.110s Main Detailed Logs C 27.110s Main ******************************************************************************** C 27.110s Main [CRASH] StructTraitsTest.BitmapWithExtraRowBytes: C 27.110s Main [ RUN ] StructTraitsTest.BitmapWithExtraRowBytes C 27.110s Main [ CRASHED ] C 27.110s Main ******************************************************************************** C 27.110s Main Summary C 27.110s Main ******************************************************************************** C 27.110s Main [==========] 1 test ran. C 27.111s Main [ PASSED ] 0 tests. C 27.111s Main [ FAILED ] 1 test, listed below: C 27.111s Main [ FAILED ] StructTraitsTest.BitmapWithExtraRowBytes (CRASHED) C 27.111s Main C 27.111s Main 1 FAILED TEST C 27.111s Main ******************************************************************************** Note: this test passes if the release version of skia_unittests_apk is built+run instead. Test passes if https://chromium.googlesource.com/skia/+/25954b64c066b143819bc720b20a9b4287042ecc is reverted. Symbolized stack trace: Reading native crash info from stdin Reading Android symbols from: src Searching for Chrome symbols from within: src/out/DebugGN/lib.unstripped:src/out/DebugGN/lib:src/out/DebugGN Find ABI:arm Using toolchain from: src/third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi- signal 7 (SIGBUS), code 1, fault addr 0xf3dbbea2 in tid 22075 (st:test_process) pid: 22075, tid: 22075, name: st:test_process >>> org.chromium.native_test:test_process <<< signal 7 (SIGBUS), code 1 (BUS_ADRALN), fault addr 0xf3dbbea2 r0 f3dbbea2 r1 00000000 r2 00000002 r3 00000002 r4 d7d72118 r5 f3dbbea2 r6 00000000 r7 ffaeda58 r8 00000000 r9 00000003 sl 00000022 fp 00000002 ip 00000000 sp ffaed9f8 lr f3dbbea2 pc d7a159be Stack Trace: RELADDR FUNCTION FILE:LINE v------> void neon::memsetT<unsigned int>(unsigned int*, unsigned int, int) src/third_party/skia/src/opts/SkUtils_opts.h:29 000bf9be neon::memset32(unsigned int*, unsigned int, int) src/third_party/skia/src/opts/SkUtils_opts.h:37 v------> sk_memset32(unsigned int*, unsigned int, int) src/third_party/skia/src/core/SkUtils.h:24 000dab07 SkPixmap::erase(unsigned int, SkIRect const&) const src/third_party/skia/src/core/SkPixmap.cpp:190 00072d3b SkBitmap::erase(unsigned int, SkIRect const&) const src/third_party/skia/src/core/SkBitmap.cpp:452 000500d7 skia::StructTraitsTest_BitmapWithExtraRowBytes_Test::TestBody() src/skia/public/interfaces/test/struct_traits_unittest.cc:95 00061f65 testing::Test::Run() src/third_party/googletest/src/googletest/src/gtest.cc:2472 000623ad testing::TestInfo::Run() src/third_party/googletest/src/googletest/src/gtest.cc:2654 000625e3 testing::TestCase::Run() src/third_party/googletest/src/googletest/src/gtest.cc:2772 00065041 testing::internal::UnitTestImpl::RunAllTests() src/third_party/googletest/src/googletest/src/gtest.cc:4677 00064e97 testing::UnitTest::Run() src/third_party/googletest/src/googletest/src/gtest.cc:4285 000a0887 base::TestSuite::Run() src/base/test/test_suite.cc:270 00051e6f int base::internal::Invoker<base::internal::BindState<int (base::TestSuite::*)(), base::internal::UnretainedWrapper<base::TestSuite> >, int ()>::RunImpl<int (base::TestSuite::* const&)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<base::TestSuite> > const&, 0u>(int (base::TestSuite::* const&)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<base::TestSuite> > const&, std::__ndk1::integer_sequence<unsigned int, 0u>) src/base/bind_internal.h:349 v------> base::(anonymous namespace)::LaunchUnitTestsInternal(base::RepeatingCallback<int ()> const&, unsigned int, int, bool, base::RepeatingCallback<void ()> const&) src/base/test/launcher/unit_test_launcher.cc:192 000ab59f base::LaunchUnitTests(int, char**, base::RepeatingCallback<int ()> const&) src/base/test/launcher/unit_test_launcher.cc:475 00051e01 main src/mojo/edk/test/run_all_unittests.cc:46 v------> testing::android::RunTests(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jstring*> const&) src/testing/android/native_test/native_test_launcher.cc:130 00095115 Java_org_chromium_native_1test_NativeTest_nativeRunTests src/out/DebugGN/gen/testing/android/native_test/native_test_jni_headers/testing/jni/NativeTest_jni.h:56 00013cd7 offset 0x11000 /data/app/org.chromium.native_test-T8VU_yFcd-cJq9UEDF2uFA==/oat/arm/base.odex
,
Oct 31 2017
,
Oct 31 2017
Hi there, Since this is a test failure, I think the easiest way to check the issue would be to compile the test target (skia_unittests_apk) so you get the test runner script generated for you and run it locally ([out debug Android directory]/bin/run_skia_unittests --num_retries 0 -f StructTraitsTest.BitmapWithExtraRowBytes).
,
Oct 31 2017
Thank you for providing more feedback. Adding requester "sandeepkumars@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 31 2017
Looping to few chromium//src/skia/OWNERS for debugging this failure.
,
Nov 1 2017
Bizarrely, replacing the while-loop with an equivalent for-loop in SkUtils_opts.h:28 (https://cs.chromium.org/chromium/src/third_party/skia/src/opts/SkUtils_opts.h?l=28&rcl=0b78a6912eba5b50eeb4a13f735957dd57ca6be2) allows the test to pass. while (count --> 0) { *buffer++ = value; } vs for (int i = 0; i < count; i++) { *buffer++ = value; } Haven't looked into the assembly to see what's going on there, but this might be related to a compiler issue?
,
Dec 8 2017
,
Dec 8 2017
Never mind, I realized that one is almost certainly the fault of the client that's using Skia. Dunno about this one.
,
Dec 12 2017
,
Dec 12 2017
Wow, crazy. That does sound like a compiler issue to me. Those loops definitely should do the same thing. Any chance you can send me your two versions of src/core/SkOpts.o, one with the while-loop and one with the for-loop?
,
Dec 12 2017
Ooops, by "send me" I mean, attach to the bug, of course. :)
,
Dec 20 2017
Hey there, sorry about the delayed response! I've uploaded the two versions of SkOpts.o (one for the while-loop and one for the modified for-loop), both from debug builds of the skia_unittests target in my Chromium Android workspace.
,
Jan 24 2018
Looks like the neon::memset32() routines in each object file are identical except for n<4 tail handling at the end. Here's the diff from the while-loop to the for-loop version. This is the very end of the function (the pop into pc is how the function returns).
@@ -23,9 +23,10 @@ Disassembly of section .text._ZN4neon8memset32EPjji:
28: dafa bge.n 20 <neon::memset32(unsigned int*, unsigned int, int)+0x20>
2a: eba2 020c sub.w r2, r2, ip
2e: eb00 008c add.w r0, r0, ip, lsl #2
- 32: 2a01 cmp r2, #1
- 34: bfb8 it lt
- 36: bd80 poplt {r7, pc}
- 38: c002 stmia r0!, {r1}
- 3a: 3a01 subs r2, #1
- 3c: e7f9 b.n 32 <neon::memset32(unsigned int*, unsigned int, int)+0x32>
+ 32: 2300 movs r3, #0
+ 34: 4293 cmp r3, r2
+ 36: bfa8 it ge
+ 38: bd80 popge {r7, pc}
+ 3a: f840 1023 str.w r1, [r0, r3, lsl #2]
+ 3e: 3301 adds r3, #1
+ 40: e7f8 b.n 34 <neon::memset32(unsigned int*, unsigned int, int)+0x34>
I hate to say this, but they both look fine. One steps count (r2) down to zero, while the other steps i (r3) from zero up to count. One stores the 4 byte value (r1) and increments the dst (r0) using stmia, while the other stores those 4 bytes to dst+4*i as i steps along. I don't think the flag-setting 's' part of the adds or the subs have any effect. b.n is an unconditional jump (n is for "near"), so nothing tricky or conditional happening there. The cmp-it-pop parts in the middle are basically just, return if done.
,
Jan 24 2018
Ah. I think I understand! The while-loop version is storing with stmia, from the stm "store-multiple" family of instructions. These require 4 byte alignment. The for-loop version is storing with str.w, which is a 4 byte store that doesn't require 4 byte alignment when a particular bit is set in a system control register. The vectored load and store instructions they both share also obey that same bit as str.w. So I bet that bit is set (probably by default), letting the vst1.32 and str.w instructions write to unaligned memory, while the stmia instructions instead crash. I'll take a look at that unit test to see if it's using any inappropriately aligned buffers. I can certainly imagine someone has offset themselves into a byte buffer to do something tricky in a unit test.
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0366708ab215ba3c1f792371eb14abad4f66bc61 commit 0366708ab215ba3c1f792371eb14abad4f66bc61 Author: Mike Klein <mtklein@chromium.org> Date: Wed Jan 24 22:59:04 2018 align extra bitmap padding in unit test to avoid crash At head this test adds 2 extra bytes of padding to each row of an 8888 (4 byte per pixel) bitmap. This misaligns all the odd rows. Through some sort of combination of lack of testing and luck of code generation, eraseColor() is not crashing on Chromium's tree, but it's within its rights to do so. The attached bug shows such a crash on a 32-bit ARM build running on a Nexus5x. To fix this, I think we can just pad each row by 4 extra bytes. This keeps every row aligned, and I think the spirit of the test intact. Bug: chromium:779715 Change-Id: I58e98e9d7dfe55dbe3512d287158449e21e25d2d Reviewed-on: https://chromium-review.googlesource.com/883741 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Mike Klein <mtklein@chromium.org> Cr-Commit-Position: refs/heads/master@{#531730} [modify] https://crrev.com/0366708ab215ba3c1f792371eb14abad4f66bc61/skia/public/interfaces/test/struct_traits_unittest.cc
,
Jan 24 2018
I think that ought to do it. Please let me know if you are still encountering issues. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by sandeepkumars@chromium.org
, Oct 31 2017Labels: Needs-triage-Mobile Triaged-Mobile Needs-Feedback