C11 _Bool bitfield diagnostic

[rcraik]

Summary

Implement DR262 (for C). This patch will mainly affect bitfields of type _Bool

[hubert.reinterpretcast]

The change in the C11 Standard is that the number of bits in the value representation is now used as opposed to the number of bits in the object representation. The message is generic to allow for additional types with padding bits; however, the change does not use a generic mechanism to handle such types.

In addition, feedback is requested as to whether the message for the case of _Bool should be limited to C11 modes or not.

[rcraik]

Ping!

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

4324–4329

Since this is a constraint violation according to the C11 standard, this should be an ExtWarn or Extension rather than merely a Warning.

4324–4329

I think this diagnostic should be more specific. The diagnostic text should make it clear that: (1) the problem is that the field has a boolean type, and (2) why that might deserve a warning. Something like "ISO C11 forbids bit-field of type %0 of size greater than 1 (%1 specified)", maybe.

lib/Sema/SemaDecl.cpp

12653

It looks like this should apply in all modes that aren't C++: even in C99, we have a constraint in 6.7.2.1 that the specified bit-width cannot be greater than the width of the type, and width is defined in 6.2.6.2 as the number of precision + sign bits.

[hubert.reinterpretcast]

include/clang/Basic/DiagnosticSemaKinds.td

4324–4329

I have not found any normative text which indicates that _Bool cannot have width greater then 1. Also, the proposed new message text makes it less clear that the property is platform-dependent.

Note that the check associated with _Bool hard-codes "1", and the message may be emitted even when the constraint is not violated. The message text proposed in the patch is sufficiently generic to accommodate that.

lib/Sema/SemaDecl.cpp

12653

Yes, this was a change applied in TC2.

[rcraik]

I've updated the patch to produce the warning in C99 as well as C11 modes

[rcraik]

Ping!

[hubert.reinterpretcast]

The values of _Bool is not limited to 0 and 1 in C (although storing such a value is not straightforward). It is conceivable that some platforms would have _Bool, aside from conversions, behave the same as unsigned char. My earlier statement that the message is sufficiently generic does not take this consideration into account.

It seems making this a plain warning specifically for _Bool on a portability basis is viable; however, the best argument I could come up with for why _Bool warrants special treatment is that it is somewhat likely to have padding bits. My guess is that implementations which consider _Bool to have padding bits are likely to consider all cases where the padding bits are non-zero to be trap representations.

[rcraik]

I've updated the message to make it a bit clearer that this is a portability concern. Are there any further concerns with this patch?

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

4324–4329

I suppose your argument for the width of _Bool being possibly greater than 1 is that, while _Bool is required to be large enough to store the values 0 and 1 (6.2.5/2), it may theoretically have value bits representing 2, 4, 8, ... (even though 6.3.1.2/1 does not allow creation of a value that sets those bits). That interpretation seems consistent with C's rules.

However, for Clang, the width of _Bool is 1, so this is a constraint violation.

lib/Sema/SemaDecl.cpp

12627–12628

I think the right way to fix this is to call getIntWidth here instead of getTypeSize, and finesse our error message to clarify that we're talking about the width of the type (the number of value bits) rather than the size of the type (the number of storage bits).

12653

This is redundant: C99 is always set whenever C11 is (C99 means "C99 or later"). Just test for C99 here. I also think we should apply this rule to C89, where we support _Bool as an extension and use the C99 rules for it. The above check for !CPlusPlus seems right.

12655

This will assert if the specified bitfield width doesn't fit in 64 bits. You can use Value.ugt(1) instead.

The check above for TypeSize has the same bug. This testcase causes an assertion to fire:

struct X { int n : 0xffffffff * (__int128)0xffffffff * 0xffffffff; };

[hubert.reinterpretcast]

lib/Sema/SemaDecl.cpp

12655

I think this is PR 23505.

[hubert.reinterpretcast]

lib/Sema/SemaDecl.cpp

12627–12628

The implementation of getIntWidth currently makes this consideration moot at this time, but should this extend to C89 (aside from the _Bool extension)?

[rcraik]

lib/Sema/SemaDecl.cpp

12627–12628

I think we have three options (the special case for _Bool bitfields being removed in each case):

1. change getTypeSize to getIntWidth and leave the rest of the checks as-is
2. change getTypeSize to getIntWidth and update the C/MS diagnostic to either ExtWarn or Warning (for some or all language levels)
3. leave as getTypeSize for lower language levels

