CoordinationUnitImplTest.AddChild crashes on Android when built with clang |
||||||
Issue descriptionhttps://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/43182 I 46.250s run_tests_on_device(0607455313c8659d) [ RUN ] CoordinationUnitImplTest.AddChild I 46.250s run_tests_on_device(0607455313c8659d) [ CRASHED ] Bot output isn't very helpful; possibly some alignment issue (see e.g. issue 729059 or issue 726137 for an example of an alignment bug.) (filed issue 732060 for the trybots not finding this)
,
Jun 12 2017
# enable test locally, then: $ time ninja -C out/gnandrel -j 2000 services_unittests $ out/gnandrel/bin/run_services_unittests --gtest_filter=CoordinationUnitImplTest.* # in another shell, run logcat, copy stack into stack.txt, then: $ third_party/android_platform/development/scripts/stack --output-directory=out/gnandrel stack.txt Stack Trace: RELADDR FUNCTION FILE:LINE 01b54e10 MurmurHash64A(void const*, int, unsigned long long) /usr/local/google/home/thakis/src/chrome/src/third_party/smhasher/src/MurmurHash2.cpp:108 0108a379 CoordinationUnitID /usr/local/google/home/thakis/src/chrome/src/services/resource_coordinator/public/cpp/coordination_unit_id.cc:24 001b8437 TestCoordinationUnit /usr/local/google/home/thakis/src/chrome/src/services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:31 001b8669 TestBody /usr/local/google/home/thakis/src/chrome/src/services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:108 01002c2f Run /usr/local/google/home/thakis/src/chrome/src/third_party/googletest/src/googletest/src/gtest.cc:2471 010030c5 Run /usr/local/google/home/thakis/src/chrome/src/third_party/googletest/src/googletest/src/gtest.cc:2653 0100332b Run /usr/local/google/home/thakis/src/chrome/src/third_party/googletest/src/googletest/src/gtest.cc:2771 01005de5 RunAllTests /usr/local/google/home/thakis/src/chrome/src/third_party/googletest/src/googletest/src/gtest.cc:4648 01005c3b Run /usr/local/google/home/thakis/src/chrome/src/third_party/googletest/src/googletest/src/gtest.cc:4256 01b1c1ef Run /usr/local/google/home/thakis/src/chrome/src/base/test/test_suite.cc:271 v------> LaunchUnitTestsInternal /usr/local/google/home/thakis/src/chrome/src/base/test/launcher/unit_test_launcher.cc:192 01b1f4fd LaunchUnitTests /usr/local/google/home/thakis/src/chrome/src/base/test/launcher/unit_test_launcher.cc:458 010933ed InitializeAndLaunchUnitTests /usr/local/google/home/thakis/src/chrome/src/services/service_manager/public/cpp/test/common_initialization.cc:52 001c0f5b main /usr/local/google/home/thakis/src/chrome/src/services/test/run_all_service_tests.cc:59 v------> RunTests /usr/local/google/home/thakis/src/chrome/src/testing/android/native_test/native_test_launcher.cc:131 0109483b Java_org_chromium_native_1test_NativeTest_nativeRunTests /usr/local/google/home/thakis/src/chrome/src/out/gnandrel/gen/testing/android/native_test/native_test_jni_headers/testing/jni/NativeTest_jni.h:49 005b2c29 offset 0x587000 /data/app/org.chromium.native_test-1/oat/arm/base.odex
,
Jun 12 2017
(this is with a checkout at 05d5a8764e7541360793bdb1b1c06748121fa0d9 from May 26; line numbers possibly not very reliable)
,
Jun 12 2017
https://chromium.googlesource.com/chromium/src/+/05d5a8764e7541360793bdb1b1c06748121fa0d9/services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc https://chromium.googlesource.com/chromium/src/+/05d5a8764e7541360793bdb1b1c06748121fa0d9/services/resource_coordinator/public/cpp/coordination_unit_id.cc https://cs.chromium.org/chromium/src/third_party/smhasher/src/MurmurHash2.cpp?q=MurmurHash2.cpp+package:%5Echromium$&dr&l=108 Sure smells like some alignment thing!
,
Jun 12 2017
That code was added here: https://codereview.chromium.org/2914003002 matthalp, murmurhash assumes that inputs are aligned (see link in comment 5), yet CreateMurmurHash64A() just calls it hoping that that's already the case. Oh, that change merely moved that code around. Code actually from oysteine's https://codereview.chromium.org/2798713002
,
Jun 12 2017
,
Jun 12 2017
Yup, this fixes it:
thakis@thakis:~/src/chrome/src$ cd third_party/smhasher/src/
thakis@thakis:~/src/chrome/src/third_party/smhasher/src$ git diff
diff --git a/MurmurHash2.cpp b/MurmurHash2.cpp
index cd1e53a..ef15566 100644
--- a/MurmurHash2.cpp
+++ b/MurmurHash2.cpp
@@ -15,6 +15,8 @@
#include "MurmurHash2.h"
+#include <string.h>
+
//-----------------------------------------------------------------------------
// Platform-specific functions and macros
@@ -52,7 +54,9 @@ uint32_t MurmurHash2 ( const void * key, int len, uint32_t seed )
while(len >= 4)
{
- uint32_t k = *(uint32_t*)data;
+ //uint32_t k = *(uint32_t*)data;
+ unsigned int k;
+ memcpy(&k, data, sizeof k);
k *= m;
k ^= k >> r;
@@ -100,12 +104,10 @@ uint64_t MurmurHash64A ( const void * key, int len, uint64_t seed )
uint64_t h = seed ^ (len * m);
- const uint64_t * data = (const uint64_t *)key;
- const uint64_t * end = data + (len/8);
-
- while(data != end)
- {
- uint64_t k = *data++;
+ const unsigned char * data = (const unsigned char *)key;
+ while (len >= 8) {
+ uint64_t k;
+ memcpy(&k, data, sizeof k);
k *= m;
k ^= k >> r;
@@ -113,19 +115,21 @@ uint64_t MurmurHash64A ( const void * key, int len, uint64_t seed )
h ^= k;
h *= m;
+ data += 8;
+ len -= 8;
}
const unsigned char * data2 = (const unsigned char*)data;
switch(len & 7)
{
- case 7: h ^= uint64_t(data2[6]) << 48;
- case 6: h ^= uint64_t(data2[5]) << 40;
- case 5: h ^= uint64_t(data2[4]) << 32;
- case 4: h ^= uint64_t(data2[3]) << 24;
- case 3: h ^= uint64_t(data2[2]) << 16;
- case 2: h ^= uint64_t(data2[1]) << 8;
- case 1: h ^= uint64_t(data2[0]);
+ case 7: h ^= uint64_t(data[6]) << 48;
+ case 6: h ^= uint64_t(data[5]) << 40;
+ case 5: h ^= uint64_t(data[4]) << 32;
+ case 4: h ^= uint64_t(data[3]) << 24;
+ case 3: h ^= uint64_t(data[2]) << 16;
+ case 2: h ^= uint64_t(data[1]) << 8;
+ case 1: h ^= uint64_t(data[0]);
h *= m;
};
Sadly, third_party/smhasher has no OWNERS file. It was added very long ago in http://codereview.chromium.org/7848010 . The README.chromium points to a Google Code project.
So the fix is pretty easy, but figuring out where to put it isn't.
,
Jun 12 2017
Looks like it lives in https://github.com/aappleby/smhasher now. aappleby, does comment 7 / 8 look acceptable for upstream smhasher? On x86 the compiler should be able to figure out that this is a no-op, and on arm it'd stop crashing.
,
Jun 13 2017
Alternatively: oysteine, do we care which hash function this code uses? If not, we could just use CityHash instead of MurmurHash, which already gets alignment right.
,
Jun 13 2017
If the problem here is just oysteine's CL I'd go for either #10 or stringcpy the string into an aligned buffer before.
,
Jun 13 2017
Nothing else calls MurmurHash() from what I can tell.
,
Jun 13 2017
No particular preferences for which hash function to use, as long as it avoids the collision issues that base::Hash have. CityHash seems like it would work fine, I'll make the change.
,
Jun 13 2017
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4daac667c2f27ed1a594c99bf69963014d87ce61 commit 4daac667c2f27ed1a594c99bf69963014d87ce61 Author: Oystein Eftevaag <oysteine@chromium.org> Date: Tue Jun 13 19:44:24 2017 GRC: Use CityHash instead of MurmurHash2 The implementation of the latter in smhasher doesn't deal with alignment well. R=primiano@chromium.org,thakis@chromium.org BUG= 732061 Change-Id: I2d516bd2082d773477707281357d9b0bba7477fb Reviewed-on: https://chromium-review.googlesource.com/533697 Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Commit-Queue: Oystein Eftevaag <oysteine@chromium.org> Cr-Commit-Position: refs/heads/master@{#479103} [modify] https://crrev.com/4daac667c2f27ed1a594c99bf69963014d87ce61/services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc [modify] https://crrev.com/4daac667c2f27ed1a594c99bf69963014d87ce61/services/resource_coordinator/public/cpp/BUILD.gn [modify] https://crrev.com/4daac667c2f27ed1a594c99bf69963014d87ce61/services/resource_coordinator/public/cpp/coordination_unit_id.cc
,
Jun 13 2017
\o/ |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Jun 10 2017