[webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types

[Ryosuke Niwa]

Hi all,

I'm going to resurrect this thread from 2012 and suggest that we introduce
a new coding style guide as it came up in https://webkit.org/b/216069.

It looks like we added the rule to only use signed or unsigned as the
underlying type of bit fields to our style checker in
https://trac.webkit.org/r87771 without much discussion on webkit-dev. The
problem of this rule is that the type of bit fields determine the size of
the padding in most compilers such as GCC and clang. In the following
example, sizeof(U) is 4 but sizeof(C) is 1.

struct U {
  unsigned x : 1;
};

struct C {
  unsigned char x : 1;
};

The rule I propose instead is as follows:

Consecutive bit fields must use the same type.

*Right*:
struct S {
    uint8_t count : 7;
    uint8_t valid : 1;
}

struct C {
    uint32_t foo : 30;
    uint32_t bar : 2;
}

*Wrong*:
struct S {
    uint8_t count : 7;
    bool valid : 1;
}

struct C {
    uint32_t foo : 30;
    uint8_t bar : 2;
}

- R. Niwa

On Thu, Mar 29, 2012 at 1:21 AM Ryosuke Niwa <rniwa at webkit.org> wrote:

> Hi,
>
> Unlike gcc and clang, MSVC pads each consecutive member variables of the
> same type in bitfields. e.g. if you have:
> struct AB {
>     unsigned m_1 : 31;
>     bool m_2 : 1;
> }
> then *MSVC pads m_1 and allocates sizeof(unsigned) * 2 for AB* whereas
> gcc and clang only allocate sizeof(unsigned) * 1 for AB.
>
> This is *not a compiler bug*. It's a spec. compliant behavior, and may in
> fact have a better run-time performance in some situations. However, for
> core objects like RenderObject and InlineBox, allocating extra 4-8 bytes
> per each object is prohibitory expensive.
>
> In such cases, please *use the same POD type for all bitfield member
> variables*. (Storage class classifiers and variable qualifiers seem to
> have no effect on how variables are packed; e.g. mutable, const, etc...).
> For example, MSVC will allocate sizeof(unsigned) * 1 for AB if we rewrite
> the above code as:
> struct AB {
>     unsigned m_1 : 31;
>     unsigned m_2 : 1;
> }
>
> When you're making this change, *be sure to audit all code that assigns a
> non-boolean value to m_2* because implicit type coercion into boolean is
> no longer in effect. For example,
>
> AB ab;
> ab.m_2 = 2;
> puts(ab.m_2 ? "true" : "false");
>
> will print "true" before the change and will print "false" after the
> change. An easy way to ensure you've audited all such code is to add
> getters and setters for all bitfield member variables or wrap them in a
> special structure or class as done in
> http://trac.webkit.org/changeset/103353 and
> http://trac.webkit.org/changeset/104254.
>
> Best regards,
> Ryosuke Niwa
> Software Engineer
> Google Inc.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20200903/48fad9bd/attachment.htm>