COMPILE_ASSERT in CSSStyleSelector.cpp doesn't compile on Windows

• Component: CSS
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Kenneth Russell]

Committed r102621: <http://trac.webkit.org/changeset/102621>

[Kenneth Russell]

Representative build failure:
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/17574/steps/compile/logs/stdio

[Ojan Vafai]

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.

[Ryosuke Niwa]

For RenderObject, it's 32bytes vs. 24bytes (33% increase!)

[Ryosuke Niwa]

Created attachment 118931 [details]
work in progress

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Early Warning System Bot]

Comment on attachment 118931 [details]
work in progress

Attachment 118931 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10846161

[Ryosuke Niwa]

Created attachment 118933 [details]
fixes the bug

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 118933 [details]
fixes the bug

Attachment 118933 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10847188

[WebKit Review Bot]

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

[Ryosuke Niwa]

Created attachment 118935 [details]
Actually add BitfieldsType.h

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Darin Adler]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 119053 [details]
Patch

[Eric Seidel (no email)]

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.

[Marc-Antoine Ruel]

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...

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Ping reviewers.

[Eric Seidel (no email)]

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?

[Ryosuke Niwa]

(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 :)

[Luke Macpherson]

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?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Comment on attachment 119053 [details]
Patch

Clearing flags on attachment: 119053

Committed r102835: <http://trac.webkit.org/changeset/102835>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.