[libc++] Remove explicit mentions of __need_FOO macros #119025
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesThis change has a long history. It was first attempted naively in https://reviews.llvm.org/D131425, which didn't work because we broke the ability for code to include e.g. <stdio.h> multiple times and get different definitions based on the pre-defined macros. However, in #86843 we managed to simplify <stddef.h> by including the underlying system header outside of any include guards, which worked. This patch applies the same simplification we did to <stddef.h> to the other headers that currently mention __need_FOO macros explicitly. Full diff: https://github.com/llvm/llvm-project/pull/119025.diff 4 Files Affected:
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index ed2b7fb1921642..db36ad5bc69592 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -2265,15 +2265,15 @@ module std_stdbool_h [system] {
textual header "stdbool.h"
}
module std_stddef_h [system] {
- // <stddef.h>'s __need_* macros require textual inclusion.
+ // <stddef.h> supports being included multiple times with different pre-defined macros
textual header "stddef.h"
}
module std_stdio_h [system] {
- // <stdio.h>'s __need_* macros require textual inclusion.
+ // <stdio.h> supports being included multiple times with different pre-defined macros
textual header "stdio.h"
}
module std_stdlib_h [system] {
- // <stdlib.h>'s __need_* macros require textual inclusion.
+ // <stdlib.h> supports being included multiple times with different pre-defined macros
textual header "stdlib.h"
}
module std_string_h [system] {
@@ -2289,7 +2289,7 @@ module std_uchar_h [system] {
export *
}
module std_wchar_h [system] {
- // <wchar.h>'s __need_* macros require textual inclusion.
+ // <wchar.h> supports being included multiple times with different pre-defined macros
textual header "wchar.h"
}
module std_wctype_h [system] {
diff --git a/libcxx/include/stdio.h b/libcxx/include/stdio.h
index 3aa559393f1853..2516e9b78342d6 100644
--- a/libcxx/include/stdio.h
+++ b/libcxx/include/stdio.h
@@ -7,17 +7,6 @@
//
//===----------------------------------------------------------------------===//
-#if defined(__need_FILE) || defined(__need___FILE)
-
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
-
-# include_next <stdio.h>
-
-#elif !defined(_LIBCPP_STDIO_H)
-# define _LIBCPP_STDIO_H
-
/*
stdio.h synopsis
@@ -98,11 +87,21 @@ int ferror(FILE* stream);
void perror(const char* s);
*/
-# include <__config>
+#include <__config>
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+// The inclusion of the system's <stdio.h> is intentionally done once outside of any include
+// guards because some code expects to be able to include the underlying system header multiple
+// times to get different definitions based on the macros that are set before inclusion.
+#if __has_include_next(<stdio.h>)
+# include_next <stdio.h>
+#endif
+
+#ifndef _LIBCPP_STDIO_H
+# define _LIBCPP_STDIO_H
# if __has_include_next(<stdio.h>)
# include_next <stdio.h>
diff --git a/libcxx/include/stdlib.h b/libcxx/include/stdlib.h
index 358b10c0392a52..84f8882be41dbb 100644
--- a/libcxx/include/stdlib.h
+++ b/libcxx/include/stdlib.h
@@ -7,17 +7,6 @@
//
//===----------------------------------------------------------------------===//
-#if defined(__need_malloc_and_calloc)
-
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
-
-# include_next <stdlib.h>
-
-#elif !defined(_LIBCPP_STDLIB_H)
-# define _LIBCPP_STDLIB_H
-
/*
stdlib.h synopsis
@@ -84,11 +73,21 @@ void *aligned_alloc(size_t alignment, size_t size); // C11
*/
-# include <__config>
+#include <__config>
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+// The inclusion of the system's <stdlib.h> is intentionally done once outside of any include
+// guards because some code expects to be able to include the underlying system header multiple
+// times to get different definitions based on the macros that are set before inclusion.
+#if __has_include_next(<stdlib.h>)
+# include_next <stdlib.h>
+#endif
+
+#if !defined(_LIBCPP_STDLIB_H)
+# define _LIBCPP_STDLIB_H
# if __has_include_next(<stdlib.h>)
# include_next <stdlib.h>
@@ -146,6 +145,6 @@ inline _LIBCPP_HIDE_FROM_ABI lldiv_t div(long long __x, long long __y) _NOEXCEPT
# endif
# endif // _LIBCPP_MSVCRT
} // extern "C++"
-# endif // __cplusplus
+# endif // __cplusplus
#endif // _LIBCPP_STDLIB_H
diff --git a/libcxx/include/wchar.h b/libcxx/include/wchar.h
index b7cd5d7fc62cde..66d7f8f8de8603 100644
--- a/libcxx/include/wchar.h
+++ b/libcxx/include/wchar.h
@@ -7,17 +7,6 @@
//
//===----------------------------------------------------------------------===//
-#if defined(__need_wint_t) || defined(__need_mbstate_t)
-
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
-
-# include_next <wchar.h>
-
-#elif !defined(_LIBCPP_WCHAR_H)
-# define _LIBCPP_WCHAR_H
-
/*
wchar.h synopsis
@@ -105,23 +94,33 @@ size_t wcsrtombs(char* restrict dst, const wchar_t** restrict src, size_t len,
*/
-# include <__config>
-# include <stddef.h>
+#include <__config>
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
// We define this here to support older versions of glibc <wchar.h> that do
// not define this for clang.
-# ifdef __cplusplus
-# define __CORRECT_ISO_CPP_WCHAR_H_PROTO
-# endif
+#if defined(__cplusplus) && !defined(__CORRECT_ISO_CPP_WCHAR_H_PROTO)
+# define __CORRECT_ISO_CPP_WCHAR_H_PROTO
+#endif
+
+// The inclusion of the system's <wchar.h> is intentionally done once outside of any include
+// guards because some code expects to be able to include the underlying system header multiple
+// times to get different definitions based on the macros that are set before inclusion.
+#if __has_include_next(<wchar.h>)
+# include_next <wchar.h>
+#endif
+
+#ifndef _LIBCPP_WCHAR_H
+# define _LIBCPP_WCHAR_H
+
+# include <__mbstate_t.h> // provide mbstate_t
+# include <stddef.h> // provide size_t
# if __has_include_next(<wchar.h>)
# include_next <wchar.h>
-# else
-# include <__mbstate_t.h> // make sure we have mbstate_t regardless of the existence of <wchar.h>
# endif
// Determine whether we have const-correct overloads for wcschr and friends.
|
ldionne
force-pushed
the
review/remove-need-macros
branch
2 times, most recently
from
135c6ab to
310c3ea
Compare
This change has a long history. It was first attempted naively in https://reviews.llvm.org/D131425, which didn't work because we broke the ability for code to include e.g. <stdio.h> multiple times and get different definitions based on the pre-defined macros. However, in llvm#86843 we managed to simplify <stddef.h> by including the underlying system header outside of any include guards, which worked. This patch applies the same simplification we did to <stddef.h> to the other headers that currently mention __need_FOO macros explicitly.
…his is happening and it looks like a potential compiler bug. XFAILing the AIX test for now until this can be investigated.
ldionne
force-pushed
the
review/remove-need-macros
branch
from
310c3ea to
057a39d
Compare
| // TODO: Fix seemingly circular inclusion or <wchar.h> on AIX | ||
| // UNSUPPORTED: LIBCXX-AIX-FIXME |
There was a problem hiding this comment.
@daltenty This appears to start failing on AIX after this patch, see e.g. https://buildkite.com/llvm-project/libcxx-ci/builds/39304#0193b131-ead6-42af-b231-48d8de48b2a6. I don't understand why it fails though, since we have proper include guards in wchar.h around the functions that are seemingly re-defined. Since I don't have access to an AIX box matching the CI to reproduce, it would be helpful if someone on the AIX side could take a look.
|
We started seeing a different failure on our downstream builders after this change: This is while building libc++ for aarch64 Linux against Debian 8 "jessie" sysroot. |
Same here, while building libcxx-fuzzing for x86-64 linux against Debian jessie sysroot. |
|
This would seem to point to a missing include in the system headers that is being exposed by this patch, but it's really hard to tell without having a local repro. I tried installing Debian Jessie in a Docker image but I can't get anything to work in it, |
If you want to reproduce this locally, you can download our sysroot from https://chrome-infra-packages.appspot.com/p/fuchsia/third_party/sysroot/linux/+/dCkoVcYlgGIP_pmR61ZMIrSDgdrV81BtxcPeTRuyqlUC and use it with We still use Debian 8 "jessie" as our sysroot because it corresponds to Ubuntu 14.04 which is still used by a number of projects (including Android) and that way we can guarantee that binaries produced by our toolchain will run there, but we don't actually do any builds on Debian 8 "jessie" or ubuntu 14.04. |
|
I am trying to run in a ubuntu Docker image with your sysroot and hitting some failures: If you want me to investigate, I'm happy to do that but I'll need a clear reproducer. |
Our sysroot doesn't contain libgcc since we always use compiler-rt and libunwind, so you need |
|
Now I get: Sorry, but I can't play whack-a-mole like that. Can you please provide a working reproducer (e.g. a script that sets up the necessary Docker image + CMake invocation), a pre-commit CI bot that catches it, or perform the analysis of why this is now failing yourself? Otherwise, that isn't really workable -- this patch is correct according to our CI, and that's how we determine what's supported. I don't want to be pedantic and use that criterion to dismiss arbitrary breakage, but we do need to draw a line about where contributor responsibility ends and consumers' responsibility begins, otherwise folks can offload arbitrary troubleshooting to us. |
|
Please try the sysroot from #119025 (comment) |
|
I tested the following instructions locally and confirmed that it reproduces the failure: |
|
Thanks for the steps, I followed exactly that but I still get: |
|
Are you using an Arm machine by any chance? What's the output of |
|
Not sure if it is helpful for understanding the underlying issue. I reproduced the issue locally and used -dD and -E to see the pre-processed file. And noticed a difference in handling the headers Before this patch: system wchar.h ->(include) libcxx/stdio.h ->(include) system stdio.h ->(end of included file) libcxx/stdio.h ->(include) system wchar.h -> define mbstate_t type -> ... -> (include) system _G_config.h -> define __need_mbstate_t -> (include) system wchar.h (but skipped due to header macro) -> used __need_mbstate_t After this patch: system wchar.h ->(include) libcxx/stdio.h ->(include) system stdio.h -> (end of included file) libcxx/stdio.h -> (include) system stdio.h -> (end of included file) libcxx/stdio.h ->(include) system stdio.h -> ... -> system _G_config.h -> define __need_mbstate_t -> system wchar.h (but skipped due to header macro) -> use before define error It appears to me after this patch, during the processing of |
|
Isn't that a circular dependency in the system headers? After the patch, it looks like we have How does including just the system's |
|
The behavior of system @ldionne Would it be possible to revert this change? I plan to continue looking into this issue to see if there's another way to achieve the same result without breaking the compatibility with older sysroots, but I'd like to avoid leaving our downstream builders completely broken for the next two weeks since that will mean will loose test coverage during that time. |
This change also broke Android's ToT integration in the same way. |
This was introduced in llvm#119025 and not only seems unnecessary, it broke the build with older versions of glibc.
This was introduced in #119025 and not only seems unnecessary, it broke the build with older versions of glibc.
|
The breakage should be addressed by #120946, at least on our side the builders are back to green. Let me know if you still see the issue. |
This change has a long history. It was first attempted naively in https://reviews.llvm.org/D131425, which didn't work because we broke the ability for code to include e.g. <stdio.h> multiple times and get different definitions based on the pre-defined macros.
However, in #86843 we managed to simplify <stddef.h> by including the underlying system header outside of any include guards, which worked.
This patch applies the same simplification we did to <stddef.h> to the other headers that currently mention __need_FOO macros explicitly.