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

4320–4325

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

4320–4325

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

12611

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

4320–4325

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

12611

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

4320–4325

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

12586

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).

12611

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.

12613

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

12613

I think this is PR 23505.

[hubert.reinterpretcast]

lib/Sema/SemaDecl.cpp

12586

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

12586

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_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_incorrect_number_of_vector_initializers : Error<
   "number of elements must be either one or match the size of the vector">;
+def warn_bitfield_width_longer_than_necessary : Warning<
+  "size of bit-field %0 (%1 bits) exceeds the minimum width required to "
+  "represent all valid values of that bit-field type">, InGroup<BitFieldWidth>;
+def warn_anon_bitfield_width_longer_than_necessary : Warning<
+  "size of anonymous bit-field (%0 bits) exceeds the minimum width required to "
+  "represent all valid values of that bit-field type">, InGroup<BitFieldWidth>;
 
 // 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">;
+  "truncated to %2 bits">, InGroup<BitFieldWidth>;
 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">;
+  "be truncated to %1 bits">, 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

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) {
       if (!getLangOpts().CPlusPlus || IsMsStruct ||
           Context.getTargetInfo().getCXXABI().isMicrosoft()) {
         if (FieldName) 
           return Diag(FieldLoc, diag::err_bitfield_width_exceeds_type_size)
             << FieldName << (unsigned)Value.getZExtValue() 
             << (unsigned)TypeSize;
         
         return Diag(FieldLoc, diag::err_anon_bitfield_width_exceeds_type_size)
           << (unsigned)Value.getZExtValue() << (unsigned)TypeSize;
       }
       
       if (FieldName)
         Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_size)
           << FieldName << (unsigned)Value.getZExtValue() 
           << (unsigned)TypeSize;
       else
         Diag(FieldLoc, diag::warn_anon_bitfield_width_exceeds_type_size)
           << (unsigned)Value.getZExtValue() << (unsigned)TypeSize;        
     }
+    // C11 6.7.2.1p4 + footnote 122/C99 6.7.2.1p3 - the width of a bitfield
+    // of type _Bool may be erroneous if it exceeds 1 bit.
+    // Warn about _Bool bitfields > 1 bit as they will not be portable across
+    // different platforms
+    if ((getLangOpts().C11 || getLangOpts().C99) && 
+        FieldTy->isBooleanType() &&
+        Value.getZExtValue() > 1) {
+      if (FieldName)
+        Diag(FieldLoc, diag::warn_bitfield_width_longer_than_necessary)
+            << FieldName << (unsigned)Value.getZExtValue();
+      else
+        Diag(FieldLoc, diag::warn_anon_bitfield_width_longer_than_necessary)
+            << (unsigned)Value.getZExtValue();
+    }
   }
 
   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,

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/c11-bitfield-width.c

+// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c11
+// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c99
+
+struct A {
+  _Bool : 0;    // ok
+  _Bool : 3;    // expected-warning {{size of anonymous bit-field (3 bits) exceeds the minimum width required to represent all valid values of that bit-field type}}
+  _Bool : 5;    // expected-warning {{size of anonymous bit-field (5 bits) exceeds the minimum width required to represent all valid values of that bit-field type}}
+  _Bool : 7;    // expected-warning {{size of anonymous bit-field (7 bits) exceeds the minimum width required to represent all valid values of that bit-field type}}
+  _Bool a : 1;  // ok
+  _Bool b : 2;  // expected-warning {{size of bit-field 'b' (2 bits) exceeds the minimum width required to represent all valid values of that bit-field type}}
+  _Bool c : 4;  // expected-warning {{size of bit-field 'c' (4 bits) exceeds the minimum width required to represent all valid values of that bit-field type}}
+  _Bool d : 8;  // expected-warning {{size of bit-field 'd' (8 bits) exceeds the minimum width required to represent all valid values of that bit-field type}}
+};