Opinions?

[rcraik]

switched to using getIntWidth instead of getTypeSize and updated the error and warning messages accordingly, as have the necessary test cases. The separate check for _Bool bitfields has been removed, so the check is now consistent for all types.

[rsmith]

Looks great, please go ahead. Sorry it's taken so long to get this reviewed.

include/clang/Basic/DiagnosticGroups.td

 def ArrayBounds : DiagGroup<"array-bounds">;
 def ArrayBoundsPointerArithmetic : DiagGroup<"array-bounds-pointer-arithmetic">;
 def Availability : DiagGroup<"availability">;
 def Section : DiagGroup<"section">;
 def AutoImport : DiagGroup<"auto-import">;
 def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
 def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
 def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
+def BitFieldWidth : DiagGroup<"bitfield-width">;
 def ConstantConversion :
   DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
 def LiteralConversion : DiagGroup<"literal-conversion">;
 def StringConversion : DiagGroup<"string-conversion">;
 def SignConversion : DiagGroup<"sign-conversion">;
 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;
 def UndefinedBoolConversion : DiagGroup<"undefined-bool-conversion">;
 def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion,

include/clang/Basic/DiagnosticSemaKinds.td

def err_init_objc_class : Error<
   "cannot initialize Objective-C class type %0">;
 def err_implicit_empty_initializer : Error<
   "initializer for aggregate with no elements requires explicit braces">;
 def err_bitfield_has_negative_width : Error<
   "bit-field %0 has negative width (%1)">;
 def err_anon_bitfield_has_negative_width : Error<
   "anonymous bit-field has negative width (%0)">;
 def err_bitfield_has_zero_width : Error<"named bit-field %0 has zero width">;
-def err_bitfield_width_exceeds_type_size : Error<
-  "size of bit-field %0 (%1 bits) exceeds size of its type (%2 bits)">;
-def err_anon_bitfield_width_exceeds_type_size : Error<
-  "size of anonymous bit-field (%0 bits) exceeds size of its type (%1 bits)">;
+def err_bitfield_width_exceeds_type_width : Error<
+  "width of bit-field %0 (%1 bits) exceeds width of its type (%2 bit%s2)">;
+def err_anon_bitfield_width_exceeds_type_width : Error<
+  "width of anonymous bit-field (%0 bits) exceeds width of its type "
+  "(%1 bit%s1)">;
 def err_incorrect_number_of_vector_initializers : Error<
   "number of elements must be either one or match the size of the vector">;
 
 // Used by C++ which allows bit-fields that are wider than the type.
-def warn_bitfield_width_exceeds_type_size: Warning<
-  "size of bit-field %0 (%1 bits) exceeds the size of its type; value will be "
-  "truncated to %2 bits">;
-def warn_anon_bitfield_width_exceeds_type_size : Warning<
-  "size of anonymous bit-field (%0 bits) exceeds size of its type; value will "
-  "be truncated to %1 bits">;
+def warn_bitfield_width_exceeds_type_width: Warning<
+  "width of bit-field %0 (%1 bits) exceeds the width of its type; value will "
+  "be truncated to %2 bit%s2">, InGroup<BitFieldWidth>;
+def warn_anon_bitfield_width_exceeds_type_width : Warning<
+  "width of anonymous bit-field (%0 bits) exceeds width of its type; value "
+  "will be truncated to %1 bit%s1">, InGroup<BitFieldWidth>;
 
 def warn_missing_braces : Warning<
   "suggest braces around initialization of subobject">,
   InGroup<MissingBraces>, DefaultIgnore;
 
 def err_redefinition_of_label : Error<"redefinition of label %0">;
 def err_undeclared_label_use : Error<"use of undeclared label %0">;
 def err_goto_ms_asm_label : Error<

lib/Sema/SemaDecl.cpp

