RenderArena::allocate falls back to using malloc when compiled for DEBUG. In addition, it introduces a debug header called RenderArenaDebugHeader which is prefixed for every allocated cell. The pointer arithmetics in RenderArena::allocate (http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderArena.cpp#L73) lead to returning a possibly misaligned pointer - depending on the size of RenderArenaDebugHeader and the data types that are placed into the allocated cell. For example, in HTMLInputElement::createRenderer a new text control renderer is created ( "return new (arena) RenderTextControl(this, false);" )- this RenderTextControl will contain a Timer, which will in turn have a TimerBase which has members of type double. Accessing those doubles on XScale when the initial base address returned by the Arena allocator was not 8byte aligned leads to a misaligned access, causing a crash.
Created attachment 55726 [details] Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators.
Darin, can you have a look?
Comment on attachment 55726 [details] Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators. > +#ifdef WTF_ARMV5_NEEDS_ALIGN_8 > +#define ARENA_ALIGN_MASK 7 > +#else > #define ARENA_ALIGN_MASK 3 > +#endif I'd like to see us do in a way that's a bit more automatic. One way is: #define ARENA_ALIGN_MASK (sizeof(AllocAlignmentInteger) - 1) All that ARM-specific stuff seems undesirable. In fact, other 64-bit platforms would probably benefit from 64-bit-aligned objects in the arenas. > +#define ARENA_ALIGNED_HEADER_SIZE ((sizeof(RenderArenaDebugHeader) + ARENA_ALIGN_MASK) & ~ARENA_ALIGN_MASK) We should use the ARENA_ALIGN macro here. We can change it so it doesn't take a "pool" argument and then use it like this: ARENA_ALIGN(sizeof(RenderArenaDebugHeader)) Does this need to be a macro? This is C++ so I'd think we could just use "static const" instead. > - RenderArenaDebugHeader* header = static_cast<RenderArenaDebugHeader*>(ptr) - 1; > + void* block = static_cast<unsigned char*>(ptr) - ARENA_ALIGNED_HEADER_SIZE; Using "unsigned char*" here instead of "char*" is a bit strange. Change seems OK, but I think it would be better to do this without so much ARM-specific. review- for now Please let me know if I misunderstood the basis of the need for this on ARM. Is it needed on 32-bit ARM?
Yes we do require this on 32 bit ARMv5TE. ARMv5TE has double word instructions LDRD and STRD that load 64bits in a single instruction; but the requirement is that the dest/src need to be 8 byte aligned. The problem is that we are running allocators from pools that are 4 byte aligned. This is all well and good for on most platforms that can do double word loads from 4 byte addresses; but that CPU can't. I agree that this would be good on all platforms to move to 8 byte alignment on objects to move them more inline with what a malloc would provide; but didn't want to influence all other platforms for this one CPU case. I'll prepare a patch as per your comments.
(In reply to comment #4) > Yes we do require this on 32 bit ARMv5TE. I think you may be right in not wanting to affect the other platforms, but Platform.h should be indicating that the alignment is needed in a way that is not ARM-specific, so not "WTF_ARMV5_NEEDS_ALIGN_8".
Created attachment 56069 [details] Generalize last patch removing ARM specific stuff.
Attachment 56069 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/Arena.h:45: Alphabetical sorting problem. [build/include_order] [4] WebCore/platform/Arena.h:83: _nb is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/Arena.h:94: _incr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 56070 [details] Add alignment based on AllocAlignmentInteger for Arenas.
Attachment 56070 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/Arena.h:83: _nb is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/Arena.h:94: _incr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 56070 [details] Add alignment based on AllocAlignmentInteger for Arenas. > +#include <wtf/FastMalloc.h> > +#include <wtf/Platform.h> Since FastMalloc.h already includes Platform.h, there is no need to include Platform.h here. > +#ifdef WTF_USE_ARENA_ALLOC_ALIGNMENT_INTEGER > +#define ARENA_ALIGN_MASK (sizeof(WTF::AllocAlignmentInteger) - 1) > +#else > #define ARENA_ALIGN_MASK 3 > +#endif I think it would be good eventually to get rid of WTF_USE_ARENA_ALLOC_ALIGNMENT_INTEGER and always do this. A FIXME mentioning that would be a plus. r=me
Created attachment 56079 [details] Add alignment based on AllocAlignmentInteger for Arenas
Attachment 56079 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/Arena.h:84: _nb is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/Arena.h:95: _incr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 56079 [details] Add alignment based on AllocAlignmentInteger for Arenas Clearing flags on attachment: 56079 Committed r59525: <http://trac.webkit.org/changeset/59525>
All reviewed patches have been landed. Closing bug.