Use a Variant instead of a union in CSSSelector
Created attachment 347086 [details] Patch
Created attachment 347092 [details] Patch
Comment on attachment 347092 [details] Patch Nice, r=me At some point we'll need to rewrite these classes completely. We need separate types for Compound/Complex/SimpleSelector instead of the current mess where everything is a CSSSelector.
I also updated the indentation of CSSSelector.h. http://trac.webkit.org/r234859
<rdar://problem/43295037>
This broke WinCairo and AppleWin builds. :( > Source\WebCore\css/CSSSelector.cpp(44): error C2338: CSSSelector should remain small. (from https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Build%29/builds/2478/steps/compile-webkit/logs/stdio)
What is sizeof(CSSSelector) on Windows (64- or 32-bit)?
(In reply to Alex Christensen from comment #7) > What is sizeof(CSSSelector) on Windows (64- or 32-bit)? On Win64 and Mac64 alike, I'm seeing 16 for the struct and 12 for the sum (8 + 4). If I change the static_assert to `== 16`, the compilation proceeds, but we still get this warning quartet over and over: > Source\WebCore\css\CSSSelector.h(363): warning C4315: 'WebCore::CSSSelector::RareData': 'this' pointer for member 'WebCore::CSSSelector::RareData::m_attribute' may not be aligned 8 as expected by the constructor > Source\WebCore\css\CSSSelector.h(365): warning C4315: 'WebCore::CSSSelector::RareData': 'this' pointer for member 'WebCore::CSSSelector::RareData::m_argument' may not be aligned 8 as expected by the constructor > Source\WebCore\css\CSSSelector.h(376): warning C4315: 'WebCore::CSSSelector::NameWithCase': 'this' pointer for member 'WebCore::CSSSelector::NameWithCase::m_originalName' may not be aligned 8 as expected by the constructor > Source\WebCore\css\CSSSelector.h(376): warning C4315: 'WebCore::CSSSelector::NameWithCase': 'this' pointer for member 'WebCore::CSSSelector::NameWithCase::m_lowercaseLocalName' may not be aligned 8 as expected by the constructor
(In reply to Ross Kirsling from comment #8) > (In reply to Alex Christensen from comment #7) > > What is sizeof(CSSSelector) on Windows (64- or 32-bit)? > > On Win64 and Mac64 alike, I'm seeing 16 for the struct and 12 for the sum (8 > + 4). > > If I change the static_assert to `== 16`, the compilation proceeds, but we > still get this warning quartet over and over: > > > Source\WebCore\css\CSSSelector.h(363): warning C4315: 'WebCore::CSSSelector::RareData': 'this' pointer for member 'WebCore::CSSSelector::RareData::m_attribute' may not be aligned 8 as expected by the constructor > > Source\WebCore\css\CSSSelector.h(365): warning C4315: 'WebCore::CSSSelector::RareData': 'this' pointer for member 'WebCore::CSSSelector::RareData::m_argument' may not be aligned 8 as expected by the constructor > > Source\WebCore\css\CSSSelector.h(376): warning C4315: 'WebCore::CSSSelector::NameWithCase': 'this' pointer for member 'WebCore::CSSSelector::NameWithCase::m_originalName' may not be aligned 8 as expected by the constructor > > Source\WebCore\css\CSSSelector.h(376): warning C4315: 'WebCore::CSSSelector::NameWithCase': 'this' pointer for member 'WebCore::CSSSelector::NameWithCase::m_lowercaseLocalName' may not be aligned 8 as expected by the constructor Oops, spoke too soon. Still encountered the error anyway...
Feel free to roll it out while you look for a fix. sizeof(CSSSelector) is definitely 12 for 64-bit Mac and linux.
(In reply to Alex Christensen from comment #10) > Feel free to roll it out while you look for a fix. sizeof(CSSSelector) is > definitely 12 for 64-bit Mac and linux. Alrighty. I hadn't realized right away that sizeof(SameSizeAsCSSSelector) was made irrelevant here, but it looks like sizeof(CSSSelector) is 13 on Windows.
Reverted r234859 for reason: Windows ports can't compile Committed r234877: <https://trac.webkit.org/changeset/234877>
Fujii or Ross, could you get this working on Windows 32-bit and 64-bit? I think it's just a matter of tweaking a few small things, but iterating from EWS is waaay too slow.
Created attachment 347150 [details] Patch It seems that MSVC doesn't pack char (type of Variant) and unsigned int (the flags). One possible workaroud is using char for the flags.
Comment on attachment 347150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347150&action=review > Source/WTF/ChangeLog:9 > + Add packing macros to make it so Variant-containing structures don't always have 7 bytes of padding per Variant. This makes subsequent members after Variant in classes unaligned. Unaligned access is a undefined behavior.
Created attachment 347151 [details] Patch * Use unsigned char for the flags * Reoder flags in order to align 8 bits
(In reply to Fujii Hironori from comment #16) > Created attachment 347151 [details] > Patch > > * Use unsigned char for the flags > * Reoder flags in order to align 8 bits Locally, it seems that: - "unsigned" -> "unsigned char" makes the assert pass on Windows - reordering fields doesn't help us at all(?) - neither of these steps makes the warnings go away - Xcode wants the fields to be initialized in the same order that they are declared
Any luck getting this to work on Windows? I'd still like to land it.
What about an idea adding a new class WTF::CompactVariant<Traits, ...> which uses a template traits to get the position of m_type member variable?
That seems quite excessive, and it wouldn't benefit all existing users of Variant.
Created attachment 349620 [details] Patch * Rebased onto ToT * Removed pragma(pack) of CSSSelector to solve the warning quartet * Try to solve "field 'm_pseudoType' will be initialized after field 'm_isLastInSelectorList'" on Mac ports
Comment on attachment 349620 [details] Patch Clearing flags on attachment: 349620 Committed r235976: <https://trac.webkit.org/changeset/235976>
All reviewed patches have been landed. Closing bug.
Rolled out in http://trac.webkit.org/r236284 We need to do something about alignment...
(In reply to Alex Christensen from comment #24) > Rolled out in http://trac.webkit.org/r236284 > We need to do something about alignment... Note that a bug was opened about this issue https://bugs.webkit.org/show_bug.cgi?id=189758