[clang] Add a warning (à la gcc) for too small enum bitfields

[faisalv]

Summary

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!

[rsmith]

clang/lib/Sema/SemaDecl.cpp

16443–16446

Please add a member function to EnumDecl to compute this. SemaChecking.cpp does the same calculation twice, and CGExpr.cpp does it once too, so it seems past time to factor out this calculation.

16448

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().

Never use get*ExtValue unless you've already checked that the integer fits in 64 bits (and even then, it's better to avoid it when you can). For example, due to the incorrect pre-existing use of getZExtValue in this function, we asserted on:

enum E {};
struct X { E e : (__int128)0x7fff'ffff'ffff'ffff * (__int128)0x7fff'ffff'ffff'ffff; };

I've fixed that in rG8e923ec2a803d54154aaa0079c1cfcf146b7a22f, but we should avoid reintroducing uses of getZExtValue here.

16454–16459

Should we be emitting such a warning for unnamed bitfields?

The warning is not useful for unnamed bit-fields, as they represent only padding and not values. We should suppress it in that case.

(Maybe we should have a warning for unnamed bit-fields of "unusual" types, since that probably indicates that a name was forgotten, but there might also be valid reasons to do that, such as to avoid the problematic corners of the MS ABI bit-field layout rule. So it seems better to suppress it entirely.)

clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp

62

Please remove the FV_ prefix here -- authorship information is in the version control history if we need it.

It would also be better to write out the test twice (once with named and once with unnamed bit-fields) instead of using a macro and running the entire test file an additional time. Each clang invocation adds to our total test time much more substantially than a few more lines of testcase do, and non-macro-expanded testcases are easier to understand and maintain.

[faisalv]

Based on Richards Feedback, this update includes the following changes:

• avoids calling the fragile getZextValue() for comparing against bit-field width, and uses APSInt's comparison operator overload
• suppresses/avoids the warning for unnamed bit-fields
• simplifies the test case and avoids preprocessor cleverness (and thus an extra pass).

Thank you for taking the time to review this Richard!

[faisalv]

clang/lib/Sema/SemaDecl.cpp

16443–16446

Done. Will try and commit a separate patch that replaces those uses, once this one has been approved.

16448

Aah! I'm glad I asked. Thanks!

clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp

62

Please remove the FV_ prefix here -- authorship information is in the version control history if we need it.

Hah - that wasn't a signifier of authorship but rather employment of a well known acronym in military circles (Field Verification : https://www.acronymfinder.com/Field-Verification-(Census)-(FV).html ) Lol - can't slip anything by you Richard ;)

It would also be better to write out the test twice (once with named and once with unnamed bit-fields) instead of using a macro and running the entire test file an additional time. Each clang invocation adds to our total test time much more substantially than a few more lines of testcase do, and non-macro-expanded testcases are easier to understand and maintain.

Good to know. Thanks. Suppression of the warning for unnamed bitfields made that test case even simpler.

[faisalv]

*ping*

[thakis]

Do you have any numbers on false positives / true positives uncovered by this tweak? In general, warning at use time instead of at declaration time tends to be much better for this rate, and we do this differently than gcc in several instances, because the gcc way has so many false positives that it makes warnings unusable, while warning on use makes the warning useful. So I'd like to see a better justification than "gcc does it" :)

[faisalv]

In D91651#2416423, @thakis wrote:

Do you have any numbers on false positives / true positives uncovered by this tweak?

That's a great question - and unfortunately not only do I have no hard data to support or discourage the addition of such a warning - I don't even know how some of those incredible folks (who make data based claims about programming smells and needs) gather such data (leave alone making sure the data is a representative sample for the "right" kind of programmers) - and would love to be shown how to run such studies :)

In general, warning at use time instead of at declaration time tends to be much better for this rate, and we do this differently than gcc in several instances, because the gcc way has so many false positives that it makes warnings unusable, while warning on use makes the warning useful. So I'd like to see a better justification than "gcc does it" :)

Aah - i did not realize that it was a deliberate decision not to implement such a warning at definition time. My justification (asides from gcc being the light for us in dark places, when all other lights go out ;) for implementing a warning at definition time probably just stems from an instinctual preference for early diagnostics - i suppose there is no one size that fits all here - for e.g. some folks prefer run-time (duck-typing) operation checking vs compile-time checking, and others who prefer diagnosing fundamental issues with template-code when they are first parsed, as opposed to waiting for some instantiation - and then there is the entire C++0X concepts vs concepts-lite discussion ...

So, since I feel I lack the authority to justify such a warning (asides from my ideological propensities) in the face of your valid concerns (either anecdotal or fueled by sufficient data) - I would prefer to defer to you and withdraw the patch :) (unless someone else feels they are able to provide justification that might resonate with you).

Thanks for chiming in!

clang/lib/Sema/SemaDecl.cpp

   }
 
   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);

clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp

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

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