Add warning when assigning enums to bitfields without an explicit unsigned underlying type

[• sashab]

Summary

Add a warning when assigning enums to bitfields without an explicit unsigned underlying type. This is to prevent problems with MSVC compatibility, since the Microsoft ABI defaults to storing enums with a signed type, causing inconsistencies with saving to/reading from bitfields.

Also disabled the warning in the dr0xx.cpp test which throws the error, and added a test for the warning.

The warning can be disabled with -Wno-signed-enum-bitfield.

[rnk]

include/clang/Basic/DiagnosticSemaKinds.td

2962 ↗

(On Diff #70500)

I think this should be DefaultIgnore, since it is only relevant for projects that care about cross-platform portability.

lib/Sema/SemaChecking.cpp

7841 ↗

(On Diff #70500)

Are we sure we want it to behave this way? The test case you already have looks problematic:

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

Did you want to warn on the assignment instead of the bitfield?

[• sashab]

Changed warning to be DefaultIgnore

[• sashab]

lib/Sema/SemaChecking.cpp

7841 ↗

(On Diff #70500)

Yes, but this is fine because in this case the behavior is consistent with clang (which also prints -1)

If you run the code below on msvc and clang (which you can do here - http://rextester.com/l/cpp_online_compiler_visual)

#include <iostream>
using namespace std;

int main()
{
    enum E : signed { E1, E2 };
    struct { E e1 : 1; E e2; } s;
    s.e1 = E2;
    cout << s.e1 << endl;
}

It prints -1 with both MSVC and Clang, but if you removed the ': signed' from E it prints -1 with MSVC and 1 with Clang, which is the dangerous inconsistency. We're assuming here that if the developer specified a signed underlying type then they know what they are doing. :)

And yes, the warning is on the assignment, which is correct.

[rnk]

lgtm

lib/Sema/SemaChecking.cpp

7841 ↗

(On Diff #70652)

Got it.

[thakis]

include/clang/Basic/DiagnosticSemaKinds.td

2962 ↗

(On Diff #70652)

Hm, isn't the more safe default to have it on by default? A "what you're doing isn't portable" warning seems useful, and people who don't care can easily turn it off. And if it's not off by default, it's hard for people who do care to turn it on (they'll have to know about the warning).

Maybe it should be off-by-default but part of -Wall?

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

2962 ↗

(On Diff #70652)

I would also vote for this being on by default rather than off by default. We usually try to avoid adding new, off-by-default warnings because they are basically undiscoverable warnings that no one enables (unless we're adding them for GCC compatibility and GCC default ignores). I would be fine if it was only enabled with -Wall.

[rnk]

I was thinking of suggesting to put it under -Wextra, but -Wall is fine too.

I still don't think it should be on by default. I think there are a lot of Unix-only projects out there that aren't concerned with MSVC compatibility, and a warning about how things with in the Microsoft ABI would be surprising and noisy. Maybe we could tack on "... to make this code portable" or something to the diagnostic text to make it more clear why the user should be concerned about this?

[• sashab]

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.

[• sashab]

Pinging for another LGTM since I have added it to -Wall and added a portability comment to the error message :)

[thakis]

Works for me if rnk likes it :-)

(We could bikeshed on if this should be one of the -Wmicrosoft warnings, but the warning can't be both in -Wall and -Wmicrosoft, so let's don't).

I do have a question about the test (the first commit below):

test/SemaCXX/warn-msvc-enum-bitfield.cpp

12 ↗

(On Diff #70760)

Shouldn't this be the version that warns? The assignment with E1 assigns 0 which is portable, but this assigns 1 which overflows, right?

39 ↗

(On Diff #70760)

We have -Wconversion warnings for similar cases; maybe we should have this here too (but not in this change):

Nicos-MBP:llvm-build thakis$ cat test2.cc
enum E {e1, e2};

struct S {

E e1 : 1;

};
void f() {

short s1 = 65536;
short s2 = 32769;

S s;
s.e1 = e2;

}
Nicos-MBP:llvm-build thakis$ clang -Wconversion -c test2.cc
test2.cc:8:14: warning: implicit conversion changes signedness: 'int' to 'short' [-Wsign-conversion]

short s2 = 32769;
      ~~   ^~~~~

test2.cc:7:14: warning: implicit conversion from 'int' to 'short' changes value from 65536 to 0 [-Wconstant-conversion]

short s1 = 65536;
      ~~   ^~~~~

2 warnings generated.

[rnk]

In D24289#543874, @thakis wrote:

Works for me if rnk likes it :-)

Yep, looks good.

(We could bikeshed on if this should be one of the -Wmicrosoft warnings, but the warning can't be both in -Wall and -Wmicrosoft, so let's don't).

I also don't think it's a good fit for -Wmicrosoft, which is usually trying to warn you that "hey, this is an MS extension!", and not "hey, your Unix code isn't portable to Windows!".

[aaron.ballman]

LGTM as well, thank you!

[• sashab]

Thanks all! :)

test/SemaCXX/warn-msvc-enum-bitfield.cpp

12 ↗

(On Diff #70760)

e2 is not a bitfield :) So this code is fine.

And we should warn on all assignments, since any assigned value could potentially be incorrect. Also, most assignments are not static so we don't always have that information :)

[• sashab]

Is this how I commit this? Hopefully this lands... :-)

[thakis]

test/SemaCXX/warn-msvc-enum-bitfield.cpp

12 ↗

(On Diff #70760)

Do you have any data on false positive rate when we just always warn unconditionally? i.e. did you build some large-ish open-source program (clang, chromium, firefox, ...) and is the true positive / false positive rate ok? (we have some problems with "always warn" with other enum warning which we currently always have to disable, e.g. https://bugs.chromium.org/p/chromium/issues/detail?id=621097)

To land, you need to svn commit or ask someone with commit access to do that for you -- llvm doesn't have a commit queue.

[mehdi_amini]

Reopen: this hasn't been committed yet apparently.

@sashab : are you still interested in moving this forward?

[• sashab]

Sorry, had this discussion elsewhere (https://bugs.chromium.org/p/chromium/issues/detail?id=648462).

I'm uncertain at this point. There is currently a bug in GCC that means enums with an explicit underlying type (or enum classes, although the latter is to be fixed) are given the size of their underlying type. For example:

enum Foo { A, B };

can be stored in a bitfield of size 1, but:

enum Foo : unsigned char { A, B };

will error in a bitfield of any size smaller than 8.

So when this modification tells the developer to add 'unsigned' to their enum, they are subsequently causing a warning to occur in GCC.

I have commented on the bug on GCC for this (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242#c28), but it looks unlikely to be fixed.

Should we go ahead and add this warning when following its instructions will cause a warning in the GCC compiler? Even though GCC is at fault here, I'm not sure what the right thing is to do.

[mehdi_amini]

So when this modification tells the developer to add 'unsigned' to their enum, they are subsequently causing a warning to occur in GCC.

I have commented on the bug on GCC for this (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242#c28), but it looks unlikely to be fixed.

Should we go ahead and add this warning when following its instructions will cause a warning in the GCC compiler? Even though GCC is at fault here, I'm not sure what the right thing is to do.

GCC seems to agree that they should fix it: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414 ; so I wouldn't consider it a blocker.

But I'm not using GCC either, and I don't know what is our usual policy, so it'd be nice to have another opinion here.

[aaron.ballman]

In D24289#592592, @mehdi_amini wrote:

So when this modification tells the developer to add 'unsigned' to their enum, they are subsequently causing a warning to occur in GCC.

I have commented on the bug on GCC for this (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242#c28), but it looks unlikely to be fixed.

Should we go ahead and add this warning when following its instructions will cause a warning in the GCC compiler? Even though GCC is at fault here, I'm not sure what the right thing is to do.

GCC seems to agree that they should fix it: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414 ; so I wouldn't consider it a blocker.

But I'm not using GCC either, and I don't know what is our usual policy, so it'd be nice to have another opinion here.

We don't need to be bug-for-bug compatible with GCC unless there's a compelling reason to do so (which I don't think this is), so I think this is fine to commit, modulo the comments from @thakis regarding the false-positive rate.

[rsmith]

This is causing warnings to fire for headers shared between C and C++, where the "give the enum an unsigned underlying type" advice doesn't work, and where the code in question will never be built for the MS ABI. It seems really hard to justify this being on by default.

I'm going to turn it off by default for now, but we should probably consider turning it back on by default when targeting the MS ABI (as a "your code is wrong" warning rather than a "your code is not portable" warning).

[mehdi_amini]

What is the correct solution for MSVC in C? (If any)

[rnk]

In D24289#598169, @rsmith wrote:

This is causing warnings to fire for headers shared between C and C++, where the "give the enum an unsigned underlying type" advice doesn't work, and where the code in question will never be built for the MS ABI. It seems really hard to justify this being on by default.

I'm going to turn it off by default for now, but we should probably consider turning it back on by default when targeting the MS ABI (as a "your code is wrong" warning rather than a "your code is not portable" warning).

Seems reasonable. I asked to make it off by default, but got argued down to putting it under -Wall.

Yeah, suggesting adding an underlying type to the enum to solve this problem seems like a bad idea, since that fundamentally changes the nature of the enum -- typically allowing it to store a lot more values, and making putting it in a bitfield a bad idea.

Any time you use a bitfield it stores fewer values than the original integer type. I don't see how enums are special here. Even if I can store values in the enum not representable as a bitwise combination of enumerators, people usually don't, and adding an underlying type doesn't change the conventions of normal C++ code.

Do you have any better suggestions for people that want this code to do the right thing when built with MSVC?

enum E /* : unsigned */ { E0, E1, E2, E3 };
struct A { E b : 2; };
int main() {
  A a;
  a.b = E3;
  return a.b; // comes out as -1 without the underlying type
}

Widening the bitfield wastes a bit. Making the bitfield a plain integer and cast in and out of the enum type, but that obscures the true type of the bitfield. So, I still support this suggestion.

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 //
 // FIXME: Should this affect C++11 (where this is an error,
 //        not just deprecated) or not?
 def GCCWriteStrings : DiagGroup<"write-strings" , [WritableStrings]>;
 
 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.
 //
 //  The goal is make -Wunreachable-code on by default, in -Wall, or at
 //  least actively used, with more noisy versions of the warning covered
 //  under separate flags.
 //
 def UnreachableCodeLoopIncrement : DiagGroup<"unreachable-code-loop-increment">;
def Most : DiagGroup<"most", [
     MismatchedTags,
     MissingBraces,
     Move,
     MultiChar,
     Reorder,
     ReturnType,
     SelfAssignment,
     SelfMove,
+    SignedEnumBitfield,
     SizeofArrayArgument,
     SizeofArrayDecay,
     StringPlusInt,
     Trigraphs,
     Uninitialized,
     UnknownPragmas,
     Unused,
     VolatileRegisterVar,

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 // for their workflow.
 def warn_int_to_pointer_cast : Warning<
   "cast to %1 from smaller integer type %0">,
   InGroup<IntToPointerCast>;
 def warn_int_to_void_pointer_cast : Warning<
   "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">,
   InGroup<DiagGroup<"attribute-packed-for-bitfield">>;
 def warn_transparent_union_attribute_field_size_align : Warning<
   "%select{alignment|size}0 of field %1 (%2 bits) does not match the "
   "%select{alignment|size}0 of the first field in transparent union; "
   "transparent_union attribute ignored">,

cfe/trunk/lib/Sema/SemaChecking.cpp

 /// Returns true if there was something fishy about the attempt.
 bool AnalyzeBitFieldAssignment(Sema &S, FieldDecl *Bitfield, Expr *Init,
                                SourceLocation InitLoc) {
   assert(Bitfield->isBitField());
   if (Bitfield->isInvalidDecl())
     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;
 
   // Ignore value- or type-dependent expressions.
   if (Bitfield->getBitWidth()->isValueDependent() ||
       Bitfield->getBitWidth()->isTypeDependent() ||
       Init->isValueDependent() ||
       Init->isTypeDependent())
if (UnaryOperator *UO = dyn_cast<UnaryOperator>(OriginalInit))
       if (UO->getOpcode() == UO_Minus || UO->getOpcode() == UO_Not)
         OriginalWidth = Value.getMinSignedBits();
 
   if (OriginalWidth <= FieldWidth)
     return false;
 
   // 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);
   if (llvm::APSInt::isSameValue(Value, TruncatedValue))
     return false;
 
   // Special-case bitfields of width 1: booleans are naturally 0/1, and
   // therefore don't strictly fit into a signed bitfield of width 1.

cfe/trunk/test/SemaCXX/warn-msvc-enum-bitfield.cpp

+// 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;
+}