[C23] Fixed the value of BOOL_WIDTH #117364

AaronBallman · 2024-11-22T19:20:38Z

The standard mandates that this returns the width of the type, which is the number of bits in the value. For bool, that's required to be 1 explicitly.

Fixes #117348

llvmbot · 2024-11-22T19:21:13Z

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

The standard mandates that this returns the width of the type, which is the number of bits in the value. For bool, that's required to be 1 explicitly.

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

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/lib/Frontend/InitPreprocessor.cpp (+9-1)
(added) clang/test/C/C23/n2412.c (+27)
(modified) clang/test/Preprocessor/init-aarch64.c (+2-2)
(modified) clang/test/Preprocessor/init-loongarch.c (+2-2)
(modified) clang/test/Preprocessor/init.c (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8bd06fadfdc984..ff8c2f92e875f8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -332,6 +332,8 @@ C23 Feature Support
 
 - Clang now supports `N3029 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3029.htm>`_ Improved Normal Enumerations.
 - Clang now officially supports `N3030 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3030.htm>`_ Enhancements to Enumerations. Clang already supported it as an extension, so there were no changes to compiler behavior.
+- Fixed the value of ``BOOL_WIDTH`` in ``<limits.h>`` to return ``1``
+  explicitly, as mandated by the standard.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
diff --git a/clang/lib/Frontend/InitPreprocessor.cpp b/clang/lib/Frontend/InitPreprocessor.cpp
index 9a0fdb175ff29e..6ecb201fd46970 100644
--- a/clang/lib/Frontend/InitPreprocessor.cpp
+++ b/clang/lib/Frontend/InitPreprocessor.cpp
@@ -1103,7 +1103,15 @@ static void InitializePredefinedMacros(const TargetInfo &TI,
   assert(TI.getCharWidth() == 8 && "Only support 8-bit char so far");
   Builder.defineMacro("__CHAR_BIT__", Twine(TI.getCharWidth()));
 
-  Builder.defineMacro("__BOOL_WIDTH__", Twine(TI.getBoolWidth()));
+  // The macro is specifying the number of bits in the value representation,
+  // not the number of bits in the object representation, which is what
+  // getBoolWidth() will return. For the bool/_Bool data type, there is only
+  // ever one bit in the value representation. See C23 6.2.6.2p2 for the rules
+  // in C. Note that [basic.fundamental]p10 allows an implementation-defined
+  // value representation for bool; Clang represents bool as an i1 when
+  // lowering to LLVM, so this value is also correct for C++ for our
+  // implementation.
+  Builder.defineMacro("__BOOL_WIDTH__", "1");
   Builder.defineMacro("__SHRT_WIDTH__", Twine(TI.getShortWidth()));
   Builder.defineMacro("__INT_WIDTH__", Twine(TI.getIntWidth()));
   Builder.defineMacro("__LONG_WIDTH__", Twine(TI.getLongWidth()));
diff --git a/clang/test/C/C23/n2412.c b/clang/test/C/C23/n2412.c
new file mode 100644
index 00000000000000..7d4f32ae68a734
--- /dev/null
+++ b/clang/test/C/C23/n2412.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -verify -std=c23 -ffreestanding %s
+
+/* WG14 N2412: Clang 14
+ * Two's complement sign representation
+ */
+// expected-no-diagnostics
+
+#include <limits.h>
+
+// GH117348 -- BOOL_WIDTH was accidentally expanding to the number of bits in
+// the object representation (8) rather than the number of bits in the value
+// representation (1).
+static_assert(BOOL_WIDTH == 1);
+
+// Validate the other macro requirements.
+static_assert(CHAR_WIDTH == SCHAR_WIDTH);
+static_assert(CHAR_WIDTH == UCHAR_WIDTH);
+static_assert(CHAR_WIDTH == CHAR_BIT);
+
+static_assert(USHRT_WIDTH >= 16);
+static_assert(UINT_WIDTH >= 16);
+static_assert(ULONG_WIDTH >= 32);
+static_assert(ULLONG_WIDTH >= 64);
+static_assert(BITINT_MAXWIDTH >= ULLONG_WIDTH);
+
+static_assert(MB_LEN_MAX >= 1);
+
diff --git a/clang/test/Preprocessor/init-aarch64.c b/clang/test/Preprocessor/init-aarch64.c
index c52c49a94e016e..8ee6c6ba60af43 100644
--- a/clang/test/Preprocessor/init-aarch64.c
+++ b/clang/test/Preprocessor/init-aarch64.c
@@ -44,7 +44,7 @@
 // AARCH64:      #define __BIGGEST_ALIGNMENT__ 16
 // AARCH64_BE-NEXT: #define __BIG_ENDIAN__ 1
 // AARCH64-NEXT: #define __BITINT_MAXWIDTH__ 128
-// AARCH64-NEXT: #define __BOOL_WIDTH__ 8
+// AARCH64-NEXT: #define __BOOL_WIDTH__ 1
 // AARCH64_BE-NEXT: #define __BYTE_ORDER__ __ORDER_BIG_ENDIAN__
 // AARCH64_LE-NEXT: #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
 // AARCH64-NEXT: #define __CHAR16_TYPE__ unsigned short
@@ -785,7 +785,7 @@
 // ARM64EC-MSVC: #define __ATOMIC_SEQ_CST 5
 // ARM64EC-MSVC: #define __BIGGEST_ALIGNMENT__ 16
 // ARM64EC-MSVC: #define __BITINT_MAXWIDTH__ 128
-// ARM64EC-MSVC: #define __BOOL_WIDTH__ 8
+// ARM64EC-MSVC: #define __BOOL_WIDTH__ 1
 // ARM64EC-MSVC: #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
 // ARM64EC-MSVC: #define __CHAR16_TYPE__ unsigned short
 // ARM64EC-MSVC: #define __CHAR32_TYPE__ unsigned int
diff --git a/clang/test/Preprocessor/init-loongarch.c b/clang/test/Preprocessor/init-loongarch.c
index 0eb6977a2553c9..0e3320f01b328c 100644
--- a/clang/test/Preprocessor/init-loongarch.c
+++ b/clang/test/Preprocessor/init-loongarch.c
@@ -25,7 +25,7 @@
 // LA32-NEXT: #define __ATOMIC_SEQ_CST 5
 // LA32: #define __BIGGEST_ALIGNMENT__ 16
 // LA32: #define __BITINT_MAXWIDTH__ 128
-// LA32: #define __BOOL_WIDTH__ 8
+// LA32: #define __BOOL_WIDTH__ 1
 // LA32: #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
 // LA32: #define __CHAR16_TYPE__ unsigned short
 // LA32: #define __CHAR32_TYPE__ unsigned int
@@ -346,7 +346,7 @@
 // LA64-NEXT: #define __ATOMIC_SEQ_CST 5
 // LA64: #define __BIGGEST_ALIGNMENT__ 16
 // LA64: #define __BITINT_MAXWIDTH__ 128
-// LA64: #define __BOOL_WIDTH__ 8
+// LA64: #define __BOOL_WIDTH__ 1
 // LA64: #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
 // LA64: #define __CHAR16_TYPE__ unsigned short
 // LA64: #define __CHAR32_TYPE__ unsigned int
diff --git a/clang/test/Preprocessor/init.c b/clang/test/Preprocessor/init.c
index c177975114332a..05225c120b13de 100644
--- a/clang/test/Preprocessor/init.c
+++ b/clang/test/Preprocessor/init.c
@@ -1617,7 +1617,7 @@
 // WEBASSEMBLY-NEXT:#define __ATOMIC_SEQ_CST 5
 // WEBASSEMBLY-NEXT:#define __BIGGEST_ALIGNMENT__ 16
 // WEBASSEMBLY-NEXT:#define __BITINT_MAXWIDTH__ 128
-// WEBASSEMBLY-NEXT:#define __BOOL_WIDTH__ 8
+// WEBASSEMBLY-NEXT:#define __BOOL_WIDTH__ 1
 // WEBASSEMBLY-NEXT:#define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
 // WEBASSEMBLY-NEXT:#define __CHAR16_TYPE__ unsigned short
 // WEBASSEMBLY-NEXT:#define __CHAR32_TYPE__ unsigned int

Sirraide

LGTM

jrtc27 · 2024-11-22T19:22:43Z

clang/test/C/C23/n2412.c

+static_assert(BITINT_MAXWIDTH >= ULLONG_WIDTH);
+
+static_assert(MB_LEN_MAX >= 1);
+


Suggested change

jrtc27 · 2024-11-22T19:25:15Z

clang/lib/Frontend/InitPreprocessor.cpp

+  // not the number of bits in the object representation, which is what
+  // getBoolWidth() will return. For the bool/_Bool data type, there is only
+  // ever one bit in the value representation. See C23 6.2.6.2p2 for the rules
+  // in C. Note that [basic.fundamental]p10 allows an implementation-defined


Mention C++ before that reference? Obvious for those who know both standards and how they're typically referenced, but a bit odd to switch from C to C++ without being explicit, and then only mentioning C++ at the end.

jrtc27 · 2024-11-22T19:26:56Z

clang/lib/Frontend/InitPreprocessor.cpp

-  Builder.defineMacro("__BOOL_WIDTH__", Twine(TI.getBoolWidth()));
+  // The macro is specifying the number of bits in the value representation,
+  // not the number of bits in the object representation, which is what
+  // getBoolWidth() will return. For the bool/_Bool data type, there is only


Hm, I guess in an ideal world we'd have getTypeObjectWidth and getTypeValueWidth to avoid assuming they're the same or, in this case, hard-coding 1, but if this is the only case where we need anything different from what we have today then that would be a bit overkill.

Yes, on modern architectures, bool and _BitInt should be the only integer types that have padding bits.

zygoloid · 2024-11-22T19:34:17Z

clang/lib/Frontend/InitPreprocessor.cpp

+  // in C. Note that [basic.fundamental]p10 allows an implementation-defined
+  // value representation for bool; Clang represents bool as an i1 when
+  // lowering to LLVM, so this value is also correct for C++ for our
+  // implementation.


Clang represents bools as i8 in memory, and for example you can bit_cast between bool and char because our bool has no padding bits.

zygoloid · 2024-11-22T19:34:25Z

clang/lib/Frontend/InitPreprocessor.cpp

@@ -1103,7 +1103,15 @@ static void InitializePredefinedMacros(const TargetInfo &TI,
  assert(TI.getCharWidth() == 8 && "Only support 8-bit char so far");
  Builder.defineMacro("__CHAR_BIT__", Twine(TI.getCharWidth()));

-  Builder.defineMacro("__BOOL_WIDTH__", Twine(TI.getBoolWidth()));
+  // The macro is specifying the number of bits in the value representation,


It's the width, not the number of bits in the value representation (which in C++'s definition of "value representation" would be 8, because all 8 bits affect the determination of the value and whether that value is valid).

[C23] Fixed the value of BOOL_WIDTH

37923ec

AaronBallman added c23 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 22, 2024

AaronBallman requested review from zygoloid, jrtc27, jyknight, efriedma-quic and Sirraide November 22, 2024 19:20

llvmbot added the clang Clang issues not falling into any other category label Nov 22, 2024

Sirraide approved these changes Nov 22, 2024

View reviewed changes

jrtc27 reviewed Nov 22, 2024

View reviewed changes

jyknight approved these changes Nov 22, 2024

View reviewed changes

zygoloid reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C23] Fixed the value of BOOL_WIDTH #117364

[C23] Fixed the value of BOOL_WIDTH #117364

AaronBallman commented Nov 22, 2024 •

edited

Loading

llvmbot commented Nov 22, 2024

Sirraide left a comment

jrtc27 Nov 22, 2024

jrtc27 Nov 22, 2024

jrtc27 Nov 22, 2024

gustedt Nov 23, 2024

zygoloid Nov 22, 2024

zygoloid Nov 22, 2024

		static_assert(BITINT_MAXWIDTH >= ULLONG_WIDTH);

		static_assert(MB_LEN_MAX >= 1);

[C23] Fixed the value of BOOL_WIDTH #117364

Are you sure you want to change the base?

[C23] Fixed the value of BOOL_WIDTH #117364

Conversation

AaronBallman commented Nov 22, 2024 • edited Loading

llvmbot commented Nov 22, 2024

Sirraide left a comment

Choose a reason for hiding this comment

jrtc27 Nov 22, 2024

Choose a reason for hiding this comment

jrtc27 Nov 22, 2024

Choose a reason for hiding this comment

jrtc27 Nov 22, 2024

Choose a reason for hiding this comment

gustedt Nov 23, 2024

Choose a reason for hiding this comment

zygoloid Nov 22, 2024

Choose a reason for hiding this comment

zygoloid Nov 22, 2024

Choose a reason for hiding this comment

AaronBallman commented Nov 22, 2024 •

edited

Loading