[libc] Add C23 limits.h header. #78887
Conversation
lntue
requested review from
gchatelet,
nickdesaulniers,
jhuber6 and
michaelrj-google
|
@llvm/pr-subscribers-libc Author: None (lntue) ChangesFull diff: https://github.com/llvm/llvm-project/pull/78887.diff 12 Files Affected:
diff --git a/libc/config/darwin/arm/headers.txt b/libc/config/darwin/arm/headers.txt
index 2dd54b7c1f50535..d80628445af5c52 100644
--- a/libc/config/darwin/arm/headers.txt
+++ b/libc/config/darwin/arm/headers.txt
@@ -4,6 +4,7 @@ set(TARGET_PUBLIC_HEADERS
libc.include.fenv
libc.include.float
libc.include.inttypes
+ libc.include.limits
libc.include.math
libc.include.stdlib
libc.include.string
diff --git a/libc/config/darwin/x86_64/headers.txt b/libc/config/darwin/x86_64/headers.txt
index 510d62688a45fa3..1e81e303ddd6b47 100644
--- a/libc/config/darwin/x86_64/headers.txt
+++ b/libc/config/darwin/x86_64/headers.txt
@@ -5,6 +5,7 @@ set(TARGET_PUBLIC_HEADERS
#libc.include.fenv
libc.include.float
libc.include.inttypes
+ libc.include.limits
libc.include.math
libc.include.stdlib
libc.include.string
diff --git a/libc/config/gpu/headers.txt b/libc/config/gpu/headers.txt
index e68e18b87b10e27..3b04dd89fafe6f7 100644
--- a/libc/config/gpu/headers.txt
+++ b/libc/config/gpu/headers.txt
@@ -4,6 +4,7 @@ set(TARGET_PUBLIC_HEADERS
libc.include.string
libc.include.float
libc.include.inttypes
+ libc.include.limits
libc.include.math
libc.include.fenv
libc.include.time
diff --git a/libc/config/linux/aarch64/headers.txt b/libc/config/linux/aarch64/headers.txt
index 709a8dbdedc98f1..60d978bcaa76e44 100644
--- a/libc/config/linux/aarch64/headers.txt
+++ b/libc/config/linux/aarch64/headers.txt
@@ -6,6 +6,7 @@ set(TARGET_PUBLIC_HEADERS
libc.include.fenv
libc.include.float
libc.include.inttypes
+ libc.include.limits
libc.include.math
libc.include.pthread
libc.include.signal
diff --git a/libc/config/linux/riscv/headers.txt b/libc/config/linux/riscv/headers.txt
index dc1daa48f6c875f..0ade63dc5933b53 100644
--- a/libc/config/linux/riscv/headers.txt
+++ b/libc/config/linux/riscv/headers.txt
@@ -8,6 +8,7 @@ set(TARGET_PUBLIC_HEADERS
libc.include.fenv
libc.include.float
libc.include.inttypes
+ libc.include.limits
libc.include.math
libc.include.pthread
libc.include.sched
diff --git a/libc/config/linux/x86_64/headers.txt b/libc/config/linux/x86_64/headers.txt
index b0e0219a30e673c..d103176897a74a8 100644
--- a/libc/config/linux/x86_64/headers.txt
+++ b/libc/config/linux/x86_64/headers.txt
@@ -8,6 +8,7 @@ set(TARGET_PUBLIC_HEADERS
libc.include.fenv
libc.include.float
libc.include.inttypes
+ libc.include.limits
libc.include.math
libc.include.pthread
libc.include.sched
diff --git a/libc/include/CMakeLists.txt b/libc/include/CMakeLists.txt
index cec8b86516b51f0..89fffd1022758ea 100644
--- a/libc/include/CMakeLists.txt
+++ b/libc/include/CMakeLists.txt
@@ -83,6 +83,14 @@ add_gen_header(
.llvm-libc-macros.float_macros
)
+add_gen_header(
+ limits
+ DEF_FILE limits.h.def
+ GEN_HDR limits.h
+ DEPENDS
+ .llvm-libc-macros.limits_macros
+)
+
add_gen_header(
math
DEF_FILE math.h.def
diff --git a/libc/include/limits.h.def b/libc/include/limits.h.def
new file mode 100644
index 000000000000000..de5f3490459ed8b
--- /dev/null
+++ b/libc/include/limits.h.def
@@ -0,0 +1,14 @@
+//===-- C standard library header limits.h --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_LIMITS_H
+#define LLVM_LIBC_LIMITS_H
+
+#include <llvm-libc-macros/limits-macros.h>
+
+#endif // LLVM_LIBC_LIMITS_H
diff --git a/libc/include/llvm-libc-macros/CMakeLists.txt b/libc/include/llvm-libc-macros/CMakeLists.txt
index d965a6a9443ed2a..96ee29d7327224c 100644
--- a/libc/include/llvm-libc-macros/CMakeLists.txt
+++ b/libc/include/llvm-libc-macros/CMakeLists.txt
@@ -73,6 +73,12 @@ add_macro_header(
float-macros.h
)
+add_macro_header(
+ limits_macros
+ HDR
+ limits-macros.h
+)
+
add_macro_header(
math_macros
HDR
diff --git a/libc/include/llvm-libc-macros/float-macros.h b/libc/include/llvm-libc-macros/float-macros.h
index 8e4b91b9a0f557d..16ccc8e806617a1 100644
--- a/libc/include/llvm-libc-macros/float-macros.h
+++ b/libc/include/llvm-libc-macros/float-macros.h
@@ -9,8 +9,17 @@
#ifndef __LLVM_LIBC_MACROS_FLOAT_MACROS_H
#define __LLVM_LIBC_MACROS_FLOAT_MACROS_H
+#ifdef __clang__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wgnu-include-next"
+#endif //__clang__
+
#include_next <float.h>
+#ifdef __clang__
+#pragma GCC diagnostic pop
+#endif //__clang__
+
#ifndef FLT_RADIX
#define FLT_RADIX __FLT_RADIX__
#endif // FLT_RADIX
diff --git a/libc/include/llvm-libc-macros/limits-macros.h b/libc/include/llvm-libc-macros/limits-macros.h
new file mode 100644
index 000000000000000..e561a95bf3a09a0
--- /dev/null
+++ b/libc/include/llvm-libc-macros/limits-macros.h
@@ -0,0 +1,229 @@
+//===-- Definition of macros from limits.h --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef __LLVM_LIBC_MACROS_LIMITS_MACROS_H
+#define __LLVM_LIBC_MACROS_LIMITS_MACROS_H
+
+#ifdef __clang__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wgnu-include-next"
+#endif // __clang__
+
+#include_next <limits.h>
+
+#ifdef __clang__
+#pragma GCC diagnostic pop
+#endif // __clang__
+
+#ifndef CHAR_BIT
+#ifdef __CHAR_BIT__
+#define CHAR_BIT __CHAR_BIT__
+#else
+#define CHAR_BIT 8
+#endif // __CHAR_BIT__
+#endif // CHAR_BIT
+
+// TODO: define MB_LEN_MAX if missing
+// clang: MB_LEN_MAX = 1 -
+// https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/limits.h#L64
+// glibc: MB_LEN_MAX = 16 -
+// https://github.com/lattera/glibc/blob/master/include/limits.h#L32
+
+// *_WIDTH macros
+
+#ifndef CHAR_WIDTH
+#define CHAR_WIDTH CHAR_BIT
+#endif // CHAR_WIDTH
+
+#ifndef SCHAR_WIDTH
+#define SCHAR_WIDTH CHAR_BIT
+#endif // SCHAR_WIDTH
+
+#ifndef UCHAR_WIDTH
+#define UCHAR_WIDTH CHAR_BIT
+#endif // UCHAR_WIDTH
+
+#ifndef SHRT_WIDTH
+#ifdef __SHRT_WIDTH__
+#define SHRT_WIDTH __SHRT_WIDTH__
+#else
+#define SHRT_WIDTH 16
+#endif // __SHRT_WIDTH__
+#endif // SHRT_WIDTH
+
+#ifndef USHRT_WIDTH
+#define USHRT_WIDTH SHRT_WIDTH
+#endif // USHRT_WIDTH
+
+#ifndef INT_WIDTH
+#ifdef __INT_WIDTH__
+#define INT_WIDTH __INT_WIDTH__
+#else
+#define INT_WIDTH 32
+#endif // __INT_WIDTH__
+#endif // INT_WIDTH
+
+#ifndef UINT_WIDTH
+#define UINT_WIDTH INT_WIDTH
+#endif // UINT_WIDTH
+
+#ifndef LONG_WIDTH
+#ifdef __LONG_WIDTH__
+#define LONG_WIDTH __LONG_WIDTH__
+#elif defined(__WORDSIZE)
+#define LONG_WIDTH __WORDSIZE
+#else
+#error "Unknown WORDSIZE to define LONG_WIDTH."
+#endif // __LONG_WIDTH__
+#endif // LONG_WIDTH
+
+#ifndef ULONG_WIDTH
+#define ULONG_WIDTH LONG_WIDTH
+#endif // ULONG_WIDTH
+
+#ifndef LLONG_WIDTH
+#ifdef __LLONG_WIDTH__
+#define LLONG_WIDTH __LLONG_WIDTH__
+#else
+#define LLONG_WIDTH 64
+#endif // __LLONG_WIDTH__
+#endif // LLONG_WIDTH
+
+#ifndef ULLONG_WIDTH
+#define ULLONG_WIDTH LLONG_WIDTH
+#endif // ULLONG_WIDTH
+
+#ifndef BOOL_WIDTH
+#ifdef __BOOL_WIDTH__
+#define BOOL_WIDTH __BOOL_WIDTH__
+#else
+#define BOOL_WIDTH 1
+#endif // __BOOL_WIDTH__
+#endif // BOOL_WIDTH
+
+// *_MAX macros
+
+#ifndef SCHAR_MAX
+#ifdef __SCHAR_MAX__
+#define SCHAR_MAX __SCHAR_MAX__
+#else
+#define SCHAR_MAX 0x7f
+#endif // __SCHAR_MAX__
+#endif // SCHAR_MAX
+
+#ifndef UCHAR_MAX
+#define UCHAR_MAX (SCHAR_MAX * 2 + 1)
+#endif // UCHAR_MAX
+
+#ifndef CHAR_MAX
+#ifdef __CHAR_UNSIGNED__
+#define CHAR_MAX UCHAR_MAX
+#else
+#define CHAR_MAX SCHAR_MAX
+#endif // __CHAR_UNSIGNED__
+#endif // CHAR_MAX
+
+#ifndef SHRT_MAX
+#ifdef __SHRT_MAX__
+#define SHRT_MAX __SHRT_MAX__
+#else
+#define SHRT_MAX 0x7fff
+#endif // __SHRT_MAX__
+#endif // SHRT_MAX
+
+#ifndef USHRT_MAX
+#define USHRT_MAX (SHRT_MAX * 2U + 1U)
+#endif // USHRT_MAX
+
+#ifndef INT_MAX
+#ifdef __INT_MAX__
+#define INT_MAX __INT_MAX__
+#else
+#define INT_MAX (0 ^ (1 << (INT_WIDTH - 1)))
+#endif // __INT_MAX__
+#endif // INT_MAX
+
+#ifndef UINT_MAX
+#define UINT_MAX (~0U)
+#endif // UINT_MAX
+
+#ifndef LONG_MAX
+#ifdef __LONG_MAX__
+#define LONG_MAX __LONG_MAX__
+#else
+#define LONG_MAX (0L ^ (1L << (LONG_WIDTH - 1)))
+#endif // __LONG_MAX__
+#endif // LONG_MAX
+
+#ifndef ULONG_MAX
+#define ULONG_MAX (~0UL)
+#endif // ULONG_MAX
+
+#ifndef LLONG_MAX
+#ifdef __LONG_LONG_MAX__
+#define LLONG_MAX __LONG_LONG_MAX__
+#else
+#define LLONG_MAX (0LL ^ (1LL << (LLONG_WIDTH - 1)))
+#endif // __LONG_LONG_MAX__
+#endif // LLONG_MAX
+
+#ifndef ULLONG_MAX
+#define ULLONG_MAX (~0ULL)
+#endif // ULLONG_MAX
+
+// *_MIN macros
+
+#ifndef SCHAR_MIN
+#define SCHAR_MIN (-SCHAR_MAX - 1)
+#endif // SCHAR_MIN
+
+#ifndef UCHAR_MIN
+#define UCHAR_MIN 0
+#endif // UCHAR_MIN
+
+#ifndef CHAR_MIN
+#ifdef __CHAR_UNSIGNED__
+#define CHAR_MIN UCHAR_MIN
+#else
+#define CHAR_MIN SCHAR_MIN
+#endif // __CHAR_UNSIGNED__
+#endif // CHAR_MIN
+
+#ifndef SHRT_MIN
+#define SHRT_MIN (-SHRT_MAX - 1)
+#endif // SHRT_MIN
+
+#ifndef USHRT_MIN
+#define USHRT_MIN 0U
+#endif // USHRT_MIN
+
+#ifndef INT_MIN
+#define INT_MIN (-INT_MAX - 1)
+#endif // INT_MIN
+
+#ifndef UINT_MIN
+#define UINT_MIN 0U
+#endif // UINT_MIN
+
+#ifndef LONG_MIN
+#define LONG_MIN (-LONG_MAX - 1L)
+#endif // LONG_MIN
+
+#ifndef ULONG_MIN
+#define ULONG_MIN 0UL
+#endif // ULONG_MIN
+
+#ifndef LLONG_MIN
+#define LLONG_MIN (-LLONG_MAX - 1LL)
+#endif // LLONG_MIN
+
+#ifndef ULLONG_MIN
+#define ULLONG_MIN 0ULL
+#endif // ULLONG_MIN
+
+#endif // __LLVM_LIBC_MACROS_FLOAT_MACROS_H
diff --git a/libc/spec/stdc.td b/libc/spec/stdc.td
index 99e9b3ca65ca59b..aa394d6c92f0993 100644
--- a/libc/spec/stdc.td
+++ b/libc/spec/stdc.td
@@ -856,6 +856,8 @@ def StdC : StandardSpec<"stdc"> {
]
>;
+ HeaderSpec Limits = HeaderSpec<"limits.h">;
+
NamedType SigAtomicT = NamedType<"sig_atomic_t">;
HeaderSpec Signal = HeaderSpec<
"signal.h",
@@ -1159,6 +1161,7 @@ def StdC : StandardSpec<"stdc"> {
Errno,
Fenv,
Float,
+ Limits,
Math,
String,
StdIO,
|
|
@petrhosek Do you mind checking if this header is correct for arm32 / riscv32 / baremetal? Can I leave it to you to enable this header for those targets later? Thanks, |
|
Ideally we wouldn't provide this header, since it's provided by the compiler. I'm not sure this will fix the problem on the gcc bot since this still includes the system's header, which is what is missing the |
on my system, that file and line 145 use and later has: so it seems what's happening is that we have code (somewhere) that is including Perhaps we need an |
|
src/__support/CPP/limits.h is including limits.h and getting glibc's limits.h, not the compilers'. |
|
recent GCC does not seem to provide a standalone version of |
^ it's right there. |
|
I see. |
| // Supplement missing macros. | ||
|
|
There was a problem hiding this comment.
For which compiler/version/env are these macros missing? (Especially since I'd expect these to be provided by the compiler).
Would be good to comment in the source. I'd like to triple check that these are indeed missing in those environments.
There was a problem hiding this comment.
Besides some old versions of gcc do not have LLONG_MIN, LLONG_MAX, ULLONG_MAX as in glibc's limits.h comments, there are also C23 new macros *_WIDTH that are only added to both clang and gcc recently. The rest are for completeness.
There was a problem hiding this comment.
Besides some old versions of gcc do not have LLONG_MIN, LLONG_MAX, ULLONG_MAX as in glibc's limits.h comments
Ah, but do we have to support any of those versions of GCC? If not, I don't think we should add these.
there are also C23 new macros *_WIDTH that are only added to both clang and gcc recently.
Ok, those are fair to retain with a comment //why//.
The rest are for completeness.
Right, but we don't //need// them; they should come from the compiler's resource headers which we should always expect to be able to rely on. If they don't, then I think it would be better to:
- allow the build to fail loudly because such preprocessor defines are missing.
- add the bare minimum to get the build back to green, with comments/commit messages explaining why.
The thing I worry most about overriding the compiler's resource headers is that we risk getting these wrong, which may lead to hard to diagnose issues, particularly on platforms other than the host machine.
In particular, if any of these are missing, for reasons other than "this is some brand new thing for a new C standard" then I'd prefer not to hide that reason by providing our own definitions of preprocessor macros typically provided by the compiler.
There was a problem hiding this comment.
Right, but we don't //need// them; they should come from the compiler's resource headers which we should always expect to be able to rely on.
Ok, so looking at GCC's resource headers, it seems that GCC only defines LLONG_MIN, LLONG_MAX and ULLONG_MAX if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L which I think is a problem for C++, and our libc/test/src/__support/CPP/limits_test.cpp which references those. So those are the minimum we may want to just define ourselves.
| // The system's limits.h may, in turn, try to #include_next GCC's limits.h when | ||
| // building with GCC. | ||
| // Define the macro _GCC_LIMITS_H_ to stop its chains of #include_next. |
There was a problem hiding this comment.
It's the other way around. It's gcc's limits.h which includes gcc's syslimits.h which reincluded gcc's limits.h which finally include_next's glibc's limits.h which is the problem. I would reword this as:
Prevent GCC from including glibc's limits.h.
For reference the include chain looks like:
In file included:
(GLIBC) from /usr/include/limits.h:26,
(GCC) from /usr/lib/gcc/x86_64-linux-gnu/13/include/limits.h:205,
(GCC) from /usr/lib/gcc/x86_64-linux-gnu/13/include/syslimits.h:7,
(GCC) from /usr/lib/gcc/x86_64-linux-gnu/13/include/limits.h:34,
(LLVMLIBC) from /android0/llvm-project/libc/src/__support/CPP/limits.h:16,
There was a problem hiding this comment.
Bumping this thread. I still think the comment need to be updated (or my understanding corrected).
There was a problem hiding this comment.
Since the current state of those limits.h headers is messy, I just remove the #include_next <limits.h>, only use compiler supplied macros __INT_MAX__ and friends, and filling in the rest of C23 limits.h macros if missing.
|
FWIW, I was able to get the GCC build back to green with just: diff --git a/libc/src/__support/CPP/limits.h b/libc/src/__support/CPP/limits.h
index d9e7090a0b7b..3c5074a15132 100644
--- a/libc/src/__support/CPP/limits.h
+++ b/libc/src/__support/CPP/limits.h
@@ -13,6 +13,9 @@
#include "src/__support/CPP/type_traits/is_signed.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE
+#ifndef __clang__
+#define _LIBC_LIMITS_H_
+#endif
#include <limits.h> // CHAR_BIT
namespace LIBC_NAMESPACE {
diff --git a/libc/src/time/mktime.cpp b/libc/src/time/mktime.cpp
index e57565f0d1fe..83d55e0000ba 100644
--- a/libc/src/time/mktime.cpp
+++ b/libc/src/time/mktime.cpp
@@ -10,6 +10,9 @@
#include "src/__support/common.h"
#include "src/time/time_utils.h"
+#ifndef __clang__
+#define _LIBC_LIMITS_H_
+#endif
#include <limits.h>
namespace LIBC_NAMESPACE {
diff --git a/libc/src/time/time_utils.cpp b/libc/src/time/time_utils.cpp
index 199a74cb168a..9a8a3049374a 100644
--- a/libc/src/time/time_utils.cpp
+++ b/libc/src/time/time_utils.cpp
@@ -9,6 +9,9 @@
#include "src/time/time_utils.h"
#include "src/__support/common.h"
+#ifndef __clang__
+#define _LIBC_LIMITS_H_
+#endif
#include <limits.h>
namespace LIBC_NAMESPACE {I think we could provide our own limits.h (as you do here) and just do that one define if not clang; then require all TUs to include our limits.h, not EDIT: some unit tests need fixes, too. EDIT2: #79211 |
| namespace LIBC_NAMESPACE { | ||
| namespace cpp { | ||
|
|
||
| // Some older gcc distributions don't define these for 32 bit targets. |
There was a problem hiding this comment.
Is there any information on when this was fixed, such as a specific version of GCC?
Especially since this is src/ so we don't need to support versions of GCC older than gcc-11.
I'd be curious how many of the fallbacks you could remove to satisfy the build with GCC; I suspect not all of the definitions are necessary.
There was a problem hiding this comment.
I would defer this question to @michaelrj-google who encountered the issue.
| #define _GCC_LIMITS_H_ | ||
| #endif | ||
|
|
||
| // Include compiler's header |
There was a problem hiding this comment.
Can you confirm that this is the compiler's header?
I guess I would have thought that #include <limits.h> is the compiler's header, so #include_next <limits.h> is...perhaps /usr/include/limits.h which would be from glibc. The -H compiler flag can give you a very verbose output about the chain of includes.
There was a problem hiding this comment.
Drop #include_next <limits.h> to get out of these messy chains of header inclusion.
| // The system's limits.h may, in turn, try to #include_next GCC's limits.h when | ||
| // building with GCC. | ||
| // Define the macro _GCC_LIMITS_H_ to stop its chains of #include_next. |
There was a problem hiding this comment.
Bumping this thread. I still think the comment need to be updated (or my understanding corrected).
| # `*.__copied_hdr__` is created only to copy the header files to the target | ||
| # location, not to be linked against. | ||
| set(link_lib "") | ||
| foreach(dep ${ADD_HEADER_DEPENDS}) | ||
| if (NOT dep MATCHES "__copied_hdr__") | ||
| list(APPEND link_lib ${dep}) | ||
| endif() | ||
| endforeach() |
There was a problem hiding this comment.
Question: is this exposing your newly added limits.h to the consumers of llvm-libc?
There was a problem hiding this comment.
No it's not. It just removes those extra command-only targets from getting linked. We won't add limits.h to the consumers, only the "internal" include/llvm-libc-macros/limit-macros.h.
My GCC build is failing with issues similar why we added our own. Looks like we missed one spot. See also: commit 72ce629 ("[libc] Add C23 limits.h header. (llvm#78887)")
No description provided.