[PATCH] D71142: [Sema] Validate large bitfields

[Mark de Wever via Phabricator via cfe-commits]

Mordante planned changes to this revision.
Mordante added a comment.

In D71142#1967409 , @aaron.ballman 
wrote:

> In D71142#1967315 , @xbolva00 wrote:
>
> > Any comments?
> >
> > @rsmith @aaron.ballman
>
>
> There are outstanding review comments, so I'm waiting for the patch author to 
> address those, unless I've missed a question somewhere?


No I still need to address some issues, but I haven't found time for it yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71142#1967315 , @xbolva00 wrote:

> Any comments?
>
> @rsmith @aaron.ballman


There are outstanding review comments, so I'm waiting for the patch author to 
address those, unless I've missed a question somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Any comments?

@rsmith @aaron.ballman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

In D71142#1774270 , @xbolva00 wrote:

> Maybe a bit offtopic, but it would be good to diagnose this case too:
>
>   enum A {a, b, c = 8};
>  
>   struct T {
>   enum A field : 2;
>   };
>
>
> GCC catches this bug.


I created https://bugs.llvm.org/show_bug.cgi?id=44251 and will look at adding 
this warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Maybe a bit offtopic, but it would be good to diagnose this case too:

  enum A {a, b, c = 8};
  
  struct T {
  enum A field : 2;
  };

GCC catches this bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added a comment.

