Bug 19946 - Possible misalignment in RenderArena when compiled for debug
Summary: Possible misalignment in RenderArena when compiled for debug
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-08 09:09 PDT by Dominik Röttsches (drott)
Modified: 2010-05-15 01:25 PDT (History)
7 users (show)

See Also:


Attachments
Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators. (5.41 KB, patch)
2010-05-11 11:24 PDT, David Tapuska
darin: review-
Details | Formatted Diff | Diff
Generalize last patch removing ARM specific stuff. (8.47 KB, patch)
2010-05-14 07:39 PDT, David Tapuska
no flags Details | Formatted Diff | Diff
Add alignment based on AllocAlignmentInteger for Arenas. (8.47 KB, patch)
2010-05-14 07:55 PDT, David Tapuska
darin: review+
Details | Formatted Diff | Diff
Add alignment based on AllocAlignmentInteger for Arenas (8.58 KB, patch)
2010-05-14 09:19 PDT, David Tapuska
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2008-07-08 09:09:25 PDT
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.
Comment 1 David Tapuska 2010-05-11 11:24:46 PDT
Created attachment 55726 [details]
Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators.
Comment 2 Adam Treat 2010-05-11 11:51:24 PDT
Darin, can you have a look?
Comment 3 Darin Adler 2010-05-11 17:09:43 PDT
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?
Comment 4 David Tapuska 2010-05-11 17:34:11 PDT
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.
Comment 5 Darin Adler 2010-05-11 17:37:46 PDT
(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".
Comment 6 David Tapuska 2010-05-14 07:39:50 PDT
Created attachment 56069 [details]
Generalize last patch removing ARM specific stuff.
Comment 7 WebKit Review Bot 2010-05-14 07:43:30 PDT
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.
Comment 8 David Tapuska 2010-05-14 07:55:12 PDT
Created attachment 56070 [details]
Add alignment based on AllocAlignmentInteger for Arenas.
Comment 9 WebKit Review Bot 2010-05-14 08:01:36 PDT
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 10 Darin Adler 2010-05-14 09:02:36 PDT
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
Comment 11 David Tapuska 2010-05-14 09:19:32 PDT
Created attachment 56079 [details]
Add alignment based on AllocAlignmentInteger for Arenas
Comment 12 WebKit Review Bot 2010-05-14 09:20:31 PDT
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 13 WebKit Commit Bot 2010-05-15 01:25:26 PDT
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>
Comment 14 WebKit Commit Bot 2010-05-15 01:25:32 PDT
All reviewed patches have been landed.  Closing bug.