Bug 54807 – Always explicitly use "signed" keyword to declare signed bitfields

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yong Li]

struct {
  int a : 31;
  bool b : 1;
};

Many compilers will treat "a" as a signed integer. However, according to standards, making "a" signed or unsigned relies on the implementation of the compiler. We have known armcc treats "a" as unsigned integer. (See Bug 54687). This difference can cause unexpected runtime error. For example:

a = -1;
if (a == -1)
  printf("a is signed");
else
  printf("a is unsigned"); // -1 will be converted to unsigned number first => 0xFFFFFFFF, and "a" is 0x7FFFFFFF, so they are different.

To make sure our code has same behavior for all compilers and avoid hard-to-locate bugs caused by this, we should always explicitly use "signed" keywords when we want to use signed bitfields (and of course, use "unsigned" keyword in the other case), given that the change will be no-op for most compilers.

[Antonio Gomes]

Even after you change them all, it would be good to have a way to avoid it to happen in the future.

Maybe we should teach the stylebot to check this.

[Eric Seidel (no email)]

I can see it making sense to have check-webkit-style warn about needing explicit signed/unsigned for bitfields since we know that at least the MSVC compiler gets enum bitfields "wrong" (makes them signed when other compilers leave them unsigned).  Would be nice to have a URL linking to the armcc bug (assuming we've decided it's a bug) about armcc treating "int" as unsigned when used in a bitfield.

[Eric Seidel (no email)]

