[PATCH] D24289: Add warning when assigning enums to bitfields without an explicit unsigned underlying type

[Sasha Bermeister via cfe-commits]

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 <arthur.j.odwyer at gmail.com>
wrote:

> On Thu, Nov 17, 2016 at 9:52 AM, Richard Smith via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
> > On 17 Nov 2016 8:56 am, "Reid Kleckner" <rnk at google.com> 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 increasing the bit-field's
> width
>
> Changing the width of the bit-field can affect only the layout of the
> struct in question; you could even static_assert that the size *doesn't*
> change, if there are padding bits available.
> Changing a whole type from signed to unsigned seems like it would have
> more dangerous action-at-a-distance than just increasing the width of the
> bit-field: that would *unavoidably* affect comparisons (and overload
> resolutions?) across the entire codebase using that type.
> http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#172
> Again I'm making a philosophical distinction between the details of
> storage (e.g. struct layout) and the actual semantics of values and types
> (e.g. signedness).
>
> The problem is with the bit-field inside the struct, so IMHO the solution
> should involve changing the bit-field and/or the struct. Not altering the
> enum type... which by the way, the programmer might not even control,
> right? What if the relevant enum type comes from a third-party library?
>
> > How about we consider msvc's behaviour to just be a bug, and zero-extend
> > loads from enum bitfields with no negative enumerators? Since loading
> such a
> > negative value results in UB, this wouldn't change the meaning of any
> > program with defined behaviour, and still respects the MS ABI.
>
> SGTM, at least to put this behavior under a separately toggleable
> command-line switch. (-fms-enum-bitfields?)
>
> my $.02,
> –Arthur
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161118/7bf3f227/attachment-0001.html>