[PATCH] D91651: [clang] Add a warning (à la gcc) for too small enum bitfields

[Faisal Vali via Phabricator via cfe-commits]

faisalv updated this revision to Diff 305970.
faisalv added a comment.

Based on Richards Feedback, this update includes the following changes:

- avoids calling the fragile getZextValue() for comparing against bit-field width, and uses APSInt's comparison operator overload
- suppresses/avoids the warning for unnamed bit-fields
- simplifies the test case and avoids preprocessor cleverness (and thus an extra pass).

Thank you for taking the time to review this Richard!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91651/new/

https://reviews.llvm.org/D91651

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp


Index: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
===================================================================
--- clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
+++ clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -57,3 +57,18 @@
   f.two_bits = (unsigned)three_bits_signed;
   f.two_bits = static_cast<unsigned>(three_bits_signed);
 }
+
+enum class E_UNSIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long long) * 8 - 2)) };
+enum class E_SIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long long) * 8 - 2)), MINUS = -1 };
+
+struct E64 {
+  E_UNSIGNED_LONG_LONG b3 : (sizeof(long long) * 8) - 1; // OK.
+  E_SIGNED_LONG_LONG   b4 : (sizeof(long long) * 8);     // OK.
+
+  E_UNSIGNED_LONG_LONG b1 : 5; //expected-warning {{bit-field 'b1' is not wide enough}} expected-note {{widen this field to 63}}
+  E_SIGNED_LONG_LONG   b2 : 5; //expected-warning {{bit-field 'b2' is not wide enough}} expected-note {{widen this field to 64}}
+  
+  E_UNSIGNED_LONG_LONG : 5; // OK.  no warning for unnamed bit fields.
+  E_SIGNED_LONG_LONG   : 5; // OK.  no warning for unnamed bit fields.
+  
+};
Index: clang/lib/Sema/SemaDecl.cpp
===================================================================
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16430,6 +16430,22 @@
   }
 
   if (!FieldTy->isDependentType()) {
+    // Check if a named enum bit-field has enough bits to accomodate all its
+    // enumerators. We can ignore unnamed enum bit-fields since they do not
+    // represent values.
+    if (FieldName && FieldTy->isEnumeralType()) {
+      const EnumDecl *const ED = FieldTy->castAs<EnumType>()->getDecl();
+
+      const auto BitsNeeded = ED->getTotalBitsNeeded();
+      if (BitsNeeded > Value) {
+        Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+            << FieldName << ED;
+
+        Diag(BitWidth->getExprLoc(), diag::note_widen_bitfield)
+            << BitsNeeded << ED << BitWidth->getSourceRange();
+      }
+    }
+
     uint64_t TypeStorageSize = Context.getTypeSize(FieldTy);
     uint64_t TypeWidth = Context.getIntWidth(FieldTy);
     bool BitfieldIsOverwide = Value.ugt(TypeWidth);
Index: clang/include/clang/AST/Decl.h
===================================================================
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -3723,6 +3723,14 @@
   ///                     -101     1001011                     8
   unsigned getNumNegativeBits() const { return EnumDeclBits.NumNegativeBits; }
 
+  /// Returns the least width in bits required to store all the enumerators.
+  unsigned getTotalBitsNeeded() const {
+    // If the enum has negative values, we need one more bit than the normal
+    // number of positive bits to represent the sign bit.
+    return getNumNegativeBits() > 0
+               ? std::max(getNumPositiveBits() + 1, getNumNegativeBits())
+               : getNumPositiveBits();
+  }
   /// Returns true if this is a C++11 scoped enumeration.
   bool isScoped() const { return EnumDeclBits.IsScoped; }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D91651.305970.patch
Type: text/x-patch
Size: 3071 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201118/acc210c6/attachment-0001.bin>