Bug 82586

Summary: Add a compile assert for the size of RenderBlock
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, kling, koivisto, morrita, ojan, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Initial attempt
none
Patch for landing none

Description Ryosuke Niwa 2012-03-29 02:05:18 PDT
While it's not as critical as RenderObject or InlineBox, we still don't want to increase the size of RenderBlock unintentionally.
Comment 1 Ryosuke Niwa 2012-03-29 02:30:03 PDT
Created attachment 134531 [details]
Initial attempt
Comment 2 Ryosuke Niwa 2012-03-29 02:33:31 PDT
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.
Comment 3 Tony Chang 2012-03-29 09:51:45 PDT
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.
Comment 4 Eric Seidel (no email) 2012-03-29 10:46:06 PDT
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?
Comment 5 Tony Chang 2012-03-29 10:49:32 PDT
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.
Comment 6 Eric Seidel (no email) 2012-03-29 10:53:09 PDT
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.
Comment 7 Ojan Vafai 2012-03-29 10:58:51 PDT
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?
Comment 8 Ryosuke Niwa 2012-03-29 11:48:54 PDT
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.
Comment 9 Ryosuke Niwa 2012-03-29 11:50:14 PDT
Created attachment 134631 [details]
Patch for landing
Comment 10 Ojan Vafai 2012-03-29 11:58:30 PDT
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?
Comment 11 Ryosuke Niwa 2012-03-29 12:28:10 PDT
(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.
Comment 12 WebKit Review Bot 2012-03-29 13:20:31 PDT
Comment on attachment 134631 [details]
Patch for landing

Clearing flags on attachment: 134631

Committed r112566: <http://trac.webkit.org/changeset/112566>
Comment 13 WebKit Review Bot 2012-03-29 13:20:45 PDT
All reviewed patches have been landed.  Closing bug.