[webkit-reviews] review denied: [Bug 19946] Possible misalignment in RenderArena when compiled for debug : [Attachment 55726] Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Tapuska <dtapuska at rim.com>'s
request for review:
Bug 19946: Possible misalignment in RenderArena when compiled for debug
https://bugs.webkit.org/show_bug.cgi?id=19946

Attachment 55726: Apply appropriate defines for ARMv5TE architectures so we can
force 8 byte alignment in object allocators.
https://bugs.webkit.org/attachment.cgi?id=55726&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +#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?