Bit packing on Windows and 'unsigned' bit fields

[Sasha Morrissey]

Hi all,

We've been using 'unsigned' as the type for the bitfields on ComputedStyle because Windows used to have this alignment issue where differently-typed adjacent bitfields would align to the closest word rather than pack tightly. MSVC’s packing algorithm is described in the Visual C++ documentation (see Padding and Alignment of Structure Members, Storage of Bit Fields, C++ Bit Fields, etc) and doesn't mention this behavior.

I've done some experimentation and seem to have found that this is no longer the case. See the code at the bottom of the this doc:

https://docs.google.com/document/d/1BfjMEXIzvY1WZEn_nESBxyqkazkfa0sBSEvm-F89YwM/edit#

which defines a packed struct with differently-typed enum bitfields that fits into the 4 bytes of allocated space.

Without this issue, I don't see any reason why we need to keep using unsigned's for the ComputedStyle bitfields, and can switch back to using the correct enum type, which is easier to develop and work with.

Sasha

P.S. In case you don't have a local compiler set up, ideone.com provides a cloud Linux compiler, and webcompiler.cloudapp.net provides a Visual C++ one.

[Timothy Loh]

I'm not sure this was actually due to packing issues. IIRC using enum types in bitfields on MSVC behaves differently to other compilers because it interprets the value as signed, see below code snippet which outputs "a1: 1, -1" according to http://rextester.com/l/cpp_online_compiler_visual.

#include <iostream>

using namespace std;

enum A { a0, a1 };

class S {

public:

    A a() const { return m_a; }

    void setA(A val) { m_a = val; }

private:

    A m_a : 1;

};

int main()

{

    S s;

    s.setA(a0);

    cout << "a0: " << a0 << ", " << s.a() << endl;

    s.setA(a1);

    cout << "a1: " << a1 << ", " << s.a() << endl;

}

--
You received this message because you are subscribed to the Google Groups "style-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to style-dev+...@chromium.org.
To post to this group, send email to styl...@chromium.org.
To view this discussion on the web, visit https://groups.google.com/a/chromium.org/d/msgid/style-dev/CADAgNurPp47TyFcws6LAyY%3DAxT5RpRb0MkAWJ_G3J2oL0Uj-mg%40mail.gmail.com.

[Sasha Morrissey]

This can be fixed by using enum classes to extend from an unsigned, rather than the default signed int:

#include <iostream>

using namespace std;

enum class A : unsigned int { a0, a1 };

class S {

public:

    int a() const { return (int)m_a; }

    void setA(A val) { m_a = val; }

private:

    A m_a : 1;

};

int main()

{

    S s;

    s.setA(A::a0);

    cout << "a0: " << (int)(A::a0) << ", " << s.a() << endl;

    s.setA(A::a1);

    cout << "a1: " << (int)(A::a1) << ", " << s.a() << endl;

}

The above prints "a1: 1, 1" with both MSVC and clang.

So maybe the correct way forward is to change all enums to enum classes that extend from an unsigned type.

[Elliott Sprehn]

That seems legit but a bit brittle, make sure we have static_asserts for the size of the objects that embed these types before we do the switch. :)

To view this discussion on the web, visit https://groups.google.com/a/chromium.org/d/msgid/style-dev/CADAgNuqkM3dp4w7kEuHV%3DDOD2rCA0uj8NPS5%3D5ndRaxsDWVE4g%40mail.gmail.com.

[Sasha Morrissey]

Maybe later we can write a Clang plugin or presubmit check to do this automatically. For now, leaving them as unsigned seems safe.

FYI, here's a patch I had in progress for adding a TypeTraits template for use in a static assert for whether an enum class is unsigned:

https://codereview.chromium.org/2200053002

You could also do this using a macro, but that would look ugly/make codesearch hard.

Since we're moving towards generating ComputedStyle anyway, and that seems like the main place where we use enum bitfields, this may not matter for now and we can re-investigate later. :)