ExprResult Sema::VerifyBitField(SourceLocation FieldLoc,
   if (Value.isSigned() && Value.isNegative()) {
     if (FieldName)
       return Diag(FieldLoc, diag::err_bitfield_has_negative_width)
                << FieldName << Value.toString(10);
     return Diag(FieldLoc, diag::err_anon_bitfield_has_negative_width)
       << Value.toString(10);
   }
 
   if (!FieldTy->isDependentType()) {
-    uint64_t TypeSize = Context.getTypeSize(FieldTy);
-    if (Value.getZExtValue() > TypeSize) {
+    uint64_t TypeWidth = Context.getIntWidth(FieldTy);
+    if (Value.ugt(TypeWidth)) {
       if (!getLangOpts().CPlusPlus || IsMsStruct ||
           Context.getTargetInfo().getCXXABI().isMicrosoft()) {
         if (FieldName) 
-          return Diag(FieldLoc, diag::err_bitfield_width_exceeds_type_size)
+          return Diag(FieldLoc, diag::err_bitfield_width_exceeds_type_width)
             << FieldName << (unsigned)Value.getZExtValue() 
-            << (unsigned)TypeSize;
+            << (unsigned)TypeWidth;
         
-        return Diag(FieldLoc, diag::err_anon_bitfield_width_exceeds_type_size)
-          << (unsigned)Value.getZExtValue() << (unsigned)TypeSize;
+        return Diag(FieldLoc, diag::err_anon_bitfield_width_exceeds_type_width)
+          << (unsigned)Value.getZExtValue() << (unsigned)TypeWidth;
       }
       
       if (FieldName)
-        Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_size)
+        Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_width)
           << FieldName << (unsigned)Value.getZExtValue() 
-          << (unsigned)TypeSize;
+          << (unsigned)TypeWidth;
       else
-        Diag(FieldLoc, diag::warn_anon_bitfield_width_exceeds_type_size)
-          << (unsigned)Value.getZExtValue() << (unsigned)TypeSize;        
+        Diag(FieldLoc, diag::warn_anon_bitfield_width_exceeds_type_width)
+          << (unsigned)Value.getZExtValue() << (unsigned)TypeWidth;        
     }
   }
 
   return BitWidth;
 }
 
 /// ActOnField - Each field of a C struct/union is passed into this in order
 /// to create a FieldDecl object for it.
 Decl *Sema::ActOnField(Scope *S, Decl *TagD, SourceLocation DeclStart,
                        Declarator &D, Expr *BitfieldWidth) {
   FieldDecl *Res = HandleField(S, cast_or_null<RecordDecl>(TagD),
                                DeclStart, D, static_cast<Expr*>(BitfieldWidth),
                                /*InitStyle=*/ICIS_NoInit, AS_public);
   return Res;
 }
 

test/CodeGen/bitfield-2.c

unsigned long long test_5() {
   res ^= f5_load(&g5) ^ f5_store(&g5) ^ f5_reload(&g5);
   res ^= g5.f0 ^ g5.f1 ^ g5.f2;
   return res;
 }
 
 /***/
 
 struct s6 {
-  _Bool f0 : 2;
+  unsigned f0 : 2;
 };
 
 struct s6 g6 = { 0xF };
 
 int f6_load(struct s6 *a0) {
   return a0->f0;
 }
 int f6_store(struct s6 *a0) {

test/CodeGenCXX/warn-padded-packed.cpp

 
 struct S12 {
   bool b : 1;
   char c; // expected-warning {{padding struct 'S12' with 7 bits to align 'c'}}
 };
 
 struct S13 { // expected-warning {{padding size of 'S13' with 6 bits to alignment boundary}}
   char c;
-  bool b : 10; // expected-warning {{size of bit-field 'b' (10 bits) exceeds the size of its type}}
+  bool b : 10; // expected-warning {{width of bit-field 'b' (10 bits) exceeds the width of its type}}
 };
 
 // The warnings are emitted when the layout of the structs is computed, so we have to use them.
 void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*) { }

test/Misc/warning-flags.c

(2) It prevents us adding new warnings to Clang that have no -W flag.  All
     new warnings should have -W flags.
 
     If you add a new warning without a flag, this test will fail.  To fix
     this test, simply add a warning group to that warning.
 
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (92):
+CHECK: Warnings without flags (90):
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_initializer_string_for_char_array_too_long
 CHECK-NEXT:   ext_missing_declspec
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst

 CHECK-NEXT:   pp_include_next_in_primary
 CHECK-NEXT:   pp_invalid_string_literal
 CHECK-NEXT:   pp_out_of_date_dependency
 CHECK-NEXT:   pp_poisoning_existing_macro
 CHECK-NEXT:   pp_pragma_once_in_main_file
 CHECK-NEXT:   pp_pragma_sysheader_in_main_file
 CHECK-NEXT:   w_asm_qualifier_ignored
 CHECK-NEXT:   warn_accessor_property_type_mismatch
