[clang][objc] Add a warning when declaring BOOL bitfields with width 1

[teemperor]

Summary

In Objective-C, BOOL is either signed char or _Bool depending on the platform and the well defined values for it are YES(1) and NO(0).

For BOOL being signed char this brings up the confusing situation below:

struct X {
  BOOL b : 1;
};
int main() {
  X x;
  x.b = YES;
  if (x.b == YES) {
    // This will never be executed as `x.b` is actually`-1`.
  }
}

We already added support for detecting this situation to ubsan (see https://reviews.llvm.org/D30423?id=89914#inline-266079),
but I think this should also be a compiler warning and that tells people to use unsigned as the bitfield type. This way users
that don't run the affected code with ubsan also know that it probably won't work (and won't file radars against LLDB such as
rdar://71815375 ).

This patch adds a -Wobjc-bool-bitfield that suggests using unsigned for BOOL bitfields of width 1. I enabled it by default
but restricted it to configurations where BOOL is actually signed char as this is where the issue actually arises. For BOOL = bool
configurations this would only limit portability and I guess we don't know if users care about this (and if they do, they'll see
the warning when they compile for a platform where this is an issue).

A related follow-up warning I want to add: We should probably also warn if BOOL bitfields have a width > 1 as that only
works when BOOL = signed char (but stops compiling once the platform changes to BOOL = bool). It's also
kind of nonsensical to use a width greater than 1 for BOOL.

clang/include/clang/Basic/DiagnosticGroups.td

 def ObjCReadonlyPropertyHasSetter : DiagGroup<"objc-readonly-with-setter-property">;
 def ObjCInvalidIBOutletProperty : DiagGroup<"invalid-iboutlet">;
 def ObjCRootClass : DiagGroup<"objc-root-class">;
 def ObjCPointerIntrospectPerformSelector : DiagGroup<"deprecated-objc-pointer-introspection-performSelector">;
 def ObjCPointerIntrospect : DiagGroup<"deprecated-objc-pointer-introspection", [ObjCPointerIntrospectPerformSelector]>;
 def ObjCMultipleMethodNames : DiagGroup<"objc-multiple-method-names">;
 def ObjCFlexibleArray : DiagGroup<"objc-flexible-array">;
 def ObjCBoxing : DiagGroup<"objc-boxing">;
+def ObjCBoolBitfield : DiagGroup<"objc-bool-bitfield">;
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 def UnderalignedExceptionObject : DiagGroup<"underaligned-exception-object">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def Packed : DiagGroup<"packed">;
 def Padded : DiagGroup<"padded">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
def Most : DiagGroup<"most", [
     Trigraphs,
     Uninitialized,
     UnknownPragmas,
     Unused,
     VolatileRegisterVar,
     ObjCMissingSuperCalls,
     ObjCDesignatedInit,
     ObjCFlexibleArray,
+    ObjCBoolBitfield,
     OverloadedVirtual,
     PrivateExtern,
     SelTypeCast,
     ExternCCompat,
     UserDefinedWarnings
  ]>;
 
 // Thread Safety warnings

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def ext_flexible_array_union_gnu : Extension<
   "flexible array member %0 in a union is a GNU extension">, InGroup<GNUFlexibleArrayUnionMember>;
 
 def err_flexible_array_not_at_end : Error<
   "flexible array member %0 with type %1 is not at the end of"
   " %select{struct|interface|union|class|enum}2">;
 def err_objc_variable_sized_type_not_at_end : Error<
   "field %0 with variable sized type %1 is not at the end of class">;
+def warn_objc_bool_bitfield_has_width_one : Warning<"BOOL bit-field %0 with "
+  " width 1 can not store BOOL's YES value. Change the bitfield type to "
+  "'unsigned' to silence this warning.">, InGroup<ObjCBoolBitfield>;
 def note_next_field_declaration : Note<
   "next field declaration is here">;
 def note_next_ivar_declaration : Note<
   "next %select{instance variable declaration|synthesized instance variable}0"
   " is here">;
 def err_synthesize_variable_sized_ivar : Error<
   "synthesized property with variable size type %0"
   " requires an existing instance variable">;

clang/lib/Sema/SemaDecl.cpp

if (!FieldTy->isDependentType()) {
     // Warn on types where the user might conceivably expect to get all
     // specified bits as value bits: that's all integral types other than
     // 'bool'.
     if (BitfieldIsOverwide && !FieldTy->isBooleanType() && FieldName) {
       Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_width)
           << FieldName << Value.toString(10)
           << (unsigned)TypeWidth;
     }
+
+    // If Objective C's BOOL type is a signed char (or any signed integer), then
+    // 1 bit is not wide enough to store Objective-C's YES(1) value.
+    if (LangOpts.ObjC && Value == 1 && NSAPIObj->isObjCBOOLType(FieldTy) &&
+        FieldTy->isSignedIntegerType())
+      Diag(FieldLoc, diag::warn_objc_bool_bitfield_has_width_one) << FieldName;
   }
 
   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,

clang/test/Misc/warning-wall.c

 CHECK-NEXT:          -Wpotentially-evaluated-expression
 CHECK-NEXT:      -Wunused-variable
 CHECK-NEXT:        -Wunused-const-variable
 CHECK-NEXT:      -Wunused-property-ivar
 CHECK-NEXT:    -Wvolatile-register-var
 CHECK-NEXT:    -Wobjc-missing-super-calls
 CHECK-NEXT:    -Wobjc-designated-initializers
 CHECK-NEXT:    -Wobjc-flexible-array
+CHECK-NEXT:    -Wobjc-bool-bitfield
 CHECK-NEXT:    -Woverloaded-virtual
 CHECK-NEXT:    -Wprivate-extern
 CHECK-NEXT:    -Wcast-of-sel-type
 CHECK-NEXT:    -Wextern-c-compat
 CHECK-NEXT:    -Wuser-defined-warnings
 CHECK-NEXT:  -Wparentheses
 CHECK-NEXT:    -Wlogical-op-parentheses
 CHECK-NEXT:    -Wlogical-not-parentheses

clang/test/Sema/bitfield.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify -std=c11 -Wno-unused-value
 
+typedef signed char BOOL;
+
 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{{width of bit-field 'b' (33 bits) exceeds width of its type (32 bits)}}
 
struct a {
   
   // PR4017  
   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)}}
+
+  // Make sure this doesn't trigger Objective-C's BOOL bitfield warning.
+  BOOL i : 1;
 };
 
 struct b {unsigned x : 2;} x;
 __typeof__(x.x+1) y;
 int y;
 
 struct {unsigned x : 2;} x2;
 __typeof__((x.x+=1)+1) y;

clang/test/SemaObjC/class-bool-bitfield.m

+// RUN: %clang_cc1 %s -fobjc-runtime=macosx-fragile-10.5 -fsyntax-only -verify=signed
+// RUN: %clang_cc1 %s -DBOOL_IS_BOOL -fobjc-runtime=macosx-fragile-10.5 -fsyntax-only -verify=bool
+
+#ifdef BOOL_IS_BOOL
+// If BOOL is _Bool then bitfields with a width of 1 are fine.
+typedef _Bool BOOL;
+#else
+typedef signed char BOOL;
+#endif
+
+typedef BOOL BOOL_TYPEDEF;
+
+@interface X {
+  // If BOOL is a signed char then a 1-bit field isn't wide enough to store
+  // YES(1).
+  BOOL f : 1;         // signed-warning {{BOOL bit-field 'f' with width 1 can not store BOOL's YES value.}}
+  BOOL_TYPEDEF g : 1; // signed-warning {{BOOL bit-field 'g' with width 1 can not store BOOL's YES value.}}
+  // Only warn if Objective-C's BOOL type or a typedef for it is used.
+  signed char h : 1;
+  // A width of 2 or more can store YES and NO.
+  BOOL i : 2; // bool-error {{exceeds width of its type}}
+  BOOL j : 3; // bool-error {{exceeds width of its type}}
+}
+@end
+
+struct S {
+  BOOL f : 1; // signed-warning {{BOOL bit-field 'f' with width 1 can not store BOOL's YES value.}}
+};