Further testing revealed the Codegen already has decided the limit.
`clang/lib/CodeGen/CGRecordLayout.h:64`:

  struct CGBitFieldInfo {
/// The offset within a contiguous run of bitfields that are represented as
/// a single "field" within the LLVM struct type. This offset is in bits.
unsigned Offset : 16;
  
/// The total size of the bit-field, in bits.
unsigned Size : 15;

So I'll look at using that limit. I also want to look at improving the 
diagnostics a bit more.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5186-5189
+def warn_bitfield_width_exceeds_maximum_width: Error<
+  "width of bit-field %0 doesn't fit in a 64 bit unsigned integer">;
+def warn_anon_bitfield_width_exceeds_maximum_width : Error<
+  "width of anonymous bit-field doesn't fit in a 64 bit unsigned integer">;

aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > I feel like this situation should be an error rather than a warning -- 
> > > what could the code possibly have meant?
> > The name of the diagnostic and the kind of diagnostic should agree. 
> > Currently we have `warn_` vs `Error<`.
> Ooof, good catch! These diagnostics should be renamed to start with `err_` 
> instead.
Hmm odd I'm sure I left a comment here yesterday, but it seems I didn't commit 
it properly.
It is a copy paste bug and I forgot to change the `warn_` prefix to `err_`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71142#1773933 , @Mordante wrote:

> I'll have a look whether I can find a sane maximum width for a bit-field.


The limits according to C are the size of the field's declared type (C17 
6.7.2.1p4), but it seems that C++ decided it would make sense to turn the bits 
into padding bits (http://eel.is/c++draft/class.bit#1.sentence-6), so I guess a 
reasonable implementation limit would be `SIZE_MAX` for the target architecture 
(aka, `sizeof(size_t) * CHAR_BIT` bits).




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5186-5189
+def warn_bitfield_width_exceeds_maximum_width: Error<
+  "width of bit-field %0 doesn't fit in a 64 bit unsigned integer">;
+def warn_anon_bitfield_width_exceeds_maximum_width : Error<
+  "width of anonymous bit-field doesn't fit in a 64 bit unsigned integer">;

rsmith wrote:
> aaron.ballman wrote:
> > I feel like this situation should be an error rather than a warning -- what 
> > could the code possibly have meant?
> The name of the diagnostic and the kind of diagnostic should agree. Currently 
> we have `warn_` vs `Error<`.
Ooof, good catch! These diagnostics should be renamed to start with `err_` 
instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 3 inline comments as done.
Mordante added a comment.

I'll have a look whether I can find a sane maximum width for a bit-field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

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

rsmith added a comment.

It seems fine and reasonable to reject this as a violation of an implementation 
limit. Can we actually support the full range 2^64 bits range, or would it be 
wiser to pick a smaller limit? (There's at least no point in allowing 
bit-widths that would mean the size of the bitfield doesn't fit in `size_t`, 
which means no more than 35 bits for a 32-bit target.)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5186-5189
+def warn_bitfield_width_exceeds_maximum_width: Error<
+  "width of bit-field %0 doesn't fit in a 64 bit unsigned integer">;
+def warn_anon_bitfield_width_exceeds_maximum_width : Error<
+  "width of anonymous bit-field doesn't fit in a 64 bit unsigned integer">;

aaron.ballman wrote:
> I feel like this situation should be an error rather than a warning -- what 
> could the code possibly have meant?
The name of the diagnostic and the kind of diagnostic should agree. Currently 
we have `warn_` vs `Error<`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5186-5189
+def warn_bitfield_width_exceeds_maximum_width: Error<
+  "width of bit-field %0 doesn't fit in a 64 bit unsigned integer">;
+def warn_anon_bitfield_width_exceeds_maximum_width : Error<
+  "width of anonymous bit-field doesn't fit in a 64 bit unsigned integer">;

I feel like this situation should be an error rather than a warning -- what 
could the code possibly have meant?



Comment at: clang/lib/Sema/SemaDecl.cpp:15845
 
+// Don't accept too wide bit-fields they will cause assertion failures
+// when used.

too wide -> too-wide
bit-fields they -> bit-fields, they


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71142



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

[PATCH] D71142: [Sema] Validate large bitfields

[Mark de Wever via Phabricator via cfe-commits]

Mordante created this revision.
Mordante added reviewers: rsmith, majnemer, aaron.ballman.
Mordante added a project: clang.

Note: I'm not entirely happy with the text of the diagnostics and I'm open to 
suggestions.

Fixes PR23505: Large bitfield: Assertion `getActiveBits() <= 64 && "Too many 
bits for uint64_t"' failed


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71142

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Sema/bitfield.c


Index: clang/test/Sema/bitfield.c
===
--- clang/test/Sema/bitfield.c
+++ clang/test/Sema/bitfield.c
@@ -28,6 +28,12 @@
 
   _Bool : 2;   // expected-error {{width of anonymous bit-field (2 bits) 
exceeds width of its type (1 bit)}}
   _Bool h : 5; // expected-error {{width of bit-field 'h' (5 bits) exceeds 
width of its type (1 bit)}}
+
+  // PR23505
+  _Bool : 1 + (unsigned __int128)0x;   // expected-error 
{{width of anonymous bit-field doesn't fit in a 64 bit unsigned integer}}
+  _Bool i : 1 + (unsigned __int128)0x; // expected-error 
{{width of bit-field 'i' doesn't fit in a 64 bit unsigned integer}}
+  int : 1 + (unsigned __int128)0x; // expected-error 
{{width of anonymous bit-field doesn't fit in a 64 bit unsigned integer}}
+  int j : 1 + (unsigned __int128)0x;   // expected-error 
{{width of bit-field 'j' doesn't fit in a 64 bit unsigned integer}}
 };
 
 struct b {unsigned x : 2;} x;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15842,6 +15842,17 @@
 uint64_t TypeWidth = Context.getIntWidth(FieldTy);
 bool BitfieldIsOverwide = Value.ugt(TypeWidth);
 
+// Don't accept too wide bit-fields they will cause assertion failures
+// when used.
+if (BitfieldIsOverwide && Value.ugt(UINT64_MAX)) {
+  if (FieldName)
+return Diag(FieldLoc, diag::warn_bitfield_width_exceeds_maximum_width)
+   << FieldName;
+
+  return Diag(FieldLoc,
+  diag::warn_anon_bitfield_width_exceeds_maximum_width);
+}
+
 // Over-wide bitfields are an error in C or when using the MSVC bitfield
 // ABI.
 bool CStdConstraintViolation =
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5183,6 +5183,10 @@
   "number of elements must be either one or match the size of the vector">;
 
 // Used by C++ which allows bit-fields that are wider than the type.
+def warn_bitfield_width_exceeds_maximum_width: Error<
+  "width of bit-field %0 doesn't fit in a 64 bit unsigned integer">;
+def warn_anon_bitfield_width_exceeds_maximum_width : Error<
+  "width of anonymous bit-field doesn't fit in a 64 bit unsigned integer">;
 def warn_bitfield_width_exceeds_type_width: Warning<
   "width of bit-field %0 (%1 bits) exceeds the width of its type; value will "
   "be truncated to %2 bit%s2">, InGroup;


Index: clang/test/Sema/bitfield.c
===
--- clang/test/Sema/bitfield.c
+++ clang/test/Sema/bitfield.c
@@ -28,6 +28,12 @@
 
   _Bool : 2;   // expected-error {{width of anonymous bit-field (2 bits) exceeds width of its type (1 bit)}}
   _Bool h : 5; // expected-error {{width of bit-field 'h' (5 bits) exceeds width of its type (1 bit)}}
+
+  // PR23505
+  _Bool : 1 + (unsigned __int128)0x;   // expected-error {{width of anonymous bit-field doesn't fit in a 64 bit unsigned integer}}
+  _Bool i : 1 + (unsigned __int128)0x; // expected-error {{width of bit-field 'i' doesn't fit in a 64 bit unsigned integer}}
+  int : 1 + (unsigned __int128)0x; // expected-error {{width of anonymous bit-field doesn't fit in a 64 bit unsigned integer}}
+  int j : 1 + (unsigned __int128)0x;   // expected-error {{width of bit-field 'j' doesn't fit in a 64 bit unsigned integer}}
 };
 
 struct b {unsigned x : 2;} x;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15842,6 +15842,17 @@
 uint64_t TypeWidth = Context.getIntWidth(FieldTy);
 bool BitfieldIsOverwide = Value.ugt(TypeWidth);
 
+// Don't accept too wide bit-fields they will cause assertion failures
+// when used.
+if (BitfieldIsOverwide && Value.ugt(UINT64_MAX)) {
+  if (FieldName)
+return Diag(FieldLoc, diag::warn_bitfield_width_exceeds_maximum_width)
+   << FieldName;
+
+  return Diag(FieldLoc,
+  diag::warn_anon_bitfield_width_exceeds_maximum_width);
+}
+
 // Over-wide bitfields are an error