[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
This revision was automatically updated to reflect the committed changes.
Closed by commit rL297761: Warn on enum assignment to bitfields that can't fit
all values (authored by rnk).
Changed prior to commit:
https://reviews.llvm.org/D30923?vs=91749&id=91752#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30923
Files:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/SemaCXX/warn-bitfield-enum-conversion.cpp
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -8729,13 +8729,66 @@
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()) {
+ EnumDecl *ED = EnumTy->getDecl();
+ bool SignedBitfield = BitfieldType->isSignedIntegerType();
+
+ // Enum types are implicitly signed on Windows, so check if there are any
+ // negative enumerators to see if the enum was intended to be signed or
+ // not.
+ bool SignedEnum = ED->getNumNegativeBits() > 0;
+
+ // Check for surprising sign changes when assigning enum values to a
+ // bitfield of different signedness. If the bitfield is signed and we
+ // have exactly the right number of bits to store this unsigned enum,
+ // suggest changing the enum to an unsigned type. This typically happens
+ // on Windows where unfixed enums always use an underlying type of 'int'.
+ unsigned DiagID = 0;
+ if (SignedEnum && !SignedBitfield) {
+DiagID = diag::warn_unsigned_bitfield_assigned_signed_enum;
+ } else if (SignedBitfield && !SignedEnum &&
+ ED->getNumPositiveBits() == FieldWidth) {
+DiagID = diag::warn_signed_bitfield_enum_conversion;
+ }
+
+ if (DiagID) {
+S.Diag(InitLoc, DiagID) << Bitfield << ED;
+TypeSourceInfo *TSI = Bitfield->getTypeSourceInfo();
+SourceRange TypeRange =
+TSI ? TSI->getTypeLoc().getSourceRange() : SourceRange();
+S.Diag(Bitfield->getTypeSpecStartLoc(), diag::note_change_bitfield_sign)
+<< SignedEnum << TypeRange;
+ }
+
+ // 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.
+ unsigned BitsNeeded = SignedEnum ? std::max(ED->getNumPositiveBits() + 1,
+ ED->getNumNegativeBits())
+ : ED->getNumPositiveBits();
+
+ // Check the bitwidth.
+ if (BitsNeeded > FieldWidth) {
+Expr *WidthExpr = Bitfield->getBitWidth();
+S.Diag(InitLoc, diag::warn_bitfield_too_small_for_enum)
+<< Bitfield << ED;
+S.Diag(WidthExpr->getExprLoc(), diag::note_widen_bitfield)
+<< BitsNeeded << ED << WidthExpr->getSourceRange();
+ }
+}
+
return false;
+ }
unsigned OriginalWidth = Value.getBitWidth();
- unsigned FieldWidth = Bitfield->getBitWidthValue(S.Context);
if (!Value.isSigned() || Value.isNegative())
if (UnaryOperator *UO = dyn_cast(OriginalInit))
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4908,6 +4908,21 @@
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;
+def warn_bitfield_too_small_for_enum : Warning<
+ "bit-field %0 is not wide enough to store all enumerators of %1">,
+ InGroup, DefaultIgnore;
+def note_widen_bitfield : Note<
+ "widen this field to %0 bits to store all values of %1">;
+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, DefaultIgnore;
+def warn_signed_bitfield_enum_conversion : Warning<
+ "signed bit-field %0 needs an extra bit to represent the largest positive "
+ "enumerators of %1">,
+ InGroup, DefaultIgnore;
+def note_change_bitfield_sign : Note<
+ "consider making the bitfield type %select{un
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk marked an inline comment as done. rnk added a comment. Cool, thanks for the reviews, this is definitely in better shape now. This is off by default, so I'm going to commit and experiment with it on a few codebases. I'd still like to hear if either of the Richards have diagnostic wording suggestions, but we can do that in a follow-up. https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
hfinkel added a comment. In https://reviews.llvm.org/D30923#700780, @rnk wrote: > In https://reviews.llvm.org/D30923#700708, @hfinkel wrote: > > > In https://reviews.llvm.org/D30923#700696, @rnk wrote: > > > > > Do you think it's worth indicating that the error can be suppressed with > > > an explicit cast, or is that wasted space? > > > > > > What might this look like? Also, I don't see a regression test for this. > > > The warning only looks through implicit casts and paren exprs, so it could > look like this: `f.two_bits = (unsigned)three_bits;` I added a test for it. Great, thanks! Comment at: lib/Sema/SemaChecking.cpp:8765 +TypeSourceInfo *TSI = Bitfield->getTypeSourceInfo(); +SourceRange TypeRange = TSI ? TSI->getTypeLoc().getSourceRange() : SourceRange(); +S.Diag(Bitfield->getTypeSpecStartLoc(), diag::note_change_bitfield_sign) Line is too long. https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk added a comment. In https://reviews.llvm.org/D30923#700708, @hfinkel wrote: > In https://reviews.llvm.org/D30923#700696, @rnk wrote: > > > Do you think it's worth indicating that the error can be suppressed with an > > explicit cast, or is that wasted space? > > > What might this look like? Also, I don't see a regression test for this. The warning only looks through implicit casts and paren exprs, so it could look like this: `f.two_bits = (unsigned)three_bits;` I added a test for it. https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk updated this revision to Diff 91749.
rnk added a comment.
- Test that explicit casts suppress the warning
https://reviews.llvm.org/D30923
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/SemaCXX/warn-bitfield-enum-conversion.cpp
Index: test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -0,0 +1,59 @@
+// 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
+
+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;
+enum TwoBitsFixed : unsigned { Hi5 = 3 } two_bits_fixed;
+
+struct Foo {
+ unsigned two_bits : 2;// expected-note 2 {{widen this field to 3 bits}} expected-note 2 {{type signed}}
+ int two_bits_signed : 2; // expected-note 2 {{widen this field to 3 bits}} expected-note 1 {{type unsigned}}
+ unsigned three_bits : 3; // expected-note 2 {{type signed}}
+ int three_bits_signed : 3;// expected-note 1 {{type unsigned}}
+
+#ifdef _WIN32
+ // expected-note@+2 {{type unsigned}}
+#endif
+ ThreeBits three_bits_enum : 3;
+ ThreeBits four_bits_enum : 4;
+};
+
+void f() {
+ 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 = two_bits_fixed;
+
+ 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;
+
+#ifdef _WIN32
+ // Enums on Windows are always implicitly 'int', which is signed, so you need
+ // an extra bit to store values that set the MSB. This is not true on SysV
+ // platforms like Linux.
+ // expected-warning@+2 {{needs an extra bit}}
+#endif
+ f.three_bits_enum = three_bits;
+ f.four_bits_enum = three_bits;
+
+ // Explicit casts suppress the warning.
+ f.two_bits = (unsigned)three_bits_signed;
+ f.two_bits = static_cast(three_bits_signed);
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8729,13 +8729,65 @@
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()) {
+ EnumDecl *ED = EnumTy->getDecl();
+ bool SignedBitfield = BitfieldType->isSignedIntegerType();
+
+ // Enum types are implicitly signed on Windows, so check if there are any
+ // negative enumerators to see if the enum was intended to be signed or
+ // not.
+ bool SignedEnum = ED->getNumNegativeBits() > 0;
+
+ // Check for surprising sign changes when assigning enum values to a
+ // bitfield of different signedness. If the bitfield is signed and we
+ // have exactly the right number of bits to store this unsigned enum,
+ // suggest changing the enum to an unsigned type. This typically happens
+ // on Windows where unfixed enums always use an underlying type of 'int'.
+ unsigned DiagID = 0;
+ if (SignedEnum && !SignedBitfield) {
+DiagID = diag::warn_unsigned_bitfield_assigned_signed_enum;
+ } else if (SignedBitfield && !SignedEnum &&
+ ED->getNumPositiveBits() == FieldWidth) {
+DiagID = diag::warn_signed_bitfield_enum_conversion;
+ }
+
+ if (DiagID) {
+S.Diag(InitLoc, DiagID) << Bitfiel
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
hfinkel added a comment. In https://reviews.llvm.org/D30923#700696, @rnk wrote: > In https://reviews.llvm.org/D30923#700626, @hfinkel wrote: > > > In https://reviews.llvm.org/D30923#700620, @thakis wrote: > > > > > Maybe it should have some "to suppress the warning, do X" fixit? > > > > > > I think we should at least include how many bits the field should have to > > fix the problem (pointing to the relevant field definition certainly seems > > helpful). > > > Agreed, I was just hacking that together. :) > > Do you think it's worth indicating that the error can be suppressed with an > explicit cast, or is that wasted space? What might this look like? Also, I don't see a regression test for this. https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk updated this revision to Diff 91741.
rnk added a comment.
- Beef up test as Nico suggests
- Add two notes, one to change the bitfield signedness, one to indicate the
required bitwidth
https://reviews.llvm.org/D30923
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/SemaCXX/warn-bitfield-enum-conversion.cpp
Index: test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -0,0 +1,55 @@
+// 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
+
+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;
+enum TwoBitsFixed : unsigned { Hi5 = 3 } two_bits_fixed;
+
+struct Foo {
+ unsigned two_bits : 2;// expected-note 2 {{widen this field to 3 bits}} expected-note 2 {{type signed}}
+ int two_bits_signed : 2; // expected-note 2 {{widen this field to 3 bits}} expected-note 1 {{type unsigned}}
+ unsigned three_bits : 3; // expected-note 2 {{type signed}}
+ int three_bits_signed : 3;// expected-note 1 {{type unsigned}}
+
+#ifdef _WIN32
+ // expected-note@+2 {{type unsigned}}
+#endif
+ ThreeBits three_bits_enum : 3;
+ ThreeBits four_bits_enum : 4;
+};
+
+void f() {
+ 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 = two_bits_fixed;
+
+ 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;
+
+#ifdef _WIN32
+ // Enums on Windows are always implicitly 'int', which is signed, so you need
+ // an extra bit to store values that set the MSB. This is not true on SysV
+ // platforms like Linux.
+ // expected-warning@+2 {{needs an extra bit}}
+#endif
+ f.three_bits_enum = three_bits;
+ f.four_bits_enum = three_bits;
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8729,13 +8729,65 @@
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()) {
+ EnumDecl *ED = EnumTy->getDecl();
+ bool SignedBitfield = BitfieldType->isSignedIntegerType();
+
+ // Enum types are implicitly signed on Windows, so check if there are any
+ // negative enumerators to see if the enum was intended to be signed or
+ // not.
+ bool SignedEnum = ED->getNumNegativeBits() > 0;
+
+ // Check for surprising sign changes when assigning enum values to a
+ // bitfield of different signedness. If the bitfield is signed and we
+ // have exactly the right number of bits to store this unsigned enum,
+ // suggest changing the enum to an unsigned type. This typically happens
+ // on Windows where unfixed enums always use an underlying type of 'int'.
+ unsigned DiagID = 0;
+ if (SignedEnum && !SignedBitfield) {
+DiagID = diag::warn_unsigned_bitfield_assigned_signed_enum;
+ } else if (SignedBitfield && !SignedEnum &&
+ ED->getNumPositiveBits() == FieldWidth) {
+DiagID = diag::warn_signed_bitfield_enum_conversion;
+ }
+
+ if (DiagID) {
+S.Diag(InitLoc, DiagID) << Bitfield << ED;
+TypeSourceInfo *TSI = Bitfield->getTypeSo
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk added a comment. In https://reviews.llvm.org/D30923#700626, @hfinkel wrote: > In https://reviews.llvm.org/D30923#700620, @thakis wrote: > > > Maybe it should have some "to suppress the warning, do X" fixit? > > > I think we should at least include how many bits the field should have to fix > the problem (pointing to the relevant field definition certainly seems > helpful). Agreed, I was just hacking that together. :) Do you think it's worth indicating that the error can be suppressed with an explicit cast, or is that wasted space? https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
hfinkel added a comment. In https://reviews.llvm.org/D30923#700620, @thakis wrote: > Maybe it should have some "to suppress the warning, do X" fixit? I think we should at least include how many bits the field should have to fix the problem (pointing to the relevant field definition certainly seems helpful). https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
thakis added a comment. Maybe it should have some "to suppress the warning, do X" fixit? https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Looks like a cool warning. Two suggestions for more exhaustive testing, but I think this looks good. Comment at: test/SemaCXX/warn-bitfield-enum-conversion.cpp:1 +// RUN: %clang_cc1 -std=c++11 -triple x86_64-windows-msvc -verify %s -Wbitfield-enum-conversion + Consider also running this test with a triple where enums don't default to signed. Comment at: test/SemaCXX/warn-bitfield-enum-conversion.cpp:15 + + ThreeBits three_bits_enum : 3; +}; Also add a : 4 version and check that that doesn't warn? https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk updated this revision to Diff 91653.
rnk marked an inline comment as done.
rnk added a comment.
- Actually make this warning off by default
https://reviews.llvm.org/D30923
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/SemaCXX/warn-bitfield-enum-conversion.cpp
Index: test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -std=c++11 -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;
+enum TwoBitsFixed : unsigned { Hi5 = 3 } two_bits_fixed;
+
+struct Foo {
+ unsigned two_bits : 2;
+ int two_bits_signed : 2;
+ unsigned three_bits : 3;
+ int three_bits_signed : 3;
+
+ ThreeBits three_bits_enum : 3;
+};
+
+void f() {
+ 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 = two_bits_fixed;
+
+ 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()) {
+ 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(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;
+def warn_bitfield_too_small_for_enum: Warning<
+ "bit-field %0 is not wide enough to store all enumerators of enum type %1">,
+ InGroup, DefaultIgnore;
+def warn_unsigned_bitfield_assigned_signed_enum: Warning<
+ "assigning value of signed enum type %1
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk marked an inline comment as done.
rnk added inline comments.
Comment at: test/Sema/warn-bitfield-enum-conversion.c:3
+
+enum TwoBits { Hi1 = 3 } two_bits;
+enum TwoBitsSigned { Lo2 = -2, Hi2 = 1 } two_bits_signed;
thakis wrote:
> can you add an enum with an explicit underlying type? will this warning
> always fire for those?
I was worried for a moment, but it looks like `EnumDecl::getNumPositiveBits`
does what I want and ignores the fixed underlying type.
https://reviews.llvm.org/D30923
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
rnk updated this revision to Diff 91652.
rnk added a comment.
- Make test C++, add fixed type enum
https://reviews.llvm.org/D30923
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/SemaCXX/warn-bitfield-enum-conversion.cpp
Index: test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -std=c++11 -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;
+enum TwoBitsFixed : unsigned { Hi5 = 3 } two_bits_fixed;
+
+struct Foo {
+ unsigned two_bits : 2;
+ int two_bits_signed : 2;
+ unsigned three_bits : 3;
+ int three_bits_signed : 3;
+
+ ThreeBits three_bits_enum : 3;
+};
+
+void f() {
+ 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 = two_bits_fixed;
+
+ 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()) {
+ 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(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;
+def warn_bitfield_too_small_for_enum: Warning<
+ "bit-field %0 is not wide enough to store all enumerators of enum type %1">,
+ InGroup;
+def warn_unsigned_bitfield_assigned_signed_enum: Warning<
+ "assigning value of signed enum type %1 to unsigned bit-field %0; "
+ "negative enumerators of enum
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
thakis added inline comments.
Comment at: test/Sema/warn-bitfield-enum-conversion.c:3
+
+enum TwoBits { Hi1 = 3 } two_bits;
+enum TwoBitsSigned { Lo2 = -2, Hi2 = 1 } two_bits_signed;
can you add an enum with an explicit underlying type? will this warning always
fire for those?
https://reviews.llvm.org/D30923
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values
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()) {
+ 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.
+
