Bug 74327 - COMPILE_ASSERT in CSSStyleSelector.cpp doesn't compile on Windows
Summary: COMPILE_ASSERT in CSSStyleSelector.cpp doesn't compile on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 74314
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 13:59 PST by Kenneth Russell
Modified: 2011-12-14 15:21 PST (History)
11 users (show)

See Also:


Attachments
work in progress (9.96 KB, patch)
2011-12-12 18:40 PST, Ryosuke Niwa
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
fixes the bug (7.89 KB, patch)
2011-12-12 19:14 PST, Ryosuke Niwa
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Actually add BitfieldsType.h (10.07 KB, patch)
2011-12-12 19:34 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (3.09 KB, patch)
2011-12-13 11:40 PST, 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 Kenneth Russell 2011-12-12 13:59:23 PST
The COMPILE_ASSERT added to CSSStyleSelector.cpp in http://trac.webkit.org/changeset/102613 doesn't compile on Windows. It looks like the patch is a good one to have so patching this up for the moment.
Comment 1 Kenneth Russell 2011-12-12 14:10:00 PST
Committed r102621: <http://trac.webkit.org/changeset/102621>
Comment 3 Ojan Vafai 2011-12-12 15:59:50 PST
It looks like MSVC packs bitfields with mixed types really poorly: http://randomascii.wordpress.com/2010/06/06/bit-field-packing-with-visual-c/

Ryosuke tested this locally and sizeof(RuleData) is 44 on windows and 32 on linux/mac! Apparently just changing bool to unsigned fixes it. 

We should probably add COMPILE_ASSERTs to all classes that have bitfields and fix this across the board.
Comment 4 Ryosuke Niwa 2011-12-12 16:06:14 PST
For RenderObject, it's 32bytes vs. 24bytes (33% increase!)
Comment 5 Ryosuke Niwa 2011-12-12 18:40:56 PST
Created attachment 118931 [details]
work in progress
Comment 6 WebKit Review Bot 2011-12-12 18:44:17 PST
Attachment 118931 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/JavaScriptCore.vcpro..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:110:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 WebKit Review Bot 2011-12-12 18:47:51 PST
Comment on attachment 118931 [details]
work in progress

Attachment 118931 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10846160
Comment 8 Early Warning System Bot 2011-12-12 18:48:46 PST
Comment on attachment 118931 [details]
work in progress

Attachment 118931 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10846161
Comment 9 Ryosuke Niwa 2011-12-12 19:14:56 PST
Created attachment 118933 [details]
fixes the bug
Comment 10 WebKit Review Bot 2011-12-12 19:16:50 PST
Attachment 118933 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:110:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Early Warning System Bot 2011-12-12 19:18:01 PST
Comment on attachment 118933 [details]
fixes the bug

Attachment 118933 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10847188
Comment 12 WebKit Review Bot 2011-12-12 19:27:49 PST
Comment on attachment 118933 [details]
fixes the bug

Attachment 118933 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10848202
Comment 13 Ryosuke Niwa 2011-12-12 19:34:31 PST
Created attachment 118935 [details]
Actually add BitfieldsType.h
Comment 14 WebKit Review Bot 2011-12-12 19:37:30 PST
Attachment 118935 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/BitfieldTypes.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Source/WebCore/css/CSSStyleSelector.cpp:110:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 49 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Darin Adler 2011-12-13 09:25:43 PST
Comment on attachment 118935 [details]
Actually add BitfieldsType.h

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

> Source/JavaScriptCore/GNUmakefile.list.am:508
>  	Source/JavaScriptCore/wtf/AVLTree.h \
> +        Source/JavaScriptCore/wtf/BitfieldTypes.h \

Looks like a tabs vs. spaces mistake.

