Bug 82586 - Add a compile assert for the size of RenderBlock
Summary: Add a compile assert for the size of RenderBlock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-29 02:05 PDT by Ryosuke Niwa
Modified: 2012-03-29 13:20 PDT (History)
9 users (show)

See Also:


Attachments
Initial attempt (11.77 KB, patch)
2012-03-29 02:30 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (12.28 KB, patch)
2012-03-29 11:50 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.