Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C23] Select the correct promoted type for a bit-field #89254

Merged

Conversation

AaronBallman
Copy link
Collaborator

Bit-fields of bit-precise integer type do not promote to int, but instead promote to the type of the field.

Fixes #87641

Bit-fields of bit-precise integer type do not promote to int, but
instead promote to the type of the field.

Fixes llvm#87641
@AaronBallman AaronBallman added c23 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 18, 2024
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Apr 18, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 18, 2024

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

Bit-fields of bit-precise integer type do not promote to int, but instead promote to the type of the field.

Fixes #87641


Full diff: https://github.com/llvm/llvm-project/pull/89254.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/lib/AST/ASTContext.cpp (+8)
  • (added) clang/test/Sema/bitint-bitfield-promote.c (+38)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3fe15934323c53..1b5d69059230e6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -179,6 +179,9 @@ C23 Feature Support
 - Clang now supports `N3018 The constexpr specifier for object definitions`
   <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3018.htm>`_.
 
+- Properly promote bit-fields of bit-precise integer types to the field's type
+  rather than to ``int``. #GH87641
+
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index b974fc28283c77..b36fb5523af5a6 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -7241,6 +7241,14 @@ QualType ASTContext::isPromotableBitField(Expr *E) const {
   //        We perform that promotion here to match GCC and C++.
   // FIXME: C does not permit promotion of an enum bit-field whose rank is
   //        greater than that of 'int'. We perform that promotion to match GCC.
+  //
+  // C23 6.3.1.1p2:
+  //   The value from a bit-field of a bit-precise integer type is converted to
+  //   the corresponding bit-precise integer type. (The rest is the same as in
+  //   C11.)
+  if (QualType QT = Field->getType(); QT->isBitIntType())
+    return QT;
+
   if (BitWidth < IntSize)
     return IntTy;
 
diff --git a/clang/test/Sema/bitint-bitfield-promote.c b/clang/test/Sema/bitint-bitfield-promote.c
new file mode 100644
index 00000000000000..df09c40efca390
--- /dev/null
+++ b/clang/test/Sema/bitint-bitfield-promote.c
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c23 %s
+
+// GH87641 noticed that integer promotion of a bit-field of bit-precise integer
+// type was promoting to int rather than the type of the bit-field.
+struct S {
+  unsigned _BitInt(7) x : 2;
+  unsigned _BitInt(2) y : 2;
+  unsigned _BitInt(72) z : 28;
+};
+
+// We don't have to worry about promotion cases where the bit-precise type is
+// smaller than the width of the bit-field; that can't happen.
+struct T {
+  unsigned _BitInt(28) oh_no : 72; // expected-error {{width of bit-field 'oh_no' (72 bits) exceeds the width of its type (28 bits)}}
+};
+
+static_assert(
+  _Generic(+(struct S){}.x,
+    int : 0,
+    unsigned _BitInt(7) : 1,
+    unsigned _BitInt(2) : 2
+  ) == 1);
+
+static_assert(
+  _Generic(+(struct S){}.y,
+    int : 0,
+    unsigned _BitInt(7) : 1,
+    unsigned _BitInt(2) : 2
+  ) == 2);
+
+static_assert(
+  _Generic(+(struct S){}.z,
+    int : 0,
+    unsigned _BitInt(72) : 1,
+    unsigned _BitInt(28) : 2
+  ) == 1);
+
+

@AaronBallman
Copy link
Collaborator Author

AaronBallman commented Apr 18, 2024

@tbaederr -- this triggers an assertion with the new constexpr interpreter in clang/test/AST/Interp/intap.cpp but I'm not certain I understand why. Can you help me figure out how to fix that?

https://buildkite.com/llvm-project/github-pull-requests/builds/56785#018ef1ee-1e9c-48be-8944-0bce2c6f441d/6-1940

@tbaederr
Copy link
Contributor

tbaederr commented Apr 19, 2024

@tbaederr -- this triggers an assertion with the new constexpr interpreter in clang/test/AST/Interp/intap.cpp but I'm not certain I understand why. Can you help me figure out how to fix that?

https://buildkite.com/llvm-project/github-pull-requests/builds/56785#018ef1ee-1e9c-48be-8944-0bce2c6f441d/6-1940

In IntegralAP::truncate() (which we call when getting the 3 and setting it as the value of the bitfield), we actually truncate the bitwith of the underlying APInt, not just the value.

This fixes things for me:

diff --git a/clang/lib/AST/Interp/IntegralAP.h b/clang/lib/AST/Interp/IntegralAP.h
index bab9774288bf..fb7ee1451571 100644
--- a/clang/lib/AST/Interp/IntegralAP.h
+++ b/clang/lib/AST/Interp/IntegralAP.h
@@ -154,7 +154,10 @@ public:
   }

   IntegralAP truncate(unsigned BitWidth) const {
-    return IntegralAP(V.trunc(BitWidth));
+    if constexpr (Signed)
+      return IntegralAP(V.trunc(BitWidth).sextOrTrunc(this->bitWidth()));
+    else
+      return IntegralAP(V.trunc(BitWidth).zextOrTrunc(this->bitWidth()));
   }

   IntegralAP<false> toUnsigned() const {

I'd push this (it doesn't break any existing tests), but I'm not 100% sure if this is the right way to do that. Also, I should follow up with a truncate -> truncateValue rename.

@AaronBallman
Copy link
Collaborator Author

@tbaederr -- this triggers an assertion with the new constexpr interpreter in clang/test/AST/Interp/intap.cpp but I'm not certain I understand why. Can you help me figure out how to fix that?
https://buildkite.com/llvm-project/github-pull-requests/builds/56785#018ef1ee-1e9c-48be-8944-0bce2c6f441d/6-1940

In IntegralAP::truncate() (which we call when getting the 3 and setting it as the value of the bitfield), we actually truncate the bitwith of the underlying IntAP, not just the value.

This fixes things for me:

diff --git a/clang/lib/AST/Interp/IntegralAP.h b/clang/lib/AST/Interp/IntegralAP.h
index bab9774288bf..fb7ee1451571 100644
--- a/clang/lib/AST/Interp/IntegralAP.h
+++ b/clang/lib/AST/Interp/IntegralAP.h
@@ -154,7 +154,10 @@ public:
   }

   IntegralAP truncate(unsigned BitWidth) const {
-    return IntegralAP(V.trunc(BitWidth));
+    if constexpr (Signed)
+      return IntegralAP(V.trunc(BitWidth).sextOrTrunc(this->bitWidth()));
+    else
+      return IntegralAP(V.trunc(BitWidth).zextOrTrunc(this->bitWidth()));
   }

   IntegralAP<false> toUnsigned() const {

I'd push this (it doesn't break any existing tests), but I'm not 100% sure if this is the right way to do that. Also, I should follow up with a truncate -> truncateValue rename.

Thank you for the help, that does pass all the tests, so I've added a commit for it.

Copy link
Contributor

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment but LGTM otherwise.

clang/test/Sema/bitint-bitfield-promote.c Show resolved Hide resolved
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks right to me! I don't know if there is value in a signed-test instead of unsigned, but a couple more tests might be nice anyway.

clang/test/Sema/bitint-bitfield-promote.c Show resolved Hide resolved
@AaronBallman AaronBallman merged commit 947cd67 into llvm:main Apr 22, 2024
4 of 5 checks passed
@AaronBallman AaronBallman deleted the aballman-bit-int-bit-field-promote branch April 22, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c23 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect behavior when promoting bit-field of bit-precise type
5 participants