Bug 82586 – Add a compile assert for the size of RenderBlock

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

While it's not as critical as RenderObject or InlineBox, we still don't want to increase the size of RenderBlock unintentionally.

[Ryosuke Niwa]

Created attachment 134531 [details]
Initial attempt

[Ryosuke Niwa]

For FloatingObject and MarginInfo, we could either define asserts in some methods of RenderBlock or add public methods that return the size of those classes.

[Tony Chang]

Comment on attachment 134531 [details]
Initial attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review

> Source/WebCore/ChangeLog:9
> +        We can't add asserts for FloatingObject and MarginInfo because they're private to RenderBlock.

You can put the COMPILE_ASSERT in the constructor.  At least, that's what we do for RenderStyle::(Non)InheritedFlags.

[Eric Seidel (no email)]

Comment on attachment 134531 [details]
Initial attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review

> Source/WebCore/rendering/RenderBlock.cpp:79
> +COMPILE_ASSERT(sizeof(RenderBlock) == sizeof(SameSizeAsRenderBlock), RenderBlock_should_stay_small);

Can we use an absolute number instead?

[Tony Chang]

Comment on attachment 134531 [details]
Initial attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:79
>> +COMPILE_ASSERT(sizeof(RenderBlock) == sizeof(SameSizeAsRenderBlock), RenderBlock_should_stay_small);
> 
> Can we use an absolute number instead?

Pointers are different sizes on 32bit vs 64bit architectures.

[Eric Seidel (no email)]

Comment on attachment 134531 [details]
Initial attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review

>>> Source/WebCore/rendering/RenderBlock.cpp:79
>>> +COMPILE_ASSERT(sizeof(RenderBlock) == sizeof(SameSizeAsRenderBlock), RenderBlock_should_stay_small);
>> 
>> Can we use an absolute number instead?
> 
> Pointers are different sizes on 32bit vs 64bit architectures.

I see.  I'm more worried that the bitfields will be padded differently, but I guess that's tested by other COMPILE_ASSERTS anyway.

[Ojan Vafai]

Comment on attachment 134531 [details]
Initial attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review

> Source/WebCore/rendering/RenderBlock.cpp:76
> +    uint32_t m_bitfields : 32;

Do you need the : 32 here?

[Ryosuke Niwa]

Comment on attachment 134531 [details]
Initial attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:76
>> +    uint32_t m_bitfields : 32;
> 
> Do you need the : 32 here?

Oh yeah, that's redundant. Will fix.

[Ryosuke Niwa]

Created attachment 134631 [details]
Patch for landing

[Ojan Vafai]

Comment on attachment 134631 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=134631&action=review

> Source/WebCore/rendering/RenderBlock.cpp:85
> +    uint32_t bitfields : 8;

I'm not a huge fan of using bitfields in these SameSize* structs. One of the major benefits of these compile asserts is that they ensure we get the same size we expect with visual studio, which we know does weird things with bitfields. Maybe make this uint8_t?

> Source/WebCore/rendering/RenderBlock.cpp:91
> +    uint32_t bitfields : 16;

Ditto, use uint16_t?

[Ryosuke Niwa]

(In reply to comment #10)
> (From update of attachment 134631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134631&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:85
> > +    uint32_t bitfields : 8;
> 
> I'm not a huge fan of using bitfields in these SameSize* structs. One of the major benefits of these compile asserts is that they ensure we get the same size we expect with visual studio, which we know does weird things with bitfields. Maybe make this uint8_t?

I don't think that'll work. Some compilers will always align sequence of bitfields to be one word (4/8 bytes) even though they may not pad other POD types smaller than int. Also, most of compilers pad struct so that the total size of struct is divisible by 4/8 unless the total size is smaller than 4/8 bytes.

[WebKit Review Bot]

Comment on attachment 134631 [details]
Patch for landing

Clearing flags on attachment: 134631

Committed r112566: <http://trac.webkit.org/changeset/112566>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.