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

[Faisal Vali via Phabricator via cfe-commits]

faisalv created this revision.
faisalv added reviewers: aaron.ballman, wchilders, bruno, rnk, BRevzin, thakis.
faisalv added a project: clang.
faisalv requested review of this revision.

Currently clang warns on 'assigning' to an enum bit-field that can not accommodate all its enumerators - but not when such a bit-field is defined.

GCC warns at definition time (https://godbolt.org/z/sKescn) - which seems like a useful thing to do, given certain programmer's propensities for memcpying.

This patch warns when such enum bit-fields are defined - and warns on unnamed bit-fields too (is that the right thing to do here?) building on work done by @rnk here https://github.com/llvm/llvm-project/commit/329f24d6f6e733fcadfd1be7cd3b430d63047c2e

Implementation Strategy:

- add the check, modeled after Reid's check in his patch, into VerifyBitField (checks if the width expression is an ICE and it's a worthy type etc.) that gets called from CheckFieldDecl

Questions for reviewers:

- Should we be emitting such a warning for unnamed bitfields?
- When comparing an APSInt to an unsigned value - should i prefer using the overloaded operator (i.e. Value < BitsNeeded) or stick with BitsNeeded > Value.getZExtValue().

Thank you!


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91651

Files:
  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
@@ -1,3 +1,4 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-windows-msvc -verify %s -Wbitfield-enum-conversion -DFV_UNNAMED_BITFIELD
 // RUN: %clang_cc1 -std=c++11 -triple x86_64-windows-msvc -verify %s -Wbitfield-enum-conversion
 // RUN: %clang_cc1 -std=c++11 -triple x86_64-linux -verify %s -Wbitfield-enum-conversion
 
@@ -57,3 +58,29 @@
   f.two_bits = (unsigned)three_bits_signed;
   f.two_bits = static_cast<unsigned>(three_bits_signed);
 }
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)
+//expected-warning@#1 {{bit-field <unnamed> is not wide enough}} 
+//expected-warning@#2 {{bit-field <unnamed> is not wide enough}} 
+//expected-note@#1 {{widen this field to 63}}
+//expected-note@#2 {{widen this field to 64}}
+#else
+#define NAME(x) x
+//expected-warning@#1 {{bit-field 'b1' is not wide enough}} 
+//expected-warning@#2 {{bit-field 'b2' is not wide enough}} 
+//expected-note@#1 {{widen this field to 63}}
+//expected-note@#2 {{widen this field to 64}}
+#endif
+
+
+enum class E_UNSIGNED_LONG_LONG : long long { PLUS_MAX = 9223372036854775807 };
+enum class E_SIGNED_LONG_LONG : long long { PLUS_MAX = 9223372036854775807, MINUS = -1 };
+
+struct FV {
+  E_UNSIGNED_LONG_LONG NAME(b1) : 5; //#1
+  E_SIGNED_LONG_LONG NAME(b2) : 5;   //#2
+  E_UNSIGNED_LONG_LONG NAME(b3) : (sizeof(long long) * 8) - 1;
+  E_SIGNED_LONG_LONG NAME(b4) : (sizeof(long long) * 8);
+
+};
Index: clang/lib/Sema/SemaDecl.cpp
===================================================================
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16430,6 +16430,38 @@
   }
 
   if (!FieldTy->isDependentType()) {
+    // Check if an enum bit-field has enough bits to accomodate all its
+    // enumerators. Skip the check for an unnamed 0 bit bit-field (alignment
+    // signifier).
+    if (FieldTy->isEnumeralType() && Value != 0) {
+      EnumDecl *ED = FieldTy->castAs<EnumType>()->getDecl();
+
+      const bool IsSignedEnum = ED->getNumNegativeBits() > 0;
+      // Compute the required bitwidth. If the enum has negative values, we need
+      // one more bit than the normal number of positive bits to represent the
+      // sign bit.
+      const unsigned BitsNeeded =
+          IsSignedEnum
+              ? std::max(ED->getNumPositiveBits() + 1, ED->getNumNegativeBits())
+              : ED->getNumPositiveBits();
+      
+      if (BitsNeeded > Value.getZExtValue()) {
+        // TODO: Should we be emitting diagnostics for unnamed bitfields that
+        // can never be assigned to?  Maybe, since they can be memcopied into?
+        if (FieldName) {
+          Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+              << FieldName << ED;
+        } else {
+          Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+              << "<unnamed>" << ED;
+              //<< 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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D91651.305848.patch
Type: text/x-patch
Size: 3423 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201117/5b590f86/attachment.bin>