Re: [PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
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()->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;
+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>, 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"
Re: [PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
sashab added a comment. Pinging for another LGTM since I have added it to -Wall and added a portability comment to the error message :) https://reviews.llvm.org/D24289 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
sashab added a comment. Thanks all! :) Comment at: test/SemaCXX/warn-msvc-enum-bitfield.cpp:12 @@ +11,3 @@ + + s.e2 = E2; + s.f2 = F2; thakis wrote: > Shouldn't this be the version that warns? The assignment with E1 assigns 0 > which is portable, but this assigns 1 which overflows, right? 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 :) https://reviews.llvm.org/D24289 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
sashab marked an inline comment as done. sashab added a comment. https://reviews.llvm.org/D24289 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
sashab closed this revision. sashab added a comment. Is this how I commit this? Hopefully this lands... :-) https://reviews.llvm.org/D24289 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
Although I agree with your philosophical discussion and suggestions, the
reality is that MSVC's behavior is not a bug and compilers are free to
interpret enum bitfields with no explicit underlying type in any way they
want (see spec reference in GCC bug link), with a signed interpretation
being a valid one. I'd say it's undefined behavior in C/C++ to store an
enum in a bitfield without specifying an underlying type, since the
compiler is free to interpret this bitfield in any way it wants -- in
general, if you haven't specified an underlying type you should probably be
warned when trying to store it in a bitfield because the compiler may not
do what you expect.
With that said, I'm happy to put this warning behind a flag, or remove it
altogether from Clang. This is an important feature for Blink and whether
or not it lands, we will be using it for compiling our own code. I
submitted the patch upstream to Clang since I thought all developers should
be aware of this undefined behavior. However, if you feel that developers
who write code that uses enum bitfields should be free to write
non-MSVC-compatible code, then there's no need for this to be in the main
Clang release.
Sasha
On Fri, Nov 18, 2016 at 7:48 AM, Arthur O'Dwyer
wrote:
> On Thu, Nov 17, 2016 at 9:52 AM, Richard Smith via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> > On 17 Nov 2016 8:56 am, "Reid Kleckner" wrote:
> >> In https://reviews.llvm.org/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).
> [...]
> >>> 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. [...]
> >
> > The range of representable values for a bitfield with no fixed underlying
> > type is actually smaller than that of its underlying type. See
> > http://eel.is/c++draft/dcl.enum#8
> >
> > So a bitfield of width equal to the number of bits needed to store any
> > enumerator does not have fewer values than the original type.
>
> My understanding (from osmosis and practice more than from reading the
> standard) is that programmers are more likely to specify an "unnaturally
> narrow" underlying type (e.g. "int8_t") than to specify an "unnaturally
> wide" underlying type (e.g. "int32_t". When I specify an underlying type,
> I'm saying "The compiler is going to do the wrong thing with this type's
> *storage* by default"; I'm not saying anything about the type's *value
> range*.
> The same goes for bit-fields: I specify a number of bits after the colon
> because the compiler would otherwise do the wrong thing with *storage*,
> not because I'm trying to change the semantics of the *values* involved.
>
>
> >> 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.
>
> The safest fix is to just change ": 2" to ": 3", even though that "wastes
> a bit" (really it wastes 0 bits in most cases and 32 to 64 bits in some
> cases).
>
> If I had my druthers, the compiler would be completely silent unless it
> detected exactly the cases that would result in changes to the semantics of
> enum *values*. That is,
>
> when declaring a bit-field of enum type E with width B bits:
> if E has an enumerator e whose value requires >B bits:
> warn that e cannot be stored in the bit-field
> if a fixit is really required, suggest increasing the bit-field's
> width
> if E has an enumerator e whose positive value requires exactly B
> bits, and E's underlying type is signed:
> warn that e cannot be stored in the bit-field when MSVC semantics
> are in use
> in C++, append the note that this happens because E's underlying
> type is signed
> if a fixit is really required, suggest increas
Re: [PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
Thanks for the great explanation, I agree with this advice. I'm going to
investigate writing a new patch to do this.
On Fri, Nov 18, 2016 at 1:59 PM, Arthur O'Dwyer
wrote:
> On Thu, Nov 17, 2016 at 2:14 PM, Sasha Bermeister
> wrote:
>
>> Although I agree with your philosophical discussion and suggestions, the
>> reality is that MSVC's behavior is not a bug and compilers are free to
>> interpret enum bitfields with no explicit underlying type in any way they
>> want (see spec reference in GCC bug link), with a signed interpretation
>> being a valid one. I'd say it's undefined behavior in C/C++ to store an
>> enum in a bitfield without specifying an underlying type, since the
>> compiler is free to interpret this bitfield in any way it wants -- in
>> general, if you haven't specified an underlying type you should probably be
>> warned when trying to store it in a bitfield because the compiler may not
>> do what you expect.
>>
>
> Incorrect. The following program has perfectly well defined behavior:
>
> enum E { e = 0 };
> struct S { E bf : 4; } s;
> int main() { s.bf = e; }
>
> No compiler in the world should produce a warning on the above program.
>
> Also, once you've got a struct type containing an offending bit-field,
> *any* use of that bit-field is subject to the implementation-defined
> behavior you noticed on MSVC. It's not just limited to
> assignment-expressions of constants. That's why it's important to produce a
> warning on the *declaration* of the bit-field, not on each subsequent
> expression that refers to that bit-field.
>
> enum E2 { e = 0, f = 1, g = 2, h = 3 };
> struct S2 { E2 bf : 2; } s; // this line should trigger a diagnostic
> int main() { s.bf = e; }
>
> Also, the current patch's diagnostic wording suggests to "consider giving
> the enum E an unsigned underlying type", which would be very bad advice in
> this situation (because it only works in C++11, and because it triggers a
> GCC bug, and because it has non-local effects on the program's semantics).
> The correct advice is to "consider giving the bit-field bf a width of 3
> bits instead of 2."
>
> HTH,
> –Arthur
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type
sashab added a comment.
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.
https://reviews.llvm.org/D24289
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
