sashab updated this revision to Diff 70760.
sashab marked an inline comment as done.
sashab added a comment.
Thanks for your feedback everyone; left the flag as DefaultIgnore but added it
to -Wall.
Keep in mind I am planning on adding two additional warnings after this, namely
"%0 is too small to hold all values of %1"
and
"%0 is too large to hold all values of %1"
for all enum bitfields.
Also, just going back to rnk's original comment, I don't think I properly
replied -- you are correct that the following code behaves strangely:
enum E : signed { E1, E2 };
struct { E e1 : 1; E e2; F f1 : 1; F f2; } s;
s.e2 = E2; // comes out as -1 instead of 1
If you are storing signed enums in a bitfield, you need 1 additional bit to
store the sign bit otherwise the enums will not serialize back correctly. This
is equivalent to storing unsigned enums in bitfields that are too small; the
whole value is not stored.
This is handled by the warning I'm adding next "%0 is too small to hold all
values of %1", which can fire for signed enum bitfields when you don't store
max(numberOfBitsNeededForUnsignedValues, numberOfBitsNeededForSignedValues) + 1.
https://reviews.llvm.org/D24289
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/SemaCXX/warn-msvc-enum-bitfield.cpp
Index: test/SemaCXX/warn-msvc-enum-bitfield.cpp
===================================================================
--- test/SemaCXX/warn-msvc-enum-bitfield.cpp
+++ test/SemaCXX/warn-msvc-enum-bitfield.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -fsyntax-only -Wsigned-enum-bitfield -verify %s --std=c++11
+
+// Enums used in bitfields with no explicitly specified underlying type.
+void test0() {
+ enum E { E1, E2 };
+ enum F { F1, F2 };
+ struct { E e1 : 1; E e2; F f1 : 1; F f2; } s;
+
+ s.e1 = E1; // expected-warning {{enums in the Microsoft ABI are signed integers by default; consider giving the enum E an unsigned underlying type to make this code portable}}
+ s.f1 = F1; // expected-warning {{enums in the Microsoft ABI are signed integers by default; consider giving the enum F an unsigned underlying type to make this code portable}}
+
+ s.e2 = E2;
+ s.f2 = F2;
+}
+
+// Enums used in bitfields with an explicit signed underlying type.
+void test1() {
+ enum E : signed { E1, E2 };
+ enum F : long { F1, F2 };
+ struct { E e1 : 1; E e2; F f1 : 1; F f2; } s;
+
+ s.e1 = E1;
+ s.f1 = F1;
+
+ s.e2 = E2;
+ s.f2 = F2;
+}
+
+// Enums used in bitfields with an explicitly unsigned underlying type.
+void test3() {
+ enum E : unsigned { E1, E2 };
+ enum F : unsigned long { F1, F2 };
+ struct { E e1 : 1; E e2; F f1 : 1; F f2; } s;
+
+ s.e1 = E1;
+ s.f1 = F1;
+
+ s.e2 = E2;
+ s.f2 = F2;
+}
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7828,6 +7828,24 @@
return false;
// White-list bool bitfields.
+ QualType BitfieldType = Bitfield->getType();
+ if (BitfieldType->isBooleanType())
+ return false;
+
+ if (BitfieldType->isEnumeralType()) {
+ EnumDecl *BitfieldEnumDecl = BitfieldType->getAs<EnumType>()->getDecl();
+ // If the underlying enum type was not explicitly specified as an unsigned
+ // type and the enum contain only positive values, MSVC++ will cause an
+ // inconsistency by storing this as a signed type.
+ if (S.getLangOpts().CPlusPlus11 &&
+ !BitfieldEnumDecl->getIntegerTypeSourceInfo() &&
+ BitfieldEnumDecl->getNumPositiveBits() > 0 &&
+ BitfieldEnumDecl->getNumNegativeBits() == 0) {
+ S.Diag(InitLoc, diag::warn_no_underlying_type_specified_for_enum_bitfield)
+ << BitfieldEnumDecl->getNameAsString();
+ }
+ }
+
if (Bitfield->getType()->isBooleanType())
return false;
@@ -7858,7 +7876,7 @@
// Compute the value which the bitfield will contain.
llvm::APSInt TruncatedValue = Value.trunc(FieldWidth);
- TruncatedValue.setIsSigned(Bitfield->getType()->isSignedIntegerType());
+ TruncatedValue.setIsSigned(BitfieldType->isSignedIntegerType());
// Check whether the stored value is equal to the original value.
TruncatedValue = TruncatedValue.extend(OriginalWidth);
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2956,6 +2956,10 @@
"cast to %1 from smaller integer type %0">,
InGroup<IntToVoidPointerCast>;
+def warn_no_underlying_type_specified_for_enum_bitfield : Warning<
+ "enums in the Microsoft ABI are signed integers by default; consider giving "
+ "the enum %0 an unsigned underlying type to make this code portable">,
+ InGroup<DiagGroup<"signed-enum-bitfield">>, DefaultIgnore;
def warn_attribute_packed_for_bitfield : Warning<
"'packed' attribute was ignored on bit-fields with single-byte alignment "
"in older versions of GCC and Clang">,
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -535,6 +535,7 @@
def CharSubscript : DiagGroup<"char-subscripts">;
def LargeByValueCopy : DiagGroup<"large-by-value-copy">;
def DuplicateArgDecl : DiagGroup<"duplicate-method-arg">;
+def SignedEnumBitfield : DiagGroup<"signed-enum-bitfield">;
// Unreachable code warning groups.
//
@@ -651,6 +652,7 @@
ReturnType,
SelfAssignment,
SelfMove,
+ SignedEnumBitfield,
SizeofArrayArgument,
SizeofArrayDecay,
StringPlusInt,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