>> Source/JavaScriptCore/wtf/BitfieldTypes.h:1
>> +/*
> 
> One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]

Best way to avoid this is to set a svn:eol-style property on new files.

> Source/JavaScriptCore/wtf/BitfieldTypes.h:40
> +#if !COMPILER(MSVC)
> +typedef bool bitfieldBool;
> +#else
> +typedef unsigned bitfieldBool;
> +#endif

This is a dangerous practice. If we have an expression like this:

    m_isLoading = flags & IsLoadingFlag;

And IsLoadingFlag is 2 (or 1 << 1 or whatever), then on MSVC the code will set m_isLoading to false, and on other compilers the code will set m_isLoading to true. Putting this difference behind a typedef makes things worse, I think, not better.

I don’t think a typedef is a good idea. Instead we should just use the types signed and unsigned exclusively for bitfields on all compilers, and then audit the code to make sure it never relies on the bitfield being boolean at the time we change it from bool to unsigned. We can’t change from bool to unsigned all at once without auditing the code.
Comment 16 Ryosuke Niwa 2011-12-13 10:06:01 PST
(In reply to comment #15)
> > Source/JavaScriptCore/wtf/BitfieldTypes.h:40
> > +#if !COMPILER(MSVC)
> > +typedef bool bitfieldBool;
> > +#else
> > +typedef unsigned bitfieldBool;
> > +#endif
> 
> This is a dangerous practice. If we have an expression like this:
> 
>     m_isLoading = flags & IsLoadingFlag;
> 
> And IsLoadingFlag is 2 (or 1 << 1 or whatever), then on MSVC the code will set m_isLoading to false, and on other compilers the code will set m_isLoading to true. Putting this difference behind a typedef makes things worse, I think, not better.

That's a good point. There was a concern about making everything unsigned but I suppose these side-effects are harder to notice with typedef.
Comment 17 Darin Adler 2011-12-13 11:19:37 PST
(In reply to comment #16)
> (In reply to comment #15)
> > > Source/JavaScriptCore/wtf/BitfieldTypes.h:40
> > > +#if !COMPILER(MSVC)
> > > +typedef bool bitfieldBool;
> > > +#else
> > > +typedef unsigned bitfieldBool;
> > > +#endif
> > 
> > This is a dangerous practice. If we have an expression like this:
> > 
> >     m_isLoading = flags & IsLoadingFlag;
> > 
> > And IsLoadingFlag is 2 (or 1 << 1 or whatever), then on MSVC the code will set m_isLoading to false, and on other compilers the code will set m_isLoading to true. Putting this difference behind a typedef makes things worse, I think, not better.
> 
> That's a good point. There was a concern about making everything unsigned but I suppose these side-effects are harder to notice with typedef.

I suppose we could use a typedef if we want something to say it’s a bool, but to make maintenance acceptable I think it should be a typedef to unsigned on all platforms, not just MSVC.
Comment 18 Ryosuke Niwa 2011-12-13 11:25:59 PST
(In reply to comment #17)
> (In reply to comment #16)
> > That's a good point. There was a concern about making everything unsigned but I suppose these side-effects are harder to notice with typedef.
> 
> I suppose we could use a typedef if we want something to say it’s a bool, but to make maintenance acceptable I think it should be a typedef to unsigned on all platforms, not just MSVC.

I've thought about it but I think it'll make it easier to make the bit mask mistake you pointed out.
Comment 19 Ryosuke Niwa 2011-12-13 11:40:45 PST
Created attachment 119053 [details]
Patch
Comment 20 Eric Seidel (no email) 2011-12-13 15:40:08 PST
Could we use our contacts in the MSVC team to see if there is a way to disable this behavior?  Although both compilers may be compliant with the (loose) spec, it seems this inconsistency must be hitting other developers as well.
Comment 21 Marc-Antoine Ruel 2011-12-13 18:37:24 PST
Eric, I wouldn't have high hopes with that. Even if you'd win and it gets into VS2012, webkit wouldn't be able to use mix-and-match bitfield types until vs2010 is deprecated, probably somewhere around 2016. So unless you are in a hurry...
Comment 22 Ryosuke Niwa 2011-12-13 19:05:05 PST
(In reply to comment #21)
> Eric, I wouldn't have high hopes with that. Even if you'd win and it gets into VS2012, webkit wouldn't be able to use mix-and-match bitfield types until vs2010 is deprecated, probably somewhere around 2016. So unless you are in a hurry...

I think we should definitely do it. 5 years is infinitely better than never.
Comment 23 Ryosuke Niwa 2011-12-13 19:36:18 PST
Note that using #pragma pack(1) reduces the size of RenderObject from 32 bytes to 29 bytes, but still suboptimal compared to gcc/clang's 24 bytes. Also, since 29 byte objects don't align, we'll probably end up consuming 32 bytes anyway.
Comment 24 Ryosuke Niwa 2011-12-14 13:14:12 PST
Ping reviewers.
Comment 25 Eric Seidel (no email) 2011-12-14 13:40:12 PST
Comment on attachment 119053 [details]
Patch

You mentioned in person yesterday that there were gotchas associated with having these be non-bools.  Can you explain those over the bug, so I can better understand how to weight this approach vs. using a class with typed accessors to hold all these fields?
Comment 26 Ryosuke Niwa 2011-12-14 13:44:31 PST
(In reply to comment #25)
> (From update of attachment 119053 [details])
> You mentioned in person yesterday that there were gotchas associated with having these be non-bools.  Can you explain those over the bug, so I can better understand how to weight this approach vs. using a class with typed accessors to hold all these fields?

Gotchas is explained in comment #15 by Darin. Basically implicit casting to bool doesn't happen anymore when we assign another integral variable. e.g.

m_bitfield = 1 + 1 // used to be true but now false
m_bitfield = number // used to be true iff number is not zero but now true iff number % 2 is 1.

I'm fine with either approach. I just want to push this forward so we can reduce the memory footprint as soon as we can :)
Comment 27 Luke Macpherson 2011-12-14 14:01:26 PST
Random, possibly undesirable idea - why not use gcc on windows if you want tight bit packing, and not bother asserting if using a compiler that decides to unpack things for you?
Comment 28 Ryosuke Niwa 2011-12-14 14:07:29 PST
(In reply to comment #27)
> Random, possibly undesirable idea - why not use gcc on windows if you want tight bit packing, and not bother asserting if using a compiler that decides to unpack things for you?

That's just impractical in so many ways.
Comment 29 Ryosuke Niwa 2011-12-14 15:21:38 PST
Comment on attachment 119053 [details]
Patch

Clearing flags on attachment: 119053

Committed r102835: <http://trac.webkit.org/changeset/102835>
Comment 30 Ryosuke Niwa 2011-12-14 15:21:44 PST
All reviewed patches have been landed.  Closing bug.