[libc] force GCC limits.h to not include_next limits.h #79211
Conversation
Who defines what the width and thus the min/max values for a given integer type? We'd like to rely on the compiler's resource dir's definitions, but GCC's limits.h can end up using include_next to include limits.h from GLIBC. This results in build failures with GCC due to __GLIBC_USE being undefined (a feature detection macro defined only relevant to glibc). We'd like to avoid the compiler pulling in the libc's headers (unless there's some critical reason why, which we don't have today AFAIK). GCC's limits.h will avoid this circular includes if _LIBC_LIMITS_H_ is defined. GCC's limits.h only defines LLONG_MAX, LLONG_MIN, and ULLONG_MAX if __STDC_VERSION__ is defined and >= 199901L, i.e. not for C++, so provide those definitions ourselves. Due to this, we should never be including <limits.h> in libc/ (other than in CPP/limits.h). Sources and tests should be including "libc/src/__support/CPP/limits.h" in place of <limits.h>.
|
@llvm/pr-subscribers-libc Author: Nick Desaulniers (nickdesaulniers) ChangesWho defines what the width and thus the min/max values for a given integer This results in build failures with GCC due to __GLIBC_USE being undefined (a GCC's limits.h only defines LLONG_MAX, LLONG_MIN, and ULLONG_MAX if Due to this, we should never be including <limits.h> in libc/ (other than in Full diff: https://github.com/llvm/llvm-project/pull/79211.diff 23 Files Affected:
diff --git a/libc/src/__support/CPP/limits.h b/libc/src/__support/CPP/limits.h
index d9e7090a0b7b190..11af4b30eaf745e 100644
--- a/libc/src/__support/CPP/limits.h
+++ b/libc/src/__support/CPP/limits.h
@@ -13,8 +13,25 @@
#include "src/__support/CPP/type_traits/is_signed.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE
+// Coax GCC to NOT attempt to include_next limits.h from glibc. llvmlibc should
+// include this file rather than <limits.h> directy for this reason.
+#ifndef __clang__
+#define _LIBC_LIMITS_H_
+#endif
#include <limits.h> // CHAR_BIT
+// GCC's limits.h only defines these for __STDC_VERSION__ >= 199901L, i.e. not
+// C++.
+#ifndef LLONG_MAX
+#define LLONG_MAX __LONG_LONG_MAX__
+#endif
+#ifndef LLONG_MIN
+#define LLONG_MIN (-LONG_MAX - 1LL)
+#endif
+#ifndef ULLONG_MAX
+#define ULLONG_MAX (~0ULL)
+#endif
+
namespace LIBC_NAMESPACE {
namespace cpp {
diff --git a/libc/src/__support/FPUtil/ManipulationFunctions.h b/libc/src/__support/FPUtil/ManipulationFunctions.h
index 81c8281f3c7bbee..589cd9c4cfb7af7 100644
--- a/libc/src/__support/FPUtil/ManipulationFunctions.h
+++ b/libc/src/__support/FPUtil/ManipulationFunctions.h
@@ -14,12 +14,12 @@
#include "NormalFloat.h"
#include "src/__support/CPP/bit.h"
+#include "src/__support/CPP/limits.h" // INT_MAX, INT_MIN
#include "src/__support/CPP/type_traits.h"
#include "src/__support/FPUtil/FEnvImpl.h"
#include "src/__support/macros/attributes.h"
#include "src/__support/macros/optimization.h" // LIBC_UNLIKELY
-#include <limits.h>
#include <math.h>
namespace LIBC_NAMESPACE {
diff --git a/libc/src/__support/math_extras.h b/libc/src/__support/math_extras.h
index 89bd0b72669ea22..8ec30396ffdb460 100644
--- a/libc/src/__support/math_extras.h
+++ b/libc/src/__support/math_extras.h
@@ -10,12 +10,11 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_MATH_EXTRAS_H
#define LLVM_LIBC_SRC___SUPPORT_MATH_EXTRAS_H
+#include "src/__support/CPP/limits.h" // CHAR_BIT
#include "src/__support/CPP/type_traits.h" // is_unsigned_v
#include "src/__support/macros/attributes.h" // LIBC_INLINE
#include "src/__support/macros/config.h" // LIBC_HAS_BUILTIN
-#include <limits.h> // CHAR_BIT
-
namespace LIBC_NAMESPACE {
// Create a bitmask with the count right-most bits set to 1, and all other bits
diff --git a/libc/src/__support/str_to_integer.h b/libc/src/__support/str_to_integer.h
index 4ee336ec9e2bbd8..e83a508e086b18a 100644
--- a/libc/src/__support/str_to_integer.h
+++ b/libc/src/__support/str_to_integer.h
@@ -15,7 +15,6 @@
#include "src/__support/ctype_utils.h"
#include "src/__support/str_to_num_result.h"
#include "src/errno/libc_errno.h" // For ERANGE
-#include <limits.h>
namespace LIBC_NAMESPACE {
namespace internal {
diff --git a/libc/src/__support/threads/linux/callonce.cpp b/libc/src/__support/threads/linux/callonce.cpp
index de1e1008784f604..ec66d41c559a0be 100644
--- a/libc/src/__support/threads/linux/callonce.cpp
+++ b/libc/src/__support/threads/linux/callonce.cpp
@@ -9,10 +9,10 @@
#include "futex_word.h"
#include "src/__support/CPP/atomic.h"
+#include "src/__support/CPP/limits.h"
#include "src/__support/OSUtil/syscall.h" // For syscall functions.
#include "src/__support/threads/callonce.h"
-#include <limits.h>
#include <linux/futex.h>
#include <sys/syscall.h> // For syscall numbers.
diff --git a/libc/src/threads/linux/call_once.cpp b/libc/src/threads/linux/call_once.cpp
index 5cdd8ebfd190ea1..dc36304282355ae 100644
--- a/libc/src/threads/linux/call_once.cpp
+++ b/libc/src/threads/linux/call_once.cpp
@@ -9,12 +9,12 @@
#include "Futex.h"
#include "src/__support/CPP/atomic.h"
+#include "src/__support/CPP/limits.h"
#include "src/__support/OSUtil/syscall.h" // For syscall functions.
#include "src/__support/common.h"
#include "src/threads/call_once.h"
#include "src/threads/linux/Futex.h"
-#include <limits.h>
#include <linux/futex.h>
#include <sys/syscall.h> // For syscall numbers.
#include <threads.h> // For call_once related type definition.
diff --git a/libc/src/time/mktime.cpp b/libc/src/time/mktime.cpp
index e57565f0d1fe016..12a1e690b0f23cc 100644
--- a/libc/src/time/mktime.cpp
+++ b/libc/src/time/mktime.cpp
@@ -7,11 +7,10 @@
//===----------------------------------------------------------------------===//
#include "src/time/mktime.h"
+#include "src/__support/CPP/limits.h"
#include "src/__support/common.h"
#include "src/time/time_utils.h"
-#include <limits.h>
-
namespace LIBC_NAMESPACE {
using LIBC_NAMESPACE::time_utils::TimeConstants;
diff --git a/libc/src/time/time_utils.cpp b/libc/src/time/time_utils.cpp
index 199a74cb168a2c3..5fd988661383e2a 100644
--- a/libc/src/time/time_utils.cpp
+++ b/libc/src/time/time_utils.cpp
@@ -7,10 +7,9 @@
//===----------------------------------------------------------------------===//
#include "src/time/time_utils.h"
+#include "src/__support/CPP/limits.h"
#include "src/__support/common.h"
-#include <limits.h>
-
namespace LIBC_NAMESPACE {
namespace time_utils {
diff --git a/libc/test/src/math/ILogbTest.h b/libc/test/src/math/ILogbTest.h
index 9fa25c9ff986148..d2e8e40a3add283 100644
--- a/libc/test/src/math/ILogbTest.h
+++ b/libc/test/src/math/ILogbTest.h
@@ -9,13 +9,12 @@
#ifndef LLVM_LIBC_TEST_SRC_MATH_ILOGBTEST_H
#define LLVM_LIBC_TEST_SRC_MATH_ILOGBTEST_H
+#include "src/__support/CPP/limits.h"
#include "src/__support/FPUtil/FPBits.h"
#include "src/__support/FPUtil/ManipulationFunctions.h"
#include "test/UnitTest/Test.h"
#include <math.h>
-#include <limits.h>
-
class LlvmLibcILogbTest : public LIBC_NAMESPACE::testing::Test {
public:
template <typename T> struct ILogbFunc {
diff --git a/libc/test/src/math/LdExpTest.h b/libc/test/src/math/LdExpTest.h
index 25120ba3646fda0..0b9ddf13a2a8af8 100644
--- a/libc/test/src/math/LdExpTest.h
+++ b/libc/test/src/math/LdExpTest.h
@@ -9,12 +9,12 @@
#ifndef LLVM_LIBC_TEST_SRC_MATH_LDEXPTEST_H
#define LLVM_LIBC_TEST_SRC_MATH_LDEXPTEST_H
+#include "src/__support/CPP/limits.h"
#include "src/__support/FPUtil/FPBits.h"
#include "src/__support/FPUtil/NormalFloat.h"
#include "test/UnitTest/FPMatcher.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <math.h>
#include <stdint.h>
diff --git a/libc/test/src/math/smoke/ILogbTest.h b/libc/test/src/math/smoke/ILogbTest.h
index 0a50abc04f727f1..9f415f1c664fada 100644
--- a/libc/test/src/math/smoke/ILogbTest.h
+++ b/libc/test/src/math/smoke/ILogbTest.h
@@ -9,13 +9,12 @@
#ifndef LLVM_LIBC_TEST_SRC_MATH_ILOGBTEST_H
#define LLVM_LIBC_TEST_SRC_MATH_ILOGBTEST_H
+#include "src/__support/CPP/limits.h" // INT_MAX
#include "src/__support/FPUtil/FPBits.h"
#include "src/__support/FPUtil/ManipulationFunctions.h"
#include "test/UnitTest/Test.h"
#include <math.h>
-#include <limits.h>
-
class LlvmLibcILogbTest : public LIBC_NAMESPACE::testing::Test {
public:
template <typename T> struct ILogbFunc {
diff --git a/libc/test/src/math/smoke/LdExpTest.h b/libc/test/src/math/smoke/LdExpTest.h
index 25120ba3646fda0..da69b85dced163c 100644
--- a/libc/test/src/math/smoke/LdExpTest.h
+++ b/libc/test/src/math/smoke/LdExpTest.h
@@ -9,12 +9,12 @@
#ifndef LLVM_LIBC_TEST_SRC_MATH_LDEXPTEST_H
#define LLVM_LIBC_TEST_SRC_MATH_LDEXPTEST_H
+#include "src/__support/CPP/limits.h" // INT_MAX
#include "src/__support/FPUtil/FPBits.h"
#include "src/__support/FPUtil/NormalFloat.h"
#include "test/UnitTest/FPMatcher.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <math.h>
#include <stdint.h>
diff --git a/libc/test/src/stdlib/AtoiTest.h b/libc/test/src/stdlib/AtoiTest.h
index 85701828c0f6b7c..add510f468f55a1 100644
--- a/libc/test/src/stdlib/AtoiTest.h
+++ b/libc/test/src/stdlib/AtoiTest.h
@@ -6,11 +6,10 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/__support/CPP/type_traits.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
-
using LIBC_NAMESPACE::cpp::is_same_v;
template <typename ReturnT>
diff --git a/libc/test/src/stdlib/StrtolTest.h b/libc/test/src/stdlib/StrtolTest.h
index 11794c4bfe05f8a..6aee049aa066a04 100644
--- a/libc/test/src/stdlib/StrtolTest.h
+++ b/libc/test/src/stdlib/StrtolTest.h
@@ -12,7 +12,6 @@
#include "src/errno/libc_errno.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <stddef.h>
using LIBC_NAMESPACE::cpp::is_signed_v;
diff --git a/libc/test/src/stdlib/atof_test.cpp b/libc/test/src/stdlib/atof_test.cpp
index ed3d4c26308cb23..65172b90072bdad 100644
--- a/libc/test/src/stdlib/atof_test.cpp
+++ b/libc/test/src/stdlib/atof_test.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/__support/FPUtil/FPBits.h"
#include "src/errno/libc_errno.h"
#include "src/stdlib/atof.h"
@@ -13,7 +14,6 @@
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <stddef.h>
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
diff --git a/libc/test/src/stdlib/strtod_test.cpp b/libc/test/src/stdlib/strtod_test.cpp
index b1bdd89e41fd151..70ab65df61899e2 100644
--- a/libc/test/src/stdlib/strtod_test.cpp
+++ b/libc/test/src/stdlib/strtod_test.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/__support/FPUtil/FPBits.h"
#include "src/errno/libc_errno.h"
#include "src/stdlib/strtod.h"
@@ -14,7 +15,6 @@
#include "test/UnitTest/RoundingModeUtils.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <stddef.h>
using LIBC_NAMESPACE::fputil::testing::ForceRoundingModeTest;
diff --git a/libc/test/src/stdlib/strtof_test.cpp b/libc/test/src/stdlib/strtof_test.cpp
index 15a8a34ef4fb171..b25a9068bbfe52e 100644
--- a/libc/test/src/stdlib/strtof_test.cpp
+++ b/libc/test/src/stdlib/strtof_test.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/__support/FPUtil/FPBits.h"
#include "src/errno/libc_errno.h"
#include "src/stdlib/strtof.h"
@@ -14,7 +15,6 @@
#include "test/UnitTest/RoundingModeUtils.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <stddef.h>
using LIBC_NAMESPACE::fputil::testing::ForceRoundingModeTest;
diff --git a/libc/test/src/stdlib/strtold_test.cpp b/libc/test/src/stdlib/strtold_test.cpp
index 51f9975772aa5e9..832d11ea1f3daf1 100644
--- a/libc/test/src/stdlib/strtold_test.cpp
+++ b/libc/test/src/stdlib/strtold_test.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/__support/FPUtil/FPBits.h"
#include "src/__support/UInt128.h"
#include "src/errno/libc_errno.h"
@@ -13,7 +14,6 @@
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <stddef.h>
#if defined(LIBC_LONG_DOUBLE_IS_FLOAT64)
diff --git a/libc/test/src/time/CMakeLists.txt b/libc/test/src/time/CMakeLists.txt
index 10b63ce6f39d295..ec274863f2feca9 100644
--- a/libc/test/src/time/CMakeLists.txt
+++ b/libc/test/src/time/CMakeLists.txt
@@ -71,6 +71,7 @@ add_libc_unittest(
TmMatcher.h
DEPENDS
libc.src.time.gmtime
+ libc.src.__support.CPP.limits
)
add_libc_unittest(
@@ -98,6 +99,7 @@ add_libc_unittest(
20
DEPENDS
libc.src.time.mktime
+ libc.src.__support.CPP.limits
)
# Sleeping is not supported on older NVPTX architectures.
@@ -136,6 +138,7 @@ add_libc_test(
clock_test.cpp
DEPENDS
libc.include.time
- libc.src.time.clock
+ libc.src.__support.CPP.limits
libc.src.errno.errno
+ libc.src.time.clock
)
diff --git a/libc/test/src/time/clock_test.cpp b/libc/test/src/time/clock_test.cpp
index a3dffc6bae3913d..99a2233da13803a 100644
--- a/libc/test/src/time/clock_test.cpp
+++ b/libc/test/src/time/clock_test.cpp
@@ -6,10 +6,10 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/time/clock.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <time.h>
TEST(LlvmLibcClockTest, SmokeTest) {
diff --git a/libc/test/src/time/gmtime_test.cpp b/libc/test/src/time/gmtime_test.cpp
index 6b1d029a693c383..c42588947710f89 100644
--- a/libc/test/src/time/gmtime_test.cpp
+++ b/libc/test/src/time/gmtime_test.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/errno/libc_errno.h"
#include "src/time/gmtime.h"
#include "src/time/time_utils.h"
@@ -13,8 +14,6 @@
#include "test/UnitTest/Test.h"
#include "test/src/time/TmMatcher.h"
-#include <limits.h>
-
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
using LIBC_NAMESPACE::time_utils::TimeConstants;
diff --git a/libc/test/src/time/mktime_test.cpp b/libc/test/src/time/mktime_test.cpp
index 6f179150953b7f1..8dc65e2406f6347 100644
--- a/libc/test/src/time/mktime_test.cpp
+++ b/libc/test/src/time/mktime_test.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/CPP/limits.h"
#include "src/time/mktime.h"
#include "src/time/time_utils.h"
#include "test/UnitTest/ErrnoSetterMatcher.h"
@@ -13,8 +14,6 @@
#include "test/src/time/TmHelper.h"
#include "test/src/time/TmMatcher.h"
-#include <limits.h>
-
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
using LIBC_NAMESPACE::time_utils::Month;
diff --git a/libc/test/src/time/time_test.cpp b/libc/test/src/time/time_test.cpp
index 1d938e8dae7adb2..d3d4dc9a285158e 100644
--- a/libc/test/src/time/time_test.cpp
+++ b/libc/test/src/time/time_test.cpp
@@ -9,7 +9,6 @@
#include "src/time/time_func.h"
#include "test/UnitTest/Test.h"
-#include <limits.h>
#include <time.h>
TEST(LlvmLibcTimeTest, SmokeTest) {
|
libc/test/src/time/CMakeLists.txt
Outdated
| @@ -136,6 +138,7 @@ add_libc_test( | |||
| clock_test.cpp | |||
| DEPENDS | |||
| libc.include.time | |||
| libc.src.time.clock | |||
| libc.src.__support.CPP.limits | |||
There was a problem hiding this comment.
it's unclear to me when we need to add these DEPENDS vs not. I didn't for many changes, and the build succeeds. Is that expected? Do we have some convention here that perhaps I'm unaware of?
There was a problem hiding this comment.
DEPENDS primarily tells cmake which object files need to be linked. For header files it's technically unnecessary, though we should still do it to make sure that cmake properly rebuilds targets that depend on those header files.
There was a problem hiding this comment.
If I change this line in particular s/limits/asdfasdfasdf/ literally nothing happens to the build...that doesn't instill confidence that this change is even doing anything...
There was a problem hiding this comment.
After an "unknown" change sometime ago, if you miss the generated header dependency in full build, the header may not be generated in time. This results in most of the full build failures that we have been fixing.
There was a problem hiding this comment.
The reason the build doesn't fail when you add an invalid dependency is because the cmake checks the dependencies when it's generating the targets. If you turn on verbose logging you should see something like -- Skipping unittest libc.test.src.time.clock_test.__unit__ as it has missing deps: libc.src.asdfasdfasdf. when you rerun the cmake, and also if you specifically run that test with ninja libc.test.src.time.clock_test.__unit__ it will say it can't find the target.
There was a problem hiding this comment.
Is that a feature or a bug?
There was a problem hiding this comment.
It's a feature, so that we skip the tests if its entrypoints are not enabled instead of erroring out, or something like that.
|
Seems weird to make |
Where is |
I'm unsure how we expect all of these to work. The compiler provides resource directories, which then include the system's versions unless STDC_HOSTED isn't set. We wrap around these to augment tyhem for some things. The idea was that we can't control GCC, so we'd do it in a header. But if GCC is excluded we can just update clang, since those are the only supported compilers right now. |
Perhaps you were referring to: llvm-project/clang/lib/Headers/limits.h Line 21 in a16f81f Which indeed does pull in It seems that we also set llvm-project/libc/cmake/modules/LLVMLibCObjectRules.cmake Lines 33 to 35 in a16f81f
That's another approach we could take, which #78887 very nearly does, except:
I think I'd be ok with that approach if 1 and 2 were addressed as well in the same PR. (or a good reason for NOT doing either was provided) Personally, I have a weak preference for "relying on the compiler (and it's resource dir)" a la this PR. But I could be convinced to not use ANY external dependency and just define these ourselves. FWIW, clang's preprocessor also generates some of these defines. Example: llvm-project/clang/lib/Frontend/InitPreprocessor.cpp Lines 1014 to 1071 in a16f81f (besides the header file I linked to above) |
I could definitely include 2 to #78887. On the other hand, the main reason I added
Clang and GCC both provide various of those definitions, but which ones they provide do vary depending on their versions and the platforms. So I think we can use C23-compliance as a reason and a selling point to just provide a complete There is only 1 constant that confuses me is |
More specifically, I worry about If you remove As for other extra macros; I'd kind of like to know what they are (if any). I think it's better to exclude them for now, then add them when we observed an issue specific to not including them. Then we can have well formed justification for why we include them, rather than pull in headers based on the hope that they contain something that might be helpful (but who knows what else).
Does |
|
Prefer #78887 |
Who defines what the width and thus the min/max values for a given integer
type? We'd like to rely on the compiler's resource dir's definitions, but GCC's
limits.h can end up using include_next to include limits.h from GLIBC.
This results in build failures with GCC due to __GLIBC_USE being undefined (a
feature detection macro defined only relevant to glibc). We'd like to avoid
the compiler pulling in the libc's headers (unless there's some critical reason
why, which we don't have today AFAIK). GCC's limits.h will avoid this circular
includes if LIBC_LIMITS_H is defined.
GCC's limits.h only defines LLONG_MAX, LLONG_MIN, and ULLONG_MAX if
STDC_VERSION is defined and >= 199901L, i.e. not for C++, so provide those
definitions ourselves.
Due to this, we should never be including <limits.h> in libc/ (other than in
CPP/limits.h). Sources and tests should be including
"libc/src/__support/CPP/limits.h" in place of <limits.h>.