rnk created this revision.
This adds -Wbitfield-enum-conversion, which warns on implicit
conversions that happen on bitfield assignment that change the value of
some enumerators.
Values of enum type typically take on a very small range of values, so
they are frequently stored in bitfields. Unfortunately, there is no
convenient way to calculate the minimum number of bits necessary to
store all possible values at compile time, so users usually hard code a
bitwidth that works today and widen it as necessary to pass basic
testing and validation. This is very error-prone, and leads to stale
widths as enums grow. This warning aims to catch such bugs.
This would have found two real bugs in clang and two instances of
questionable code. See r297680 and r297654 for the full description of
the issues.
This warning is currently disabled by default while we investigate its
usefulness outside of LLVM.
The major cause of false positives with this warning is this kind of
enum:
enum E { W, X, Y, Z, SENTINEL_LAST };
The last enumerator is an invalid value used to validate inputs or size
an array. Depending on the prevalance of this style of enum across a
codebase, this warning may be more or less feasible to deploy. It also
has trouble on sentinel values such as ~0U.
https://reviews.llvm.org/D30923
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/Sema/warn-bitfield-enum-conversion.c
Index: test/Sema/warn-bitfield-enum-conversion.c
===================================================================
--- /dev/null
+++ test/Sema/warn-bitfield-enum-conversion.c
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -verify %s -Wbitfield-enum-conversion
+
+enum TwoBits { Hi1 = 3 } two_bits;
+enum TwoBitsSigned { Lo2 = -2, Hi2 = 1 } two_bits_signed;
+enum ThreeBits { Hi3 = 7 } three_bits;
+enum ThreeBitsSigned { Lo4 = -4, Hi4 = 3 } three_bits_signed;
+
+struct Foo {
+ unsigned two_bits : 2;
+ int two_bits_signed : 2;
+ unsigned three_bits : 3;
+ int three_bits_signed : 3;
+
+ enum ThreeBits three_bits_enum : 3;
+};
+
+void f() {
+ struct Foo f;
+
+ f.two_bits = two_bits;
+ f.two_bits = two_bits_signed; // expected-warning {{negative enumerators}}
+ f.two_bits = three_bits; // expected-warning {{not wide enough}}
+ f.two_bits = three_bits_signed; // expected-warning {{negative enumerators}} expected-warning {{not wide enough}}
+
+ f.two_bits_signed = two_bits; // expected-warning {{needs an extra bit}}
+ f.two_bits_signed = two_bits_signed;
+ f.two_bits_signed = three_bits; // expected-warning {{not wide enough}}
+ f.two_bits_signed = three_bits_signed; // expected-warning {{not wide enough}}
+
+ f.three_bits = two_bits;
+ f.three_bits = two_bits_signed; // expected-warning {{negative enumerators}}
+ f.three_bits = three_bits;
+ f.three_bits = three_bits_signed; // expected-warning {{negative enumerators}}
+
+ f.three_bits_signed = two_bits;
+ f.three_bits_signed = two_bits_signed;
+ f.three_bits_signed = three_bits; // expected-warning {{needs an extra bit}}
+ f.three_bits_signed = three_bits_signed;
+
+ f.three_bits_enum = three_bits; // expected-warning {{needs an extra bit}}
+}
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8729,13 +8729,42 @@
return false;
Expr *OriginalInit = Init->IgnoreParenImpCasts();
+ unsigned FieldWidth = Bitfield->getBitWidthValue(S.Context);
llvm::APSInt Value;
- if (!OriginalInit->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects))
+ if (!OriginalInit->EvaluateAsInt(Value, S.Context,
+ Expr::SE_AllowSideEffects)) {
+ // The RHS is not constant. If the RHS has an enum type, make sure the
+ // bitfield is wide enough to hold all the values of the enum without
+ // truncation.
+ if (const auto *EnumTy = OriginalInit->getType()->getAs<EnumType>()) {
+ EnumDecl *ED = EnumTy->getDecl();
+ bool SignedBitfield = BitfieldType->isSignedIntegerType();
+
+ // Check if this might store negative values into an unsigned bitfield.
+ if (!SignedBitfield && ED->getNumNegativeBits() > 0) {
+ S.Diag(InitLoc, diag::warn_unsigned_bitfield_assigned_signed_enum)
+ << Bitfield << ED;
+ }
+
+ // Check for sufficient width.
+ if (ED->getNumPositiveBits() > FieldWidth ||
+ ED->getNumNegativeBits() > FieldWidth) {
+ S.Diag(InitLoc, diag::warn_bitfield_too_small_for_enum)
+ << Bitfield << ED;
+ } else if (SignedBitfield && ED->getNumPositiveBits() == FieldWidth) {
+ // If the bitfield type is signed and the positive field widths are
+ // equal, values with the high bit set will become negative, which is
+ // usually unintended.
+ S.Diag(InitLoc, diag::warn_signed_bitfield_enum_conversion)
+ << Bitfield << ED;
+ }
+ }
+
return false;
+ }
unsigned OriginalWidth = Value.getBitWidth();
- unsigned FieldWidth = Bitfield->getBitWidthValue(S.Context);
if (!Value.isSigned() || Value.isNegative())
if (UnaryOperator *UO = dyn_cast<UnaryOperator>(OriginalInit))
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -4908,6 +4908,17 @@
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 enum type %1">,
+ InGroup<BitFieldEnumConversion>;
+def warn_unsigned_bitfield_assigned_signed_enum: Warning<
+ "assigning value of signed enum type %1 to unsigned bit-field %0; "
+ "negative enumerators of enum %1 will be converted to positive values">,
+ InGroup<BitFieldEnumConversion>;
+def warn_signed_bitfield_enum_conversion: Warning<
+ "signed bit-field %0 needs an extra bit to represent the largest positive "
+ "values of enum type %1">,
+ InGroup<BitFieldEnumConversion>;
def warn_missing_braces : Warning<
"suggest braces around initialization of subobject">,
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -34,6 +34,7 @@
def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
+def BitFieldEnumConversion : DiagGroup<"bitfield-enum-conversion">;
def BitFieldWidth : DiagGroup<"bitfield-width">;
def ConstantConversion :
DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
@@ -606,6 +607,7 @@
[BoolConversion,
ConstantConversion,
EnumConversion,
+ BitFieldEnumConversion,
FloatConversion,
Shorten64To32,
IntConversion,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
