| Summary: | Always explicitly use "signed" keyword to declare signed bitfields | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yong Li <yong.li.webkit> | ||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Minor | CC: | ap, barraclough, commit-queue, darin, dbates, eric, levin, tonikitoo | ||||||||||||||
| Priority: | P3 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
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. 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. (I CC'd Darin just because he would remember the days of the MSVC enum confusion and might have further opinions here.) But a "signed int" type doesn't exist in C++, does it? See the spec, paragraph 3.9.1. (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. 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". (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? Yes, it's redundant and against WebKit coding style. (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. (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>. 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. 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. 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. (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 Created attachment 83399 [details]
the patch
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 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? (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 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. (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. 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. :) (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... 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. (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 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.
(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 Created attachment 83652 [details]
with test case
Created attachment 83656 [details]
with test case for style checker
Comment on attachment 83656 [details]
with test case for style checker
LGTM.
Oops, I missed the notification. I'll commit the patch Created attachment 95442 [details]
ready to commit
Created attachment 95456 [details]
fix the expected test result
Comment on attachment 95456 [details] fix the expected test result Clearing flags on attachment: 95456 Committed r87771: <http://trac.webkit.org/changeset/87771> All reviewed patches have been landed. Closing bug. |
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.