-CHECK-NEXT:   warn_anon_bitfield_width_exceeds_type_size
 CHECK-NEXT:   warn_arcmt_nsalloc_realloc
 CHECK-NEXT:   warn_asm_label_on_auto_decl
-CHECK-NEXT:   warn_bitfield_width_exceeds_type_size
 CHECK-NEXT:   warn_c_kext
 CHECK-NEXT:   warn_call_to_pure_virtual_member_function_from_ctor_dtor
 CHECK-NEXT:   warn_call_wrong_number_of_arguments
 CHECK-NEXT:   warn_case_empty_range
 CHECK-NEXT:   warn_char_constant_too_large
 CHECK-NEXT:   warn_collection_expr_type
 CHECK-NEXT:   warn_conflicting_variadic
 CHECK-NEXT:   warn_conv_to_base_not_used

test/Sema/bitfield.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify -std=c11 -Wno-unused-value
 
 enum e0; // expected-note{{forward declaration of 'enum e0'}}
 
 struct a {
   int a : -1; // expected-error{{bit-field 'a' has negative width}}
 
   // rdar://6081627
-  int b : 33; // expected-error{{size of bit-field 'b' (33 bits) exceeds size of its type (32 bits)}}
+  int b : 33; // expected-error{{width of bit-field 'b' (33 bits) exceeds width of its type (32 bits)}}
 
   int c : (1 + 0.25); // expected-error{{expression is not an integer constant expression}}
   int d : (int)(1 + 0.25); 
 
   // rdar://6138816
   int e : 0;  // expected-error {{bit-field 'e' has zero width}}
 
   float xx : 4;  // expected-error {{bit-field 'xx' has non-integral type}}
 
   // PR3607
   enum e0 f : 1; // expected-error {{field has incomplete type 'enum e0'}}
   
   int g : (_Bool)1;
   
   // PR4017  
-  char : 10;      // expected-error {{size of anonymous bit-field (10 bits) exceeds size of its type (8 bits)}}
+  char : 10;      // expected-error {{width of anonymous bit-field (10 bits) exceeds width of its type (8 bits)}}
   unsigned : -2;  // expected-error {{anonymous bit-field has negative width (-2)}}
   float : 12;     // expected-error {{anonymous bit-field has non-integral type 'float'}}
+
+  _Bool : 2;   // expected-error {{width of anonymous bit-field (2 bits) exceeds width of its type (1 bit)}}
+  _Bool h : 5; // expected-error {{width of bit-field 'h' (5 bits) exceeds width of its type (1 bit)}}
 };
 
 struct b {unsigned x : 2;} x;
 __typeof__(x.x+1) y;
 int y;
 
 struct {unsigned x : 2;} x2;
 __typeof__((x.x+=1)+1) y;

test/SemaCXX/bitfield-layout.cpp

 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=x86_64-apple-darwin10
 
 #define CHECK_SIZE(name, size) extern int name##1[sizeof(name) == size ? 1 : -1];
 #define CHECK_ALIGN(name, size) extern int name##2[__alignof(name) == size ? 1 : -1];
 
 // Simple tests.
 struct Test1 {
-  char c : 9; // expected-warning {{size of bit-field 'c' (9 bits) exceeds the size of its type; value will be truncated to 8 bits}}
+  char c : 9; // expected-warning {{width of bit-field 'c' (9 bits) exceeds the width of its type; value will be truncated to 8 bits}}
 };
 CHECK_SIZE(Test1, 2);
 CHECK_ALIGN(Test1, 1);
 
 struct Test2 {
-  char c : 16; // expected-warning {{size of bit-field 'c' (16 bits) exceeds the size of its type; value will be truncated to 8 bits}}
+  char c : 16; // expected-warning {{width of bit-field 'c' (16 bits) exceeds the width of its type; value will be truncated to 8 bits}}
 };
 CHECK_SIZE(Test2, 2);
 CHECK_ALIGN(Test2, 2);
 
 struct Test3 {
-  char c : 32; // expected-warning {{size of bit-field 'c' (32 bits) exceeds the size of its type; value will be truncated to 8 bits}}
+  char c : 32; // expected-warning {{width of bit-field 'c' (32 bits) exceeds the width of its type; value will be truncated to 8 bits}}
 };
 CHECK_SIZE(Test3, 4);
 CHECK_ALIGN(Test3, 4);
 
 struct Test4 {
-  char c : 64; // expected-warning {{size of bit-field 'c' (64 bits) exceeds the size of its type; value will be truncated to 8 bits}}
+  char c : 64; // expected-warning {{width of bit-field 'c' (64 bits) exceeds the width of its type; value will be truncated to 8 bits}}
 };
 CHECK_SIZE(Test4, 8);
 CHECK_ALIGN(Test4, 8);
 

