Fix Wbitfield-constant-conversion on 1-bit signed bitfield

[ShawnZhong]

Summary

Fix https://github.com/llvm/llvm-project/issues/53253

[aaron.ballman]

Thank you for working on this! I think this is going in the right direction, but I think there's some more work needed here because the rules depend on the type of the bit-field (at least in C, I've not checked C++ yet). See C2x 6.7.2.1p12 (and it's footnote): "A bit-field is interpreted as having a signed or unsigned integer type consisting of the specified number of bits. ..." (and the footnote goes on to say "As specified in 6.7.2 above, if the actual type specifier used is int or a typedef-name defined as int, then it is implementation-defined whether the bit-field is signed or unsigned. ...").

Assuming that we actually do treat plain int bit-fields as being signed (which I'm pretty sure we do), then I agree that these changes are fine. But you should add some CodeGen test coverage (or find existing coverage) that proves how we behave in that case and an int test in Sema showing we match the codegen behavior.

Also, can you add a release note for the fix?

clang/test/Sema/constant-conversion.c

145

[ShawnZhong]

Thanks for the quick reply and the reference on the C standard!

On the C++ side, Section C.1.8 specified that int bit-fields are signed:

Change: Bit-fields of type plain int are signed.
Rationale: Leaving the choice of signedness to implementations could lead to inconsistent definitions of template specializations. For consistency, the implementation freedom was eliminated for non-dependent types, too.
Effect on original feature: The choise is implementation-defined in C, but not so in C++.

Implementation-wise, I'll see what I can find in CodeGen on whether int bit-fields are signed for C

[aaron.ballman]

In D131255#3702286, @ShawnZhong wrote:

Thanks for the quick reply and the reference on the C standard!

On the C++ side, Section C.1.8 specified that int bit-fields are signed:

Change: Bit-fields of type plain int are signed.
Rationale: Leaving the choice of signedness to implementations could lead to inconsistent definitions of template specializations. For consistency, the implementation freedom was eliminated for non-dependent types, too.
Effect on original feature: The choise is implementation-defined in C, but not so in C++.

Agreed; I think that's falling out from https://eel.is/c++draft/class.bit#4.sentence-1 and https://eel.is/c++draft/basic.fundamental#1.sentence-1.

Implementation-wise, I'll see what I can find in CodeGen on whether int bit-fields are signed for C

Thanks!

[ShawnZhong]

• Added CodeGen tests.
• Fixed failed tests that assigned 1 to int:1.
• Modified the Sema test to use int: instead of signed char:1.
• Added release note.

[erichkeane]

I did a pass through, this seems like it is the right thing. I looked over the codegen tests, and I'm convinced it does what we want from that perspective. I'll leave it to @aaron.ballman to take another look for +1'ing this though.

[aaron.ballman]

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

[ShawnZhong]

In D131255#3706784, @aaron.ballman wrote:

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

I'm new to Phabricator. I don't need someone else to commit on my behalf, but I guess I don't have permission to merge a commit on my own.

If the patch submitted misses author information for some reason, feel free to use Shawn Zhong <[e-mail]>.

[thakis]

clang/lib/Sema/SemaChecking.cpp

13069

This was to suppress false positives. All instances we've seen of this are in fact false positives.

Was there any analysis on true / false positives for this change?

At least for our code, this seems to make the warning strictly worse.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

13069

I've not performed any such analysis, but it would be good to know. FWIW, this is the kind of situation I was thinking this diagnostic would help make far more clear: https://godbolt.org/z/336n9xex3 (not that I expect people to write that static assert, but I very much expect people who assign the value 1 into a bit-field and then check for the value 1 to come back out are going to be surprised).

That said, another approach to that specific scenario, which might have a better true positive rate would be:

struct S {
  int bit : 1;
};

int main() {
  constexpr S s{1}; // No warning
  if (s.bit == 1) { // Warn about explicit comparison of a 1-bit bit-field
    ...
  } else if (s.bit == 0) { // Warn about explicit comparison of a 1-bit bit-field?
    ...
  } else if (s.bit) { // No warning
    ...
  } else if (!s.bit) { // No warning
    ...
  }
}

[ShawnZhong]

clang/lib/Sema/SemaChecking.cpp

13069

Do you have something in mind that counts as false positives?

13069

BTW, I realized that no warning is reported when a bool is assigned to a single-bit signed bit-field:

https://godbolt.org/z/M5ex48T8s

int main() {
  struct S { int b : 1; } s;
  s.b = true;
  if (s.b == true) { puts("T"); } else { puts("F"); }
}

[ShawnZhong]

clang/lib/Sema/SemaChecking.cpp

13069

For reference, I found this issue on chromium:

https://bugs.chromium.org/p/chromium/issues/detail?id=1352183

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

13069

Do you have something in mind that counts as false positives?

In my example above, I consider the use of s.bit and !s.bit to count as false positives. The value in the bit-field being 1 or -1 has no bearing on the semantics of the program, the only thing that matters is zero vs nonzero because of the boolean conversion.

BTW, I realized that no warning is reported when a bool is assigned to a single-bit signed bit-field:

Good catch, the conversion from bool to integer does give the value 1 explicitly. At the same time, I would consider the assignment to the bit-field from a boolean to be a non-issue, the problem only stems from attempting to use inspect the value.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

13069

BTW, I realized that no warning is reported when a bool is assigned to a single-bit signed bit-field:

Good catch, the conversion from bool to integer does give the value 1 explicitly. At the same time, I would consider the assignment to the bit-field from a boolean to be a non-issue, the problem only stems from attempting to use inspect the value.

Actually, that's a more interesting case than I had originally realized. See C2x 6.7.2.1p12. "If the value 0 or 1 is stored into a nonzero-width bit-field of type bool, the value of the bit-field shall compare equal to the value stored; a bool bit-field has the semantics of a bool."

So if you store a 1 into a bool bit-field, you have to get a 1 back out of it. The bit-field is implicitly unsigned, effectively.

[ShawnZhong]

clang/lib/Sema/SemaChecking.cpp

13069

In my example above, I consider the use of s.bit and !s.bit to count as false positives. The value in the bit-field being 1 or -1 has no bearing on the semantics of the program, the only thing that matters is zero vs nonzero because of the boolean conversion.

I agree with you that if s.bit is only used to be converted to bool, then the value stored being 1 or -1 does not matter that much. But hypothetically, one could use s.bit for arithmetic (e.g., index into an array), assignment (say into another int or enum), or, in your example, comparison against another value. If a warning is not generated during conversion, handling them in other places would be harder.

Actually, that's a more interesting case than I had originally realized. See C2x 6.7.2.1p12. "If the value 0 or 1 is stored into a nonzero-width bit-field of type bool, the value of the bit-field shall compare equal to the value stored; a bool bit-field has the semantics of a bool."
So if you store a 1 into a bool bit-field, you have to get a 1 back out of it. The bit-field is implicitly unsigned, effectively.

That makes sense to me. I'm not particularly concerned with assigning 1 to bool:1 or any uint*_t:1 in general. People may want to use them to compactly store bools in a struct.

I realized that no warning is reported when bool is assigned to a single-bit signed bit-field

The issue I referred to is the following:

int main() {
  struct S { int b : 1; } s;
  s.b = 1;     // warning generated after this patch
  s.b = true;  // still no warning, but there should be
}

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

13069

If a warning is not generated during conversion, handling them in other places would be harder.

Harder, but with less false positives. We try to keep the false positive rate as low as we can for diagnostics in clang so that users are less tempted to entirely disable the diagnostic as being low-value.

The issue I referred to is the following:

int main() {
  struct S { int b : 1; } s;
  s.b = 1;     // warning generated after this patch
  s.b = true;  // still no warning, but there should be
}

On the one hand, yes -- pedantically, that is true. On the other hand, does use of a boolean indicate programmer intent that this field be treated as a boolean, and thus the actual value stored doesn't matter?

At this point, I think we need some evidence that this is finding true positives instead of only false positives. We've already heard about Chromium, but I think you should try against some other larger C and C++ code bases to see what the true positive rate is. If there's evidence that this it catching actual bugs in practice, then it strengthens the case for the current design. If we don't find many true positives but find it increases the false positive rate, we should either rework this patch or back it out entirely.

(I don't have a particular corpus in mind, but I'm thinking things like OpenSSL or sqlite3 for C code, and LLVM or Qt for C++ code in terms of size.)

[bjope]

FWIW, just some things noticed when I examined some of the new warning that popped up after this patch:

https://github.com/llvm/llvm-project/issues/53253 mentioned that for example gcc complained about this. Although, as shown here https://godbolt.org/z/bq34Kexac there are some other differences that clang now complains already with -Wall, but that is not the case with gcc (I think one need to enable -Wpedantic in gcc to get a warning about signed overflow).

A similar warning (when assigning -1 to an unsigned bitfield) can be given by both gcc and clang by using -Wsign-conversion but that is not part of -Wall either. But maybe that is a totally different thing.

[aaron.ballman]

After some more thought and some offline discussions, I think I have a reasonable way forward: let's add -Wsingle-bit-bitfield-constant-conversion as a new warning group under -Wbitfield-constant-conversion that controls the diagnostic for one-bit bitfields. The diagnostic behavior here is pedantically correct and will help catch some bugs for folks, but there's enough existing code using 1 and not misbehaving (probably because they're not inspecting the value except in a boolean context) that having separate control seems useful.

What do others think?

In D131255#3725818, @bjope wrote:

A similar warning (when assigning -1 to an unsigned bitfield) can be given by both gcc and clang by using -Wsign-conversion but that is not part of -Wall either. But maybe that is a totally different thing.

It's a different thing. When the bit-field is unsigned, the conversion from signed is well-defined and so we only issue a -Wconversion warning, but it doesn't technically truncate the bit-field. That's also why we don't diagnose: https://godbolt.org/z/WW9477jfG

[aaron.ballman]

In D131255#3729304, @aaron.ballman wrote:

After some more thought and some offline discussions, I think I have a reasonable way forward: let's add -Wsingle-bit-bitfield-constant-conversion as a new warning group under -Wbitfield-constant-conversion that controls the diagnostic for one-bit bitfields. The diagnostic behavior here is pedantically correct and will help catch some bugs for folks, but there's enough existing code using 1 and not misbehaving (probably because they're not inspecting the value except in a boolean context) that having separate control seems useful.

What do others think?

I went ahead and posted https://reviews.llvm.org/D132851.

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>`_.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags
 ------------------
 
 Deprecated Compiler Flags

clang/lib/Sema/SemaChecking.cpp

static bool AnalyzeBitFieldAssignment(Sema &S, FieldDecl *Bitfield, Expr *Init,
   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;
 
-  // Special-case bitfields of width 1: booleans are naturally 0/1, and
-  // therefore don't strictly fit into a signed bitfield of width 1.
-  if (FieldWidth == 1 && Value == 1)
-    return false;
-
   std::string PrettyValue = toString(Value, 10);
   std::string PrettyTrunc = toString(TruncatedValue, 10);
 
   S.Diag(InitLoc, diag::warn_impcast_bitfield_precision_constant)
     << PrettyValue << PrettyTrunc << OriginalInit->getType()
     << Init->getSourceRange();
 
   return true;

clang/test/CXX/class/class.bit/p1.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
 
 // Test various bit-field member declarations.
 constexpr int foo() { return 1; }
 struct A {
   int a [[]] : 1;
   int b, [[]] : 0; // expected-error {{an attribute list cannot appear here}}
   int [[]] : 0; // OK, attribute applies to the type.
   int [[]] c : 1; // OK, attribute applies to the type.
   int : 2 = 1; // expected-error {{anonymous bit-field cannot have a default member initializer}}
   int : 0 { 1 }; // expected-error {{anonymous bit-field cannot have a default member initializer}}
-  int : 0, d : 1 = 1;
+  unsigned int : 0, d : 1 = 1;
   int : 1 = 12, e : 1; // expected-error {{anonymous bit-field cannot have a default member initializer}}
-  int : 0, f : 1 = 1;
-  int g [[]] : 1 = 1;
-  int h [[]] : 1 {1};
-  int i : foo() = foo();
+  unsigned int : 0, f : 1 = 1;
+  unsigned int g [[]] : 1 = 1;
+  unsigned int h [[]] : 1 {1};
+  unsigned int i : foo() = foo();
   int j, [[]] k; // expected-error {{an attribute list cannot appear here}}
 };

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, "");
-  static_assert(A<int>{1}.n < 0, "");
-  static_assert(A<long long>{1}.n < 0, "");
+  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}}
 #endif
 }
 
 // dr676: na
 
 namespace dr677 { // dr677: no
   struct A {
     void *operator new(std::size_t);

clang/test/CodeGen/bitfield.c

 
 int g3(void) {
 // CHECK-LABEL: @g3()
 // CHECK: ret i32 1
 // PATH-LABEL: @g3()
 // PATH: ret i32 1
   return f3(20) + 130725747;
 }
+
+static int f4(void) {
+  struct s5 {
+    int b:1;
+  } x;
+  x.b = 1;
+  return x.b;
+}
+
+int g4(void) {
+// CHECK-LABEL: @g4()
+// CHECK: ret i32 1
+// PATH-LABEL: @g4()
+// PATH: ret i32 1
+  return f4() + 2;
+}

clang/test/CodeGenCXX/bitfield-layout.cpp

int test_trunc_three_bits() {
   union {
     int i : 3; // truncated to 0b111 == -1
   } const U = {15};  // 0b00001111
   return U.i;
 }
 // CHECK: define{{.*}} i32 @test_trunc_three_bits()
 // CHECK: ret i32 -1
 
+int test_trunc_one_bit() {
+  union {
+    int i : 1; // truncated to 0b1 == -1
+  } const U = {1};  // 0b00000001
+  return U.i;
+}
+// CHECK: define{{.*}} i32 @test_trunc_one_bit()
+// CHECK: ret i32 -1
+
 int test_trunc_1() {
   union {
     int i : 1; // truncated to 0b1 == -1
   } const U = {15};  // 0b00001111
   return U.i;
 }
 // CHECK: define{{.*}} i32 @test_trunc_1()
 // CHECK: ret i32 -1

clang/test/Rewriter/rewrite-modern-struct-ivar-1.mm

union {
             void *_reserved;
         } _multipleRanges;
     } _internal;    
 }
 @end
 @implementation Foo
 - (void)x:(Foo *)other {
   bar.x = 0;
-  bar.y = 1;
+  bar.y = -1;
   self->_internal._singleRange._range = (( other ->bar.x) ? &( other ->_internal._singleRange._range) : ((NSRange *)(&(((_NSRangeInfo *)( other ->_internal._multipleRanges._data))->_ranges))))[0];
 }
 @end
 @interface FooS : Foo
 @end
 @implementation FooS
 - (void)y {
 
   NSUInteger asdf =  (( self ->bar.x) ? 1 : ((_NSRangeInfo *)( self ->_internal._multipleRanges._data))->_capacity ); 
 }
 @end

clang/test/Rewriter/rewrite-modern-struct-ivar.mm

struct _S {
         int y:1;
     } s;
 
 }
 @end
 @implementation Foo
 - (void)x {
   bar.x = 0;
-  bar.y = 1;
+  bar.y = -1;
 
   s.x = 0;
-  s.y = 1;
+  s.y = -1;
 }
 @end
 
 // CHECK: (*(decltype(((Foo_IMPL *)0U)->bar) *)((char *)self + OBJC_IVAR_$_Foo$bar)).x = 0;
 // CHECK: (*(struct _S *)((char *)self + OBJC_IVAR_$_Foo$s)).x = 0;

clang/test/Sema/constant-conversion.c

 
 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 { int bit : 1; } a;
-  a.bit = 1; // shouldn't warn
+  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}}
 }
 
 enum Test2 { K_zero, K_one };
 enum Test2 test2(enum Test2 *t) {
   *t = 20;
   return 10; // shouldn't warn
 }
 
void test10(void) {
   s.a = 15;
   s.a = -8;
   s.a = ~0;
   s.a = ~0U;
   s.a = ~(1<<A);
 
   s.a = -9;  // expected-warning{{implicit truncation from 'int' to bit-field changes value from -9 to 7}}
   s.a = 16;  // expected-warning{{implicit truncation from 'int' to bit-field changes value from 16 to 0}}
 }

clang/test/SemaCXX/constant-expression-cxx11.cpp

namespace Bitfields {
   namespace PR16755 {
     struct X {
       int x : 1;
       constexpr static int f(int x) {
         return X{x}.x;
       }
     };
     static_assert(X::f(3) == -1, "3 should truncate to -1");
+    static_assert(X::f(1) == -1, "1 should truncate to -1");
   }
 
   struct HasUnnamedBitfield {
     unsigned a;
     unsigned : 20;
     unsigned b;
 
     constexpr HasUnnamedBitfield() : a(), b() {}

clang/test/SemaCXX/cxx1z-decomposition.cpp

static auto [m] = S(); // expected-note {{'m' declared here}} \
                          // expected-warning {{C++20}}
 
   struct R { int f() { return m; } };
   (void) [&] { return m; };
   (void)[m]{}; // expected-error {{'m' cannot be captured because it does not have automatic storage duration}}
 
   (void)[outerbit1]{}; // expected-error {{'outerbit1' cannot be captured because it does not have automatic storage duration}}
 
-  auto [bit, var] = S2{1, 1}; // expected-note 2{{'bit' declared here}}
+  auto [bit, var] = S2{-1, 1}; // expected-note 2{{'bit' declared here}}
 
   (void)[&bit] { // expected-error {{non-const reference cannot bind to bit-field 'a'}} \
                     // expected-warning {{C++20}}
     return bit;
   };
 
   union { // expected-note {{declared here}}
     int u;

clang/test/SemaCXX/decomposition-blocks.cpp

 // RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s -fblocks
 
 struct S {
   int i : 1;
   int j;
 };
 
 void run(void (^)());
 void test() {
-  auto [i, j] = S{1, 42}; // expected-note {{'i' declared here}}
+  auto [i, j] = S{-1, 42}; // expected-note {{'i' declared here}}
   run(^{
     (void)i; // expected-error {{reference to local binding 'i' declared in enclosing function 'test'}}
   });
 }