(I CC'd Darin just because he would remember the days of the MSVC enum confusion and might have further opinions here.)

[Alexey Proskuryakov]

But a "signed int" type doesn't exist in C++, does it? See the spec, paragraph 3.9.1.

[Daniel Bates]

(In reply to comment #4)
> But a "signed int" type doesn't exist in C++, does it? See the spec, paragraph 3.9.1.

Notice, section 3.9.1 of <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf> describes the fundamental types of the C++ language and "signed" is a simple-type-specifier (see section 7.1.6.2). In describing 7.1.6 (2) it's mentioned that "signed" can be combined with the fundamental type "int".

For completeness, this specifier as well as the valid combinations of simple-type-specifiers is summarized in Table 9.

[Alexey Proskuryakov]

Daniel, I'm not entirely sure if you intended to support my comment or to object to it. Either way, there is no "signed int" in the "type" column of table 9.

It's OK to specify a type as "signed int", but that's in no way different from "int", or from "MyInt" defined as "typedef int MyInt".

[Daniel Bates]

(In reply to comment #6)
> [...]
> It's OK to specify a type as "signed int", but that's in no way different from "int", or from "MyInt" defined as "typedef int MyInt".

Can you elaborate on your original question in comment 4? In particular, I am unclear what you are implying by your question?  Are you implying that that it's redundant to use "signed" with regards to a bit field?

[Alexey Proskuryakov]

Yes, it's redundant and against WebKit coding style.

[Daniel Bates]

(In reply to comment #8)
> Yes, it's redundant and against WebKit coding style.

Notice, "signed" has a special meaning with respect to bit fields. This is described in the "Note" in 7.1.6.2 (3) of the C++ standard <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf> describes the use of "signed" with respect to bit fields.

[Daniel Bates]

(In reply to comment #9)
> (In reply to comment #8)
> [...] This is described in the "Note" in 7.1.6.2 (3) of the C++ standard <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf> describes the use of "signed" with respect to bit fields.

I meant to write:

This is described in the "Note" in 7.1.6.2 (3) of the C++ standard <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf>.

[Alexey Proskuryakov]

I didn't know about this exception (it's well hidden, and I wonder if it even makes the spec self-contradictory!)

Objection retracted, sorry for the noise.

[Yong Li]

More information: This thing is addressed in RVCT 4.0 Compiler Reference Guide:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0348c/Babjddhe.html

"A plain bitfield, declared without either signed or unsigned qualifiers, is treated as unsigned. For example, int x:10 allocates an unsigned integer of 10 bits."

However, there is also flags like "--signed-bitfields" to change the behavior.

[Eric Seidel (no email)]

That is fantastic information to include in your Changelog!
It definitely seems like this is something we could/should add to check-webkit-style.  Dave Levin would be a good person to find on #webkit if you need tips on how to do such.

[Yong Li]

(In reply to comment #13)
> That is fantastic information to include in your Changelog!
> It definitely seems like this is something we could/should add to check-webkit-style.  Dave Levin would be a good person to find on #webkit if you need tips on how to do such.

I'll give a try

[Yong Li]

Created attachment 83399 [details]
the patch

[Yong Li]

Created attachment 83495 [details]
Updated

modified the regular expression to exclude the cases like "constint" (no space between const and int), and like "intm_data" (no space between int and m_data)

[Alexey Proskuryakov]

Comment on attachment 83495 [details]
Updated

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

Looks good to me. Since I'm not normally working on either side of this code, deferring review to others.

> Source/JavaScriptCore/assembler/ARMAssembler.h:254
> +            unsigned m_used : 1;

Could it be a bool?

> Source/JavaScriptCore/assembler/ARMv7Assembler.h:532
> +        unsigned m_used : 1;

Ditto.

> Source/JavaScriptCore/assembler/MIPSAssembler.h:207
> +        unsigned m_used : 1;

Ditto.

> Source/JavaScriptCore/assembler/X86Assembler.h:262
>          bool m_used : 1;

This one is a bool!

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:144
> +        unsigned seen : 1;

Could this be a bool, too?

[Yong Li]

(In reply to comment #17)
> (From update of attachment 83495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83495&action=review
> 
> Looks good to me. Since I'm not normally working on either side of this code, deferring review to others.
> 
> > Source/JavaScriptCore/assembler/ARMAssembler.h:254
> > +            unsigned m_used : 1;
> 
> Could it be a bool?
> 
> > Source/JavaScriptCore/assembler/ARMv7Assembler.h:532
> > +        unsigned m_used : 1;
> 
> Ditto.
> 
> > Source/JavaScriptCore/assembler/MIPSAssembler.h:207
> > +        unsigned m_used : 1;
> 
> Ditto.
> 
> > Source/JavaScriptCore/assembler/X86Assembler.h:262
> >          bool m_used : 1;
> 
> This one is a bool!
> 
> > Source/JavaScriptCore/bytecode/StructureStubInfo.h:144
> > +        unsigned seen : 1;
> 
> Could this be a bool, too?

I also noticed this. I'm not sure which one we should use. Compilers could probably pack "bool" bitfields differently.

I just did a test with VC. 

struct A {
    unsigned x : 31;
    bool y : 1;
};

sizeof(A) = 8

[Alexey Proskuryakov]

Weird. May compilers do the same for signed/unsigned? I'm wondering if it would be safer to change all int bitfields to signed then.

[Yong Li]

(In reply to comment #19)
> Weird. May compilers do the same for signed/unsigned? I'm wondering if it would be safer to change all int bitfields to signed then.

int => signed should be no-op if int is treated as signed.

Results on VC
struct {
unsigned x:30;
signed y:2;
};
sizeof = 4.

struct {
unsigned x:30;
short y:2;
};
sizeof = 8.

So basically when x and y's base types have same size, they are packed. When x and y's base types have different size, VC doesn't pack them.

so all bitfields that are supposed to be packed should have same sized base types. it doesn't matter it is signed or unsigned.

[Eric Seidel (no email)]

Sounds like we are going to want COMPILE_ASSERT(sizeof(type)) after all of our bitfield uses.  If we're using bitfields we should be ASSERTing that the compiler is making them the size we expect. :)

[Yong Li]

(In reply to comment #21)
> Sounds like we are going to want COMPILE_ASSERT(sizeof(type)) after all of our bitfield uses.  If we're using bitfields we should be ASSERTing that the compiler is making them the size we expect. :)

The task is growing...

[Eric Seidel (no email)]

You shouldn't feel like you have to do all of this (certainly not on this bug!), but you seem to have stumbled on a gold-mine of compiler incompatibilities which we should document with bugs and ideally come up with patterns/checking-tools to avoid in the future.

[Yong Li]

(In reply to comment #23)
> You shouldn't feel like you have to do all of this (certainly not on this bug!), but you seem to have stumbled on a gold-mine of compiler incompatibilities which we should document with bugs and ideally come up with patterns/checking-tools to avoid in the future.

Bug 55088 created for this

[Eric Seidel (no email)]

Comment on attachment 83495 [details]
Updated

These look fine to me, although given that these places are likely size-sensitive, I would add the COMPILE_ASSERT lines (as they should be easy to add).  The only tricky part there is that that might make landing harder if other compilers fail those checks.

The cpp.py change needs a test case.

Maybe you want to do these in separate patches?  r- for the lackof the cpp.py test case.

[Yong Li]

(In reply to comment #25)
> (From update of attachment 83495 [details])
> These look fine to me, although given that these places are likely size-sensitive, I would add the COMPILE_ASSERT lines (as they should be easy to add).  The only tricky part there is that that might make landing harder if other compilers fail those checks.
> 
> The cpp.py change needs a test case.
> 
> Maybe you want to do these in separate patches?  r- for the lackof the cpp.py test case.

I didn't even noticed cpp.py file. will add a test case

[Yong Li]

Created attachment 83652 [details]
with test case

[Yong Li]

Created attachment 83656 [details]
with test case for style checker

[Eric Seidel (no email)]

Comment on attachment 83656 [details]
with test case for style checker

LGTM.

[Yong Li]

Oops, I missed the notification. I'll commit the patch

[Yong Li]

Created attachment 95442 [details]
ready to commit

[Yong Li]

Created attachment 95456 [details]
fix the expected test result

[WebKit Commit Bot]

Comment on attachment 95456 [details]
fix the expected test result

Clearing flags on attachment: 95456

Committed r87771: <http://trac.webkit.org/changeset/87771>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.