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);
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);
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
