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

[Faisal Vali via Phabricator via cfe-commits]

faisalv added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91651/new/

https://reviews.llvm.org/D91651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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" :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91651/new/

https://reviews.llvm.org/D91651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv requested review of this revision.
faisalv marked 4 inline comments as done.
faisalv added a comment.

*ping*


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91651/new/

https://reviews.llvm.org/D91651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv planned changes to this revision.
faisalv marked 3 inline comments as done.
faisalv added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:16443-16446
+  const unsigned BitsNeeded =
+  IsSignedEnum
+  ? std::max(ED->getNumPositiveBits() + 1, 
ED->getNumNegativeBits())
+  : ED->getNumPositiveBits();

rsmith wrote:
> 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.
Done.  Will try and commit a separate patch that replaces those uses, once this 
one has been approved.



Comment at: clang/lib/Sema/SemaDecl.cpp:16448
+  
+  if (BitsNeeded > Value.getZExtValue()) {
+// TODO: Should we be emitting diagnostics for unnamed bitfields that

rsmith wrote:
> > 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''' * 
> (__int128)0x7fff'''; };
> ```
> 
> I've fixed that in rG8e923ec2a803d54154aaa0079c1cfcf146b7a22f, but we should 
> avoid reintroducing uses of `getZExtValue` here.
Aah!  I'm glad I asked.  Thanks!



Comment at: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp:62
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)

rsmith wrote:
> 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.
> 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.  



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91651/new/

https://reviews.llvm.org/D91651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv updated this revision to Diff 305970.
faisalv added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91651/new/

https://reviews.llvm.org/D91651

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp


Index: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
+++ clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -57,3 +57,18 @@
   f.two_bits = (unsigned)three_bits_signed;
   f.two_bits = static_cast(three_bits_signed);
 }
+
+enum class E_UNSIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long 
long) * 8 - 2)) };
+enum class E_SIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long 
long) * 8 - 2)), MINUS = -1 };
+
+struct E64 {
+  E_UNSIGNED_LONG_LONG b3 : (sizeof(long long) * 8) - 1; // OK.
+  E_SIGNED_LONG_LONG   b4 : (sizeof(long long) * 8); // OK.
+
+  E_UNSIGNED_LONG_LONG b1 : 5; //expected-warning {{bit-field 'b1' is not wide 
enough}} expected-note {{widen this field to 63}}
+  E_SIGNED_LONG_LONG   b2 : 5; //expected-warning {{bit-field 'b2' is not wide 
enough}} expected-note {{widen this field to 64}}
+  
+  E_UNSIGNED_LONG_LONG : 5; // OK.  no warning for unnamed bit fields.
+  E_SIGNED_LONG_LONG   : 5; // OK.  no warning for unnamed bit fields.
+  
+};
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16430,6 +16430,22 @@
   }
 
   if (!FieldTy->isDependentType()) {
+// Check if a named enum bit-field has enough bits to accomodate all its
+// enumerators. We can ignore unnamed enum bit-fields since they do not
+// represent values.
+if (FieldName && FieldTy->isEnumeralType()) {
+  const EnumDecl *const ED = FieldTy->castAs()->getDecl();
+
+  const auto BitsNeeded = ED->getTotalBitsNeeded();
+  if (BitsNeeded > Value) {
+Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+<< 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);
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -3723,6 +3723,14 @@
   /// -101 1001011 8
   unsigned getNumNegativeBits() const { return EnumDeclBits.NumNegativeBits; }
 
+  /// Returns the least width in bits required to store all the enumerators.
+  unsigned getTotalBitsNeeded() const {
+// If the enum has negative values, we need one more bit than the normal
+// number of positive bits to represent the sign bit.
+return getNumNegativeBits() > 0
+   ? std::max(getNumPositiveBits() + 1, getNumNegativeBits())
+   : getNumPositiveBits();
+  }
   /// Returns true if this is a C++11 scoped enumeration.
   bool isScoped() const { return EnumDeclBits.IsScoped; }
 


Index: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
+++ clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -57,3 +57,18 @@
   f.two_bits = (unsigned)three_bits_signed;
   f.two_bits = static_cast(three_bits_signed);
 }
+
+enum class E_UNSIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long long) * 8 - 2)) };
+enum class E_SIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long long) * 8 - 2)), MINUS = -1 };
+
+struct E64 {
+  E_UNSIGNED_LONG_LONG b3 : (sizeof(long long) * 8) - 1; // OK.
+  E_SIGNED_LONG_LONG   b4 : (sizeof(long long) * 8); // OK.
+
+  E_UNSIGNED_LONG_LONG b1 : 5; //expected-warning {{bit-field 'b1' is not wide enough}} expected-note {{widen this field to 63}}
+  E_SIGNED_LONG_LONG   b2 : 5; //expected-warning {{bit-field 'b2' is not wide enough}} expected-note {{widen this field to 64}}
+  
+  E_UNSIGNED_LONG_LONG : 5; // OK.  no warning for unnamed bit fields.
+  E_SIGNED_LONG_LONG   : 5; // OK.  no warning for unnamed bit fields.
+  
+};
Index: clang/lib/Sema/SemaDecl.cpp

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:16443-16446
+  const unsigned BitsNeeded =
+  IsSignedEnum
+  ? std::max(ED->getNumPositiveBits() + 1, 
ED->getNumNegativeBits())
+  : ED->getNumPositiveBits();

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.



Comment at: clang/lib/Sema/SemaDecl.cpp:16448
+  
+  if (BitsNeeded > Value.getZExtValue()) {
+// TODO: Should we be emitting diagnostics for unnamed bitfields that

> 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''' * 
(__int128)0x7fff'''; };
```

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



Comment at: clang/lib/Sema/SemaDecl.cpp:16454-16459
+} else {
+  Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+  << "" << ED;
+  //<< FieldName << ED;
+  //
+}

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



Comment at: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp:62
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91651/new/

https://reviews.llvm.org/D91651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv created this revision.
faisalv added reviewers: aaron.ballman, wchilders, bruno, rnk, BRevzin, thakis.
faisalv added a project: clang.
faisalv requested review of this revision.

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!


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91651

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp


Index: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
+++ clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -1,3 +1,4 @@
+// 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
 
@@ -57,3 +58,29 @@
   f.two_bits = (unsigned)three_bits_signed;
   f.two_bits = static_cast(three_bits_signed);
 }
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)
+//expected-warning@#1 {{bit-field  is not wide enough}} 
+//expected-warning@#2 {{bit-field  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);
+
+};
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16430,6 +16430,38 @@
   }
 
   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()->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)
+  << "" << 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);


Index: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
+++