test/SemaCXX/constant-expression-cxx11.cpp

namespace Lifetime {
   constexpr int k2 = S(0).t; // expected-error {{constant expression}} expected-note {{in call}}
 }
 
 namespace Bitfields {
   struct A {
     bool b : 1;
     unsigned u : 5;
     int n : 5;
-    bool b2 : 3;
-    unsigned u2 : 74; // expected-warning {{exceeds the size of its type}}
-    int n2 : 81; // expected-warning {{exceeds the size of its type}}
+    bool b2 : 3; // expected-warning {{exceeds the width of its type}}
+    unsigned u2 : 74; // expected-warning {{exceeds the width of its type}}
+    int n2 : 81; // expected-warning {{exceeds the width of its type}}
   };
 
   constexpr A a = { false, 33, 31, false, 0xffffffff, 0x7fffffff }; // expected-warning 2{{truncation}}
   static_assert(a.b == 0 && a.u == 1 && a.n == -1 && a.b2 == 0 &&
                 a.u2 + 1 == 0 && a.n2 == 0x7fffffff,
                 "bad truncation of bitfield values");
 
   struct B {

test/SemaCXX/constant-expression-cxx1y.cpp

for (int i = 0; i != 2; ++i) {
         ++*q;
     }
   }
   static_assert((lifetime_versus_loops(), true), "");
 }
 
 namespace Bitfields {
   struct A {
-    bool b : 3;
+    bool b : 1;
     int n : 4;
     unsigned u : 5;
   };
   constexpr bool test() {
     A a {};
     a.b += 2;
     --a.n;
     --a.u;

test/SemaCXX/ms_wide_bitfield.cpp

 // RUN: %clang_cc1 -fno-rtti -emit-llvm-only -triple i686-pc-win32 -fdump-record-layouts -fsyntax-only -mms-bitfields -verify %s 2>&1
 
 struct A {
-  char a : 9; // expected-error{{size of bit-field 'a' (9 bits) exceeds size of its type (8 bits)}}
-  int b : 33; // expected-error{{size of bit-field 'b' (33 bits) exceeds size of its type (32 bits)}}
-  bool c : 9; // expected-error{{size of bit-field 'c' (9 bits) exceeds size of its type (8 bits)}}
+  char a : 9; // expected-error{{width of bit-field 'a' (9 bits) exceeds width of its type (8 bits)}}
+  int b : 33; // expected-error{{width of bit-field 'b' (33 bits) exceeds width of its type (32 bits)}}
+  bool c : 9; // expected-error{{width of bit-field 'c' (9 bits) exceeds width of its type (1 bit)}}
+  bool d : 3; // expected-error{{width of bit-field 'd' (3 bits) exceeds width of its type (1 bit)}}
 };
 
 int a[sizeof(A) == 1 ? 1 : -1];

test/SemaObjC/class-bitfield.m

 // RUN: %clang_cc1 %s -fobjc-runtime=macosx-fragile-10.5 -fsyntax-only -verify 
 
 @interface X 
 {
   int a : -1; // expected-error{{bit-field 'a' has negative width}}
 
   // rdar://6081627
-  int b : 33; // expected-error{{size of bit-field 'b' (33 bits) exceeds size of its type (32 bits)}}
+  int b : 33; // expected-error{{width of bit-field 'b' (33 bits) exceeds width of its type (32 bits)}}
 
   int c : (1 + 0.25); // expected-error{{expression is not an integer constant expression}}
   int d : (int)(1 + 0.25); 
 
   // rdar://6138816
   int e : 0;  // expected-error {{bit-field 'e' has zero width}}
 }
 @end