Further update -Wbitfield-constant-conversion for 1-bit bitfield

[aaron.ballman]

Summary

https://reviews.llvm.org/D131255 (82afc9b169a67e8b8a1862fb9c41a2cd974d6691) began warning about conversion causing data loss for a single-bit bit-field. However, after landing the changes, there were reports about significant false positives from some code bases.

This alters the approach taken in that patch by introducing a new warning group (-Wsingle-bit-bitfield-constant-conversion) which is grouped under -Wbitfield-constant-conversion to allow users to selectively disable the single-bit warning without losing the other constant conversion warnings.

[erichkeane]

LGTM, questioning whether we want to do some macro introspection in C mode (as the CORRECT way to use these 1 bit-bitfields is as a bool), but at least this gives the person the ability to disable this warning.

clang/test/Sema/constant-conversion.c

24–25

Can you add a test that shows this isn't diagnosed:

s.b = true;?

Thanks to stdbool.h, this of course might have to be in the C++ test (or C23). Makes me question whether we need to do some detection to see if someone has written the above.

[aaron.ballman]

Added a test case for true in C++, and added similar logic for C to reduce the chance for false positives. If the user uses true (the macro from stdbool.h), they're signaling an intent that the field is used in boolean contexts. The macro expands to 1 (per spec) but should not be diagnosed because the actual value stored only needs to be nonzero to have the correct boolean semantics.

[erichkeane]

2 nits, otherwise LGTM.

clang/lib/Sema/SemaChecking.cpp

13080

Might consider hoisting this line above the macro-checking, and just use it in both cases. Also, perhaps a more descriptive name? IsInvalidSignedOneBitAssignment ?

[aaron.ballman]

Update based on review feedback.

[erichkeane]

Still LGTM.

[bjope]

clang/test/Sema/constant-conversion.c

30

(Sorry for being late to the party, with post commit comments. I'm just trying to understand the reasoning about always suppressing the warning based on "true".)

Isn't the code a bit suspicious when using true/false from stdbool.h, while using a signed bitfield?

When doing things like

(s.b == true) ? 1 : 0

the result would always be zero if s.b is a signed 1-bit bitfield.

So wouldn't it make more sense to actually use an unsigned bitfield (such as the bool type from stdbool.h) when the intent is to store a boolean value and using defines from stdbool.h?

Is perhaps the idea that we will get warnings about (s.b == true) being redundant in situations like this, and then we do not need a warning on the bitfield assignment? Such a reasoning would actually make some sense, since (s.b == true) never would be true even when the bitfield is assigned a non-constant value, so we can't rely on catching the problem by only looking at bitfield assignments involving true/false.

[aaron.ballman]

clang/test/Sema/constant-conversion.c

30

Isn't the code a bit suspicious when using true/false from stdbool.h, while using a signed bitfield?

Yes and no...

When doing things like

(s.b == true) ? 1 : 0

the result would always be zero if s.b is a signed 1-bit bitfield.

You're correct, but that's a bit of a code smell because of == true.

So wouldn't it make more sense to actually use an unsigned bitfield (such as the bool type from stdbool.h) when the intent is to store a boolean value and using defines from stdbool.h?

Yes, ideally.

Is perhaps the idea that we will get warnings about (s.b == true) being redundant in situations like this, and then we do not need a warning on the bitfield assignment? Such a reasoning would actually make some sense, since (s.b == true) never would be true even when the bitfield is assigned a non-constant value, so we can't rely on catching the problem by only looking at bitfield assignments involving true/false.

The idea is more that someone using true is more likely to be treating the field as a boolean and so they won't be comparing the bit-field against a specific value, but instead testing it for its boolean value. This also unifies the behavior between C and C++: https://godbolt.org/z/WKP4xcPzT

We don't currently issue a diagnostic on == true, but that sure seems like something -Wtautological-compare should pick up on (for bit-fields in general, not just for one-bit bit-fields): https://godbolt.org/z/Tj4711Ysc

(Note, godbolt seems to have a Clang trunk that's ~8 days old, so diagnostic behavior doesn't match actual trunk yet. That caught me by surprise.)

[bjope]

clang/test/Sema/constant-conversion.c

30

Ok, thanks! Sounds reasonable to me.

[aaron.ballman]

clang/test/Sema/constant-conversion.c

30

Great! Thank you for the discussion! (FWIW, it's *totally* fine to give post commit feedback, so no need to apologize. You're never late to the "why does this work that way?" party.)

clang/docs/ReleaseNotes.rst

- Clang will now correctly diagnose as ill-formed a constant expression where an
   to provide a transition period for users. This diagnostic is expected to turn
   into an error-only diagnostic in the next Clang release. Fixes
   `Issue 50055: <https://github.com/llvm/llvm-project/issues/50055>`_.
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.
-- ``-Wbitfield-constant-conversion`` now diagnoses implicit truncation when 1 is
-  assigned to a 1-bit signed integer bitfield. This fixes
-  `Issue 53253 <https://github.com/llvm/llvm-project/issues/53253>`_.
+- Introduced ``-Wsingle-bit-bitfield-constant-conversion``, grouped under
+  ``-Wbitfield-constant-conversion``, which diagnoses implicit truncation when
+  ``1`` is assigned to a 1-bit signed integer bitfield. This fixes
+  `Issue 53253 <https://github.com/llvm/llvm-project/issues/53253>`_. To reduce
+  potential false positives, this diagnostic will not diagnose use of the
+  ``true`` macro (from ``<stdbool.h>>`) in C language mode despite the macro
+  being defined to expand to ``1``.
 - ``-Wincompatible-function-pointer-types`` now defaults to an error in all C
   language modes. It may be downgraded to a warning with
   ``-Wno-error=incompatible-function-pointer-types`` or disabled entirely with
   ``-Wno-implicit-function-pointer-types``.
 - Clang will now print more information about failed static assertions. In
   particular, simple static assertion expressions are evaluated to their
   compile-time value and printed out if the assertion fails.
 - Diagnostics about uninitialized ``constexpr`` varaibles have been improved

clang/include/clang/Basic/DiagnosticGroups.td

 def FrameworkHdrAtImport : DiagGroup<"atimport-in-framework-header">;
 def CXX14BinaryLiteral : DiagGroup<"c++14-binary-literal">;
 def CXXPre14CompatBinaryLiteral : DiagGroup<"c++98-c++11-compat-binary-literal">;
 def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
 def BinaryLiteral : DiagGroup<"binary-literal", [CXX14BinaryLiteral,
                                                  CXXPre14CompatBinaryLiteral,
                                                  GNUBinaryLiteral]>;
 def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
-def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
+def SingleBitBitFieldConstantConversion :
+  DiagGroup<"single-bit-bitfield-constant-conversion">;
+def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion",
+                                           [SingleBitBitFieldConstantConversion]>;
 def BitFieldEnumConversion : DiagGroup<"bitfield-enum-conversion">;
 def BitFieldWidth : DiagGroup<"bitfield-width">;
 def CompoundTokenSplitByMacro : DiagGroup<"compound-token-split-by-macro">;
 def CompoundTokenSplitBySpace : DiagGroup<"compound-token-split-by-space">;
 def CompoundTokenSplit : DiagGroup<"compound-token-split",
                                    [CompoundTokenSplitByMacro,
                                     CompoundTokenSplitBySpace]>;
 def CoroutineMissingUnhandledException :

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_impcast_nonnegative_result : Warning<
   "the resulting value is always non-negative after implicit conversion">,
   InGroup<SignConversion>, DefaultIgnore;
 def warn_impcast_integer_64_32 : Warning<
   "implicit conversion loses integer precision: %0 to %1">,
   InGroup<Shorten64To32>, DefaultIgnore;
 def warn_impcast_integer_precision_constant : Warning<
   "implicit conversion from %2 to %3 changes value from %0 to %1">,
   InGroup<ConstantConversion>;
+def warn_impcast_single_bit_bitield_precision_constant : Warning<
+  "implicit truncation from %2 to a one-bit wide bit-field changes value from "
+  "%0 to %1">, InGroup<SingleBitBitFieldConstantConversion>;
 def warn_impcast_bitfield_precision_constant : Warning<
   "implicit truncation from %2 to bit-field changes value from %0 to %1">,
   InGroup<BitFieldConstantConversion>;
 def warn_impcast_constant_value_to_objc_bool : Warning<
   "implicit conversion from constant value %0 to 'BOOL'; "
   "the only well defined values for 'BOOL' are YES and NO">,
   InGroup<ObjCBoolConstantConversion>;
 

clang/lib/Sema/SemaChecking.cpp

if (!OriginalInit->EvaluateAsInt(Result, S.Context,
 
     return false;
   }
 
   llvm::APSInt Value = Result.Val.getInt();
 
   unsigned OriginalWidth = Value.getBitWidth();
 
+  // In C, the macro 'true' from stdbool.h will evaluate to '1'; To reduce
+  // false positives where the user is demonstrating they intend to use the
+  // bit-field as a Boolean, check to see if the value is 1 and we're assigning
+  // to a one-bit bit-field to see if the value came from a macro named 'true'.
+  bool OneAssignedToOneBitBitfield = FieldWidth == 1 && Value == 1;
+  if (OneAssignedToOneBitBitfield && !S.LangOpts.CPlusPlus) {
+    SourceLocation MaybeMacroLoc = OriginalInit->getBeginLoc();
+    if (S.SourceMgr.isInSystemMacro(MaybeMacroLoc) &&
+        S.findMacroSpelling(MaybeMacroLoc, "true"))
+      return false;
+  }
+
   if (!Value.isSigned() || Value.isNegative())
     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(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;
 
   std::string PrettyValue = toString(Value, 10);
   std::string PrettyTrunc = toString(TruncatedValue, 10);
 
-  S.Diag(InitLoc, diag::warn_impcast_bitfield_precision_constant)
+  S.Diag(InitLoc, OneAssignedToOneBitBitfield
+                      ? diag::warn_impcast_single_bit_bitield_precision_constant
+                      : diag::warn_impcast_bitfield_precision_constant)
       << PrettyValue << PrettyTrunc << OriginalInit->getType()
       << Init->getSourceRange();
 
   return true;
 }
 
 /// Analyze the given simple or compound assignment for warning-worthy
 /// operations.
 static void AnalyzeAssignment(Sema &S, BinaryOperator *E) {
   // Just recurse on the LHS.

clang/test/CXX/drs/dr6xx.cpp

namespace dr674 { // dr674: 8
   template int Y::f<>(int);
   template int Y::g<>(int); // expected-note {{in instantiation of}}
   template int Y::h<>(int);
 }
 
 namespace dr675 { // dr675: dup 739
   template<typename T> struct A { T n : 1; };
 #if __cplusplus >= 201103L
-  static_assert(A<char>{1}.n < 0, ""); // expected-warning {{implicit truncation from 'int' to bit-field changes value from 1 to -1}}
-  static_assert(A<int>{1}.n < 0, ""); // expected-warning {{implicit truncation from 'int' to bit-field changes value from 1 to -1}}
-  static_assert(A<long long>{1}.n < 0, ""); // expected-warning {{implicit truncation from 'int' to bit-field changes value from 1 to -1}}
+  static_assert(A<char>{1}.n < 0, ""); // expected-warning {{implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1}}
+  static_assert(A<int>{1}.n < 0, ""); // expected-warning {{implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1}}
+  static_assert(A<long long>{1}.n < 0, ""); // expected-warning {{implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1}}
 #endif
 }
 
 // dr676: na
 
 namespace dr677 { // dr677: no
   struct A {
     void *operator new(std::size_t);

clang/test/Sema/constant-conversion.c

-// RUN: %clang_cc1 -fsyntax-only -verify -triple x86_64-apple-darwin %s
+// RUN: %clang_cc1 -fsyntax-only -ffreestanding -verify=expected,one-bit -triple x86_64-apple-darwin %s
+// RUN: %clang_cc1 -fsyntax-only -ffreestanding -Wno-single-bit-bitfield-constant-conversion -verify -triple x86_64-apple-darwin %s
+
+#include <stdbool.h>
 
 // This file tests -Wconstant-conversion, a subcategory of -Wconversion
 // which is on by default.
 
 // rdar://problem/6792488
 void test_6792488(void) {
   int x = 0x3ff0000000000000U; // expected-warning {{implicit conversion from 'unsigned long' to 'int' changes value from 4607182418800017408 to 0}}
 }
 
 void test_7809123(void) {
   struct { int i5 : 5; } a;
 
   a.i5 = 36; // expected-warning {{implicit truncation from 'int' to bit-field changes value from 36 to 4}}
 }
 
 void test(void) {
   struct S {
     int b : 1;  // The only valid values are 0 and -1.
   } s;
 
-  s.b = -3; // expected-warning {{implicit truncation from 'int' to bit-field changes value from -3 to -1}}
-  s.b = -2; // expected-warning {{implicit truncation from 'int' to bit-field changes value from -2 to 0}}
-  s.b = -1; // no-warning
-  s.b = 0;  // no-warning
-  s.b = 1;  // expected-warning {{implicit truncation from 'int' to bit-field changes value from 1 to -1}}
-  s.b = 2;  // expected-warning {{implicit truncation from 'int' to bit-field changes value from 2 to 0}}
+  s.b = -3;    // expected-warning {{implicit truncation from 'int' to bit-field changes value from -3 to -1}}
+  s.b = -2;    // expected-warning {{implicit truncation from 'int' to bit-field changes value from -2 to 0}}
+  s.b = -1;    // no-warning
+  s.b = 0;     // no-warning
+  s.b = 1;     // one-bit-warning {{implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1}}
+  s.b = true;  // no-warning (we suppress it manually to reduce false positives)
+  s.b = false; // no-warning
+  s.b = 2;     // expected-warning {{implicit truncation from 'int' to bit-field changes value from 2 to 0}}
 }
 
 enum Test2 { K_zero, K_one };
 enum Test2 test2(enum Test2 *t) {
   *t = 20;
   return 10; // shouldn't warn
 }
 

clang/test/SemaCXX/constant-conversion.cpp

void too_big_for_char(int param) {
   // expected-warning@-1 {{implicit conversion from 'int' to 'char' changes value from 99999 to -97}}
 
   char ok1 = true ? 0 : 99999;
   char ok2 = true ? 0 : nines();
 
   char ok3 = true ? 0 : 99999 + 1;
   char ok4 = true ? 0 : nines() + 1;
 }
+
+void test_bitfield() {
+  struct S {
+    int one_bit : 1;
+  } s;
+
+  s.one_bit = 1;    // expected-warning {{implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1}}
+  s.one_bit = true; // no-warning
+}