[C23] Fixed the value of BOOL_WIDTH #117364
Merged
+43
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AaronBallman
added
c23
clang:frontend
AaronBallman
requested review from
zygoloid,
jrtc27,
jyknight,
efriedma-quic and
Sirraide
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesThe 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 Full diff: https://github.com/llvm/llvm-project/pull/117364.diff 6 Files Affected:
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
|
jrtc27
reviewed
Nov 22, 2024
| static_assert(BITINT_MAXWIDTH >= ULLONG_WIDTH); | ||
|
|
||
| static_assert(MB_LEN_MAX >= 1); | ||
|
|
There was a problem hiding this comment.
Pretty sure that's the EOL for the last line in the file, which we want to keep.
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good call, done
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, on modern architectures, bool and _BitInt should be the only integer types that have padding bits.
jyknight
approved these changes
Nov 22, 2024
zygoloid
reviewed
Nov 22, 2024
| // 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. |
There was a problem hiding this comment.
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
reviewed
Nov 22, 2024
| @@ -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, | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Good catch, fixed.
tbaederr
reviewed
Nov 25, 2024
Co-authored-by: Timm Baeder <[email protected]>
zygoloid
approved these changes
Nov 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
1explicitly.Fixes #117348