[Sema] Validate large bitfields

[Mordante]

Summary

Note: I'm not entirely happy with the text of the diagnostics and I'm open to suggestions.

Fixes PR23505: Large bitfield: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5186–5189

I feel like this situation should be an error rather than a warning -- what could the code possibly have meant?

clang/lib/Sema/SemaDecl.cpp

15845

too wide -> too-wide
bit-fields they -> bit-fields, they

[rsmith]

It seems fine and reasonable to reject this as a violation of an implementation limit. Can we actually support the full range 2^64 bits range, or would it be wiser to pick a smaller limit? (There's at least no point in allowing bit-widths that would mean the size of the bitfield doesn't fit in size_t, which means no more than 35 bits for a 32-bit target.)

clang/include/clang/Basic/DiagnosticSemaKinds.td

5186–5189

The name of the diagnostic and the kind of diagnostic should agree. Currently we have warn_ vs Error<.

[Mordante]

I'll have a look whether I can find a sane maximum width for a bit-field.

[aaron.ballman]

In D71142#1773933, @Mordante wrote:

I'll have a look whether I can find a sane maximum width for a bit-field.

The limits according to C are the size of the field's declared type (C17 6.7.2.1p4), but it seems that C++ decided it would make sense to turn the bits into padding bits (http://eel.is/c++draft/class.bit#1.sentence-6), so I guess a reasonable implementation limit would be SIZE_MAX for the target architecture (aka, sizeof(size_t) * CHAR_BIT bits).

clang/include/clang/Basic/DiagnosticSemaKinds.td

5186–5189

Ooof, good catch! These diagnostics should be renamed to start with err_ instead.

[Mordante]

Further testing revealed the Codegen already has decided the limit.
clang/lib/CodeGen/CGRecordLayout.h:64:

struct CGBitFieldInfo {
  /// The offset within a contiguous run of bitfields that are represented as
  /// a single "field" within the LLVM struct type. This offset is in bits.
  unsigned Offset : 16;

  /// The total size of the bit-field, in bits.
  unsigned Size : 15;

So I'll look at using that limit. I also want to look at improving the diagnostics a bit more.

clang/include/clang/Basic/DiagnosticSemaKinds.td

5186–5189

Hmm odd I'm sure I left a comment here yesterday, but it seems I didn't commit it properly.
It is a copy paste bug and I forgot to change the warn_ prefix to err_.

[xbolva00]

Maybe a bit offtopic, but it would be good to diagnose this case too:

enum A {a, b, c = 8};

struct T {
    enum A field : 2;
};

GCC catches this bug.

[Mordante]

In D71142#1774270, @xbolva00 wrote:

Maybe a bit offtopic, but it would be good to diagnose this case too:

enum A {a, b, c = 8};

struct T {
    enum A field : 2;
};

GCC catches this bug.

I created https://bugs.llvm.org/show_bug.cgi?id=44251 and will look at adding this warning.

[xbolva00]

Any comments?

@rsmith @aaron.ballman

[aaron.ballman]

In D71142#1967315, @xbolva00 wrote:

Any comments?

@rsmith @aaron.ballman

There are outstanding review comments, so I'm waiting for the patch author to address those, unless I've missed a question somewhere?

[Mordante]

In D71142#1967409, @aaron.ballman wrote:

In D71142#1967315, @xbolva00 wrote:

Any comments?

@rsmith @aaron.ballman

There are outstanding review comments, so I'm waiting for the patch author to address those, unless I've missed a question somewhere?

No I still need to address some issues, but I haven't found time for it yet.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_bitfield_width_exceeds_type_width : Error<
   "of its type (%3 bit%s3)">;
 def err_anon_bitfield_width_exceeds_type_width : Error<
   "width of anonymous bit-field (%0 bits) exceeds %select{width|size}1 "
   "of its type (%2 bit%s2)">;
 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_maximum_width: Error<
+  "width of bit-field %0 doesn't fit in a 64 bit unsigned integer">;
+def warn_anon_bitfield_width_exceeds_maximum_width : Error<
+  "width of anonymous bit-field doesn't fit in a 64 bit unsigned integer">;
 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_bitfield_too_small_for_enum : Warning<
   "bit-field %0 is not wide enough to store all enumerators of %1">,

clang/lib/Sema/SemaDecl.cpp

return Diag(FieldLoc, diag::err_anon_bitfield_has_negative_width)
       << Value.toString(10);
   }
 
   if (!FieldTy->isDependentType()) {
     uint64_t TypeStorageSize = Context.getTypeSize(FieldTy);
     uint64_t TypeWidth = Context.getIntWidth(FieldTy);
     bool BitfieldIsOverwide = Value.ugt(TypeWidth);
 
+    // Don't accept too wide bit-fields they will cause assertion failures
+    // when used.
+    if (BitfieldIsOverwide && Value.ugt(UINT64_MAX)) {
+      if (FieldName)
+        return Diag(FieldLoc, diag::warn_bitfield_width_exceeds_maximum_width)
+               << FieldName;
+
+      return Diag(FieldLoc,
+                  diag::warn_anon_bitfield_width_exceeds_maximum_width);
+    }
+
     // Over-wide bitfields are an error in C or when using the MSVC bitfield
     // ABI.
     bool CStdConstraintViolation =
         BitfieldIsOverwide && !getLangOpts().CPlusPlus;
     bool MSBitfieldViolation =
         Value.ugt(TypeStorageSize) &&
         (IsMsStruct || Context.getTargetInfo().getCXXABI().isMicrosoft());
     if (CStdConstraintViolation || MSBitfieldViolation) {

clang/test/Sema/bitfield.c

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)}}
+
+  // PR23505
+  _Bool : 1 + (unsigned __int128)0xffffffffffffffff;   // expected-error {{width of anonymous bit-field doesn't fit in a 64 bit unsigned integer}}
+  _Bool i : 1 + (unsigned __int128)0xffffffffffffffff; // expected-error {{width of bit-field 'i' doesn't fit in a 64 bit unsigned integer}}
+  int : 1 + (unsigned __int128)0xffffffffffffffff;     // expected-error {{width of anonymous bit-field doesn't fit in a 64 bit unsigned integer}}
+  int j : 1 + (unsigned __int128)0xffffffffffffffff;   // expected-error {{width of bit-field 'j' doesn't fit in a 64 bit unsigned integer}}
 };
 
 struct b {unsigned x : 2;} x;
 __typeof__(x.x+1) y;
 int y;
 
 struct {unsigned x : 2;} x2;
 __typeof__((x.x+=1)+1) y;