Bug 54807 - Always explicitly use "signed" keyword to declare signed bitfields
Summary: Always explicitly use "signed" keyword to declare signed bitfields
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-19 07:57 PST by Yong Li
Modified: 2011-05-31 19:40 PDT (History)
8 users (show)

See Also:


Attachments
the patch (9.67 KB, patch)
2011-02-22 15:16 PST, Yong Li
no flags Details | Formatted Diff | Diff
Updated (9.67 KB, patch)
2011-02-23 10:00 PST, Yong Li
eric: review-
Details | Formatted Diff | Diff
with test case (10.61 KB, patch)
2011-02-24 07:32 PST, Yong Li
no flags Details | Formatted Diff | Diff
with test case for style checker (10.65 KB, patch)
2011-02-24 07:45 PST, Yong Li
eric: review+
Details | Formatted Diff | Diff
ready to commit (8.61 KB, patch)
2011-05-31 09:29 PDT, Yong Li
yong.li.webkit: commit-queue-
Details | Formatted Diff | Diff
fix the expected test result (8.63 KB, patch)
2011-05-31 11:29 PDT, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2011-02-19 07:57:30 PST
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.
Comment 1 Antonio Gomes 2011-02-19 09:19:23 PST
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.
Comment 2 Eric Seidel (no email) 2011-02-19 13:03:18 PST
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.
Comment 3 Eric Seidel (no email) 2011-02-19 13:03:58 PST
(I CC'd Darin just because he would remember the days of the MSVC enum confusion and might have further opinions here.)
Comment 4 Alexey Proskuryakov 2011-02-19 20:48:01 PST
But a "signed int" type doesn't exist in C++, does it? See the spec, paragraph 3.9.1.
Comment 5 Daniel Bates 2011-02-19 22:31:19 PST
(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.
Comment 6 Alexey Proskuryakov 2011-02-19 22:41:18 PST
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".
Comment 7 Daniel Bates 2011-02-19 22:50:11 PST
(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?
Comment 8 Alexey Proskuryakov 2011-02-19 22:56:21 PST
Yes, it's redundant and against WebKit coding style.
Comment 9 Daniel Bates 2011-02-19 23:00:56 PST
(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.
Comment 10 Daniel Bates 2011-02-19 23:02:16 PST
(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>.
Comment 11 Alexey Proskuryakov 2011-02-19 23:10:24 PST
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.
Comment 12 Yong Li 2011-02-22 08:37:45 PST
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.
Comment 13 Eric Seidel (no email) 2011-02-22 12:16:17 PST
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.
Comment 14 Yong Li 2011-02-22 13:14:21 PST
(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
Comment 15 Yong Li 2011-02-22 15:16:25 PST
Created attachment 83399 [details]
the patch
Comment 16 Yong Li 2011-02-23 10:00:50 PST
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)
Comment 17 Alexey Proskuryakov 2011-02-23 11:19:23 PST
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?
Comment 18 Yong Li 2011-02-23 13:02:31 PST
(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
Comment 19 Alexey Proskuryakov 2011-02-23 13:53:13 PST
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.
Comment 20 Yong Li 2011-02-23 14:02:08 PST
(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.
Comment 21 Eric Seidel (no email) 2011-02-23 14:12:55 PST
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. :)
Comment 22 Yong Li 2011-02-23 14:47:51 PST
(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...
Comment 23 Eric Seidel (no email) 2011-02-23 14:51:33 PST
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.
Comment 24 Yong Li 2011-02-23 15:00:47 PST
(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
Comment 25 Eric Seidel (no email) 2011-02-23 15:05:41 PST
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.
Comment 26 Yong Li 2011-02-23 15:07:48 PST
(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
Comment 27 Yong Li 2011-02-24 07:32:41 PST
Created attachment 83652 [details]
with test case
Comment 28 Yong Li 2011-02-24 07:45:28 PST
Created attachment 83656 [details]
with test case for style checker
Comment 29 Eric Seidel (no email) 2011-03-22 16:30:47 PDT
Comment on attachment 83656 [details]
with test case for style checker

LGTM.
Comment 30 Yong Li 2011-05-31 08:57:20 PDT
Oops, I missed the notification. I'll commit the patch
Comment 31 Yong Li 2011-05-31 09:29:19 PDT
Created attachment 95442 [details]
ready to commit
Comment 32 Yong Li 2011-05-31 11:29:17 PDT
Created attachment 95456 [details]
fix the expected test result
Comment 33 WebKit Commit Bot 2011-05-31 19:40:41 PDT
Comment on attachment 95456 [details]
fix the expected test result

Clearing flags on attachment: 95456

Committed r87771: <http://trac.webkit.org/changeset/87771>
Comment 34 WebKit Commit Bot 2011-05-31 19:40:49 PDT
All reviewed patches have been landed.  Closing bug.