Bug 19946 – Possible misalignment in RenderArena when compiled for debug

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dominik Röttsches (drott)]

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.

[David Tapuska]

Created attachment 55726 [details]
Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators.

[Adam Treat]

Darin, can you have a look?

[Darin Adler]

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?

[David Tapuska]

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.

[Darin Adler]

(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".

[David Tapuska]

Created attachment 56069 [details]
Generalize last patch removing ARM specific stuff.

[WebKit Review Bot]

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.

[David Tapuska]

Created attachment 56070 [details]
Add alignment based on AllocAlignmentInteger for Arenas.

[WebKit Review Bot]

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.

[Darin Adler]

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

[David Tapuska]

Created attachment 56079 [details]
Add alignment based on AllocAlignmentInteger for Arenas

[WebKit Review Bot]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.