[PATCH] D131255: Fix Wbitfield-constant-conversion on 1-bit signed bitfield

Mon Aug 15 04:35:52 PDT 2022

aaron.ballman added inline comments.

================
Comment at: clang/lib/Sema/SemaChecking.cpp:13069
-  // therefore don't strictly fit into a signed bitfield of width 1.
-  if (FieldWidth == 1 && Value == 1)
-    return false;
----------------
ShawnZhong wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > ShawnZhong wrote:
> > > > ShawnZhong wrote:
> > > > > ShawnZhong wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > thakis wrote:
> > > > > > > > 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.
> > > > > > > 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
> > > > > > >     ...
> > > > > > >   }
> > > > > > > }
> > > > > > > ```
> > > > > > Do you have something in mind that counts as false positives? 
> > > > > 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"); }
> > > > > }
> > > > > ```
> > > > > 
> > > > > 
> > > > For reference, I found this issue on chromium: 
> > > > 
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=1352183
> > > > 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.
> > >>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.
> > 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 `bool`s 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
> }
> ```
> 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.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131255