[libc][cmake] reset COMPILE_DEFINITIONS #77810
Conversation
|
@llvm/pr-subscribers-libc Author: Nick Desaulniers (nickdesaulniers) ChangesWhile trying to enable -Werror (#74506), the 32b ARM build bot reported an I failed to fix these properly in #77350 (the 32b ARM build is not a fullbuild) It turns out, the preprocessor defines But who was setting these? Turns out that the use of add_compile_definitions
If we desire any of the above, we should manually reset them. Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory. Side note: to debug 'directory properties' in cmake, you first need to use Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS Fixes: #77395 Full diff: https://github.com/llvm/llvm-project/pull/77810.diff 3 Files Affected:
diff --git a/libc/CMakeLists.txt b/libc/CMakeLists.txt
index 9c82db754b5f73..6ff571aa0ae85a 100644
--- a/libc/CMakeLists.txt
+++ b/libc/CMakeLists.txt
@@ -7,10 +7,15 @@ endif()
include(${LLVM_COMMON_CMAKE_UTILS}/Modules/CMakePolicy.cmake
NO_POLICY_SCOPE)
-# `llvm-project/llvm/CMakeLists.txt` adds the following directive
-# `include_directories( ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})`
-# We undo it to be able to precisely control what is getting included.
-set_directory_properties(PROPERTIES INCLUDE_DIRECTORIES "")
+set_directory_properties(PROPERTIES
+ # `llvm-project/llvm/CMakeLists.txt` adds the following directive
+ # `include_directories( ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})` We
+ # undo it to be able to precisely control what is getting included.
+ INCLUDE_DIRECTORIES ""
+ # `llvm/cmake/modules/HandleLLVMOptions.cmake` uses `add_compile_definitions`
+ # to set a few preprocessor defines which we do not want.
+ COMPILE_DEFINITIONS ""
+)
# Default to C++17
set(CMAKE_CXX_STANDARD 17)
diff --git a/libc/src/sys/mman/linux/mmap.cpp b/libc/src/sys/mman/linux/mmap.cpp
index 16111c66859f5e..4cfed24b7b90da 100644
--- a/libc/src/sys/mman/linux/mmap.cpp
+++ b/libc/src/sys/mman/linux/mmap.cpp
@@ -12,6 +12,8 @@
#include "src/__support/common.h"
#include "src/errno/libc_errno.h"
+#include "llvm-libc-macros/sys-mman-macros.h" // For MAP_FAILED.
+
#include <linux/param.h> // For EXEC_PAGESIZE.
#include <sys/syscall.h> // For syscall numbers.
diff --git a/libc/src/sys/mman/mmap.h b/libc/src/sys/mman/mmap.h
index 4425019c4ee1a1..10f30d895b3e35 100644
--- a/libc/src/sys/mman/mmap.h
+++ b/libc/src/sys/mman/mmap.h
@@ -9,7 +9,8 @@
#ifndef LLVM_LIBC_SRC_SYS_MMAN_MMAP_H
#define LLVM_LIBC_SRC_SYS_MMAN_MMAP_H
-#include <sys/mman.h> // For size_t and off_t
+#include "llvm-libc-types/off_t.h"
+#include "llvm-libc-types/size_t.h"
namespace LIBC_NAMESPACE {
|
nickdesaulniers
requested review from
michaelrj-google,
gchatelet,
jhuber6 and
lntue
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
nickdesaulniers
force-pushed
the
cmake_shenanigans
branch
from
e6a7d3b
to
96f1026
Compare
|
oops, meant to remove the changes to mmap.{c|h}. Removed. We only need changes to the cmake. |
While trying to enable -Werror (llvm#74506), the 32b ARM build bot reported an error stemming from -Wshorten-64-to-32 related to usages of `off_t`. I failed to fix these properly in llvm#77350 (the 32b ARM build is not a fullbuild) and llvm#77396. It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and `-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the cmake build system. In particular, these preprocessor defines are feature test macros used by glibc, and which have effects no the corresponding ABI for types like `off_t` (for instance, should `off_t` be 32b or 64b on 32b targets). But who was setting these? Turns out that the use of add_compile_definitions in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and more), which is then inherited by every subdirectory. While some of these defines maybe make sense for host builds, they do not make sense for libraries for the target. The full list of defines being set prior to this commit: - `-D_GNU_SOURCE` - `-D_FILE_OFFSET_BITS=64` - `-D_DEBUG` - `-D_GLIBCXX_ASSERTIONS` - `-D_LARGEFILE_SOURCE` - `-D_FILE_OFFSET_BITS=64` - `-D__STDC_CONSTANT_MACROS` - `-D__STDC_FORMAT_MACROS` - `-D__STDC_LIMIT_MACROS` If we desire any of the above, we should manually reset them. Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory. Side note: to debug 'directory properties' in cmake, you first need to use `get_directory_property` to fetch the corresponding value into a variable first, then that variable can be printed via `message`. Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS Fixes: llvm#77395
nickdesaulniers
force-pushed
the
cmake_shenanigans
branch
from
96f1026
to
32b592a
Compare
|
(after this, I'll revert the two bad patches of mine, then restart the 32b arm buildbots) |
|
I would guess that the debug macro is being set based on the cmake flag |
I just tested that hypothesis by configuring with
So I think it makes sense to:
I'll add 2. |
In that case, since there might be more of these with different configurations and invocation, can you printout |
In that case, I suspect YAGNI, but adding logging and re-added _DEBUG in fdc1e35, PTAL. |
| # to set a few preprocessor defines which we do not want. | ||
| COMPILE_DEFINITIONS "" | ||
| ) | ||
| if (CMAKE_BUILD_TYPE STREQUAL "Debug") |
There was a problem hiding this comment.
Do you mind checking what else will be set for the other two official CMake build types: RelWithDebInfo and MinSizeRel?
There was a problem hiding this comment.
RelWithDebInfo (BEFORE): _GNU_SOURCE_FILE_OFFSET_BITS=64_LARGEFILE_SOURCE_FILE_OFFSET_BITS=64__STDC_CONSTANT_MACROS__STDC_FORMAT_MACROS__STDC_LIMIT_MACROS
(Nothing new)
MinSizeRel: (BEFORE):
_GNU_SOURCE_FILE_OFFSET_BITS=64_LARGEFILE_SOURCE_FILE_OFFSET_BITS=64__STDC_CONSTANT_MACROS__STDC_FORMAT_MACROS__STDC_LIMIT_MACROS
(Nothing new)
|
I think this makes sense, however I'll note that other things can be polluted by the "rest" of LLVM as well:
This is better than nothing, but I think the full fix is to gradually refactor the CMake files used by runtimes to avoid setting anything global. |
💯 |
This reverts commit 70cea91. The correct fix was commit f1f1875 ("[libc][cmake] reset COMPILE_DEFINITIONS (llvm#77810)") Link: llvm#77395
This reverts commit ce1305a. The correct fix was commit f1f1875 ("[libc][cmake] reset COMPILE_DEFINITIONS (llvm#77810)") Link: llvm#77395
|
I've filed #78479 as a follow up to the suggestions here and from Discord. |
While trying to enable -Werror (llvm#74506), the 32b ARM build bot reported an error stemming from -Wshorten-64-to-32 related to usages of `off_t`. I failed to fix these properly in llvm#77350 (the 32b ARM build is not a fullbuild) and llvm#77396. It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and `-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the cmake build system. In particular, these preprocessor defines are feature test macros used by glibc, and which have effects no the corresponding ABI for types like `off_t` (for instance, should `off_t` be 32b or 64b on 32b targets). But who was setting these? Turns out that the use of add_compile_definitions in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and more), which is then inherited by every subdirectory. While some of these defines maybe make sense for host builds, they do not make sense for libraries for the target. The full list of defines being set prior to this commit: - `-D_GNU_SOURCE` - `-D_FILE_OFFSET_BITS=64` - `-D_DEBUG` - `-D_GLIBCXX_ASSERTIONS` - `-D_LARGEFILE_SOURCE` - `-D_FILE_OFFSET_BITS=64` - `-D__STDC_CONSTANT_MACROS` - `-D__STDC_FORMAT_MACROS` - `-D__STDC_LIMIT_MACROS` If we desire any of the above, we should manually reset them. Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory. Side note: to debug 'directory properties' in cmake, you first need to use `get_directory_property` to fetch the corresponding value into a variable first, then that variable can be printed via `message`. Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS Fixes: llvm#77395
While trying to enable -Werror (#74506), the 32b ARM build bot reported an
error stemming from -Wshorten-64-to-32 related to usages of
off_t.I failed to fix these properly in #77350 (the 32b ARM build is not a fullbuild)
and #77396.
It turns out, the preprocessor defines
-D_LARGEFILE_SOURCEand-D_FILE_OFFSET_BITS=64were being set for llvmlibc when using the cmake buildsystem. In particular, these preprocessor defines are feature test macros used
by glibc, and which have effects no the corresponding ABI for types like
off_t(for instance, shouldoff_tbe 32b or 64b on 32b targets).But who was setting these? Turns out that the use of add_compile_definitions
in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and more),
which is then inherited by every subdirectory. While some of these defines
maybe make sense for host builds, they do not make sense for libraries for the
target. The full list of defines being set prior to this commit:
-D_GNU_SOURCE-D_FILE_OFFSET_BITS=64-D_DEBUG-D_GLIBCXX_ASSERTIONS-D_LARGEFILE_SOURCE-D_FILE_OFFSET_BITS=64-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROSIf we desire any of the above, we should manually reset them.
Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory.
Side note: to debug 'directory properties' in cmake, you first need to use
get_directory_propertyto fetch the corresponding value into a variablefirst, then that variable can be printed via
message.Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Fixes: #77395