http://bugs.webkit.org/show_bug.cgi?id=47594 There's a problem on our toolchain: sbox-arm-none-linux-gnueabi-g++ (4.4.1) WTF_ARM_ARCH_VERSION = 7 events/message-channel-gc-4.html --> pass Window/window-postmessage-clone.html --> bus error crash -- (gdb) bt #0 0x3bd2fef8 in bool WebCore::CloneDeserializer::readLittleEndian<unsigned long long>(unsigned char const*&, unsigned char const*, unsigned long long&) () from ./libQtWebKit.so.4.9.0 #1 0x3bd2cc40 in bool WebCore::CloneDeserializer::readLittleEndian<unsigned long long>(unsigned long long&) () from ./libQtWebKit.so.4.9.0 #2 0x3bd28280 in WebCore::CloneDeserializer::read(double&) () from ./libQtWebKit.so.4.9.0 #3 0x3bd28cf0 in WebCore::CloneDeserializer::readTerminal() () from ./libQtWebKit.so.4.9.0 #4 0x3bd29ff0 in WebCore::CloneDeserializer::deserialize() () from ./libQtWebKit.so.4.9.0 #5 0x3bd27e1c in WebCore::CloneDeserializer::deserialize(JSC::ExecState*, JSC::JSGlobalObject*, WTF::Vector<unsigned char, 0u> const&) () from ./libQtWebKit.so.4.9.0 #6 0x3bd2aad0 in WebCore::SerializedScriptValue::deserialize(JSC::ExecState*, JSC::JSGlobalObject*) () from ./libQtWebKit.so.4.9.0 #7 0x3ba4fd34 in WebCore::jsMessageEventData(JSC::ExecState*, JSC::JSValue, JSC::Identifier const&) () from ./libQtWebKit.so.4.9.0 #8 0x3c95aee8 in cti_op_get_by_id_custom_stub () from ./libQtWebKit.so.4.9.0 #9 0x3c95aee8 in cti_op_get_by_id_custom_stub () from ./libQtWebKit.so.4.9.0
> There's a problem on our toolchain: > sbox-arm-none-linux-gnueabi-g++ (4.4.1) > WTF_ARM_ARCH_VERSION = 7 Alignment problem on ARMv7? It is strange. > Window/window-postmessage-clone.html --> bus error crash This test should be on the skipped list for Qt (missing eventSender.beginDragWithFiles). See the output of this test on ARMv7: @@ -1,3 +1,5 @@ +CONSOLE MESSAGE: line 163: TypeError: Result of expression 'eventSender.beginDragWithFiles' [undefined] is not a function. +FAIL: Timed out waiting for notifyDone to be called Tests that we clone object hierarchies PASS: 'postMessage(cyclicObject)' threw TypeError: Cannot post cyclic structures. PASS: 'postMessage(cyclicArray)' threw TypeError: Cannot post cyclic structures. @@ -26,9 +28,4 @@ PASS: eventData is 2009-02-13T23:31:30.000Z of type object PASS: eventData is [object Object] of type object PASS: eventData is [object Array](default toString threw RangeError: Maximum call stack size exceeded.) of type object -PASS: eventData is [object File] of type object -PASS: eventData is [object FileList] of type object -PASS: eventData is [object ImageData] of type object -PASS: eventData is [object ImageData] of type object -PASS: eventData is done of type string
(In reply to comment #1) > > There's a problem on our toolchain: > > sbox-arm-none-linux-gnueabi-g++ (4.4.1) > > WTF_ARM_ARCH_VERSION = 7 > > Alignment problem on ARMv7? It is strange. > > > Window/window-postmessage-clone.html --> bus error crash > > This test should be on the skipped list for Qt (missing eventSender.beginDragWithFiles). The crash occurs with the actual when opening the file with a browser.
> > > Window/window-postmessage-clone.html --> bus error crash > > > > This test should be on the skipped list for Qt (missing eventSender.beginDragWithFiles). > > The crash occurs with the actual when opening the file with a browser. Apparently ARMv7 shouldn't be able to do multi-word unaligned reads. If it does, your kernel probably does the fixups and restores the control. This is probably as slow as it sounds (kernel trap) and should be fixed in userland code. You can cat /proc/cpu/alignment to see the fixups. http://wiki.debian.org/ArmEabiFixes
> Apparently ARMv7 shouldn't be able to do multi-word unaligned reads. If it does, your kernel probably does the fixups and restores the control. This is probably as slow as it sounds (kernel trap) and should be fixed in userland code. That is true. I have just checked the disassembled code, and see a LDRD instruction which tries to access an odd address. This is definitely an alignment fault on every ARM arch. Well, I will design a better construct (probably a template) to handle this situation also. Btw, I think that using kernel trap to fix unaligned data access is better than a crash. ;)
Created attachment 72929 [details] Misaligned memory access in CloneDeserializer on all ARM arch. This will fix the missaligned memory access with GCC and RVCT on ARM. The MSCV fix is speculative.
Attachment 72929 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/StdLibExtras.h', u'WebCore/ChangeLog', u'WebCore/bindings/js/SerializedScriptValue.cpp']" exit_code: 1 JavaScriptCore/wtf/StdLibExtras.h:94: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:103: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:129: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:138: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:156: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:162: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 6 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 73345 [details] Misaligned memory access in CloneDeserializer on all ARM arch. Fix readTime at WebCore/plugins/PluginDatabase.cpp.
Attachment 73345 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/StdLibExtras.h', u'WebCore/ChangeLog', u'WebCore/bindings/js/SerializedScriptValue.cpp', u'WebCore/plugins/PluginDatabase.cpp']" exit_code: 1 JavaScriptCore/wtf/StdLibExtras.h:94: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:103: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:129: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:138: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:156: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/StdLibExtras.h:162: unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 6 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Just a thought, but it would probably be more optimal on all platforms to ensure that 16-bit data is naturally aligned. A one byte pad on unaligned strings is not likely to significantly increase the size of our serialized format. I don't really know this code, but looking through it looks like we could probably add a NOP pad in write(const Idendentifier&) adding this to force alignment conditionally as needed. Then, in readString, when we cast the pointer to a UChar* we could also assert that it is UChar aligned. Cleaning up the serialization in this fashion would seem like a better solution for ARM – better to actually make the data naturally aligned than work around it not being so.
(In reply to comment #9) > Just a thought, but it would probably be more optimal on all platforms to ensure that 16-bit data is naturally aligned. > > A one byte pad on unaligned strings is not likely to significantly increase the size of our serialized format. I don't really know this code, but looking through it looks like we could probably add a NOP pad in write(const Idendentifier&) adding this to force alignment conditionally as needed. > > Then, in readString, when we cast the pointer to a UChar* we could also assert that it is UChar aligned. Cleaning up the serialization in this fashion would seem like a better solution for ARM – better to actually make the data naturally aligned than work around it not being so. Strings don't need padding as they're using 16bit characters, it is various tokens that are only one byte in size, if we think it's worth bumping all symbols that aren't multiple of 2 bytes up to two byte boundaries we should do that as soon as possible (eg. before localstorage, etc start using this)
(In reply to comment #10) > if we think it's worth bumping all symbols that aren't multiple of 2 bytes up to two byte boundaries we should do that as soon as possible (eg. before localstorage, etc start using this) Well, it looks better, but 'long long' data should be aligned to word boundary (see LDRD instruction on ARM).
I think the safest solution to this problem is simply to make the clone serializer and deserialiser use the byte at a time copy methods for all archs that require alignment, in addition to being the incorrect endianess
(In reply to comment #12) Oliver, if I understand your last comment correctly, you won't change the structure of the container. You would like those arch. which needs aligned addressing to read/write the unaligned data as a byte array. If this is what you want, please review the patch. My patch reads/writes unaligned data as a byte array. Btw, addressing with a packed-structure is much faster than with memcpy if the length of the data is small (for example: an unaligned long long data). If the addressed data is long (like a string), the memcpy is much faster than the packed-structure.
936 Vector<UChar> alignedBuffer(length); 937 memcpy(alignedBuffer.data(), ptr, length * sizeof(UChar)); 938 str = UString::adopt(alignedBuffer); 931 if (isPointerTypeAlignmentOkay(reinterpret_cast<const UChar*>(ptr))) 932 str = UString(reinterpret_cast<const UChar*>(ptr), length); 933 else { 934 Vector<UChar> alignedBuffer(length); 935 memcpy(alignedBuffer.data(), ptr, length * sizeof(UChar)); 936 str = UString::adopt(alignedBuffer); 937 } 939938 #else why is this change necessary? Anyway, we need to allocate memory for the string. will the new code path be faster?
I think the ARM guards here are a bad choice, as there are non-ARM platforms that expect aligned reads (MIPS i think). Honestly I think the various ARM guards make the code unpleasant to read and think a better approach is simply to make all platforms with alignment constraints use the generic byte at a time model used for non-little endian platforms.
I believe the simplest solution to this problem is to make all architectures that have alignment requirements just take the bigendian path. That will result in a single localised change and won't result in ifdef's being strewn around everywhere.
Comment on attachment 73345 [details] Misaligned memory access in CloneDeserializer on all ARM arch. r- based on Comment #15 and Comment #16.
Created attachment 84330 [details] Patch
Comment on attachment 84330 [details] Patch r+, thanks for fixing this for ARM (having tested it).
Committed r80070: <http://trac.webkit.org/changeset/80070>