Revert "[libc++] Allow the use of extensions in the implementation (#79532)" #85792
Conversation
…lvm#79532)" This reverts commit 4d323e4. The heavy use of push/pop directives causes a severe compile-time regression (>50%) when building code that uses libc++ via modules. See discussion on llvm#79532
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Sam McCall (sam-mccall) ChangesThis reverts commit 4d323e4. The heavy use of push/pop directives causes a severe compile-time See discussion on #79532 Full diff: https://github.com/llvm/llvm-project/pull/85792.diff 2 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 11e13e0c24986a..3a438e85a7b819 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -406,10 +406,6 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define __has_include(...) 0
# endif
-# ifndef __has_warning
-# define __has_warning(...) 0
-# endif
-
# if !defined(_LIBCPP_COMPILER_CLANG_BASED) && __cplusplus < 201103L
# error "libc++ only supports C++03 with Clang-based compilers. Please enable C++11"
# endif
@@ -727,23 +723,6 @@ typedef __char32_t char32_t;
# define _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION _LIBCPP_ALWAYS_INLINE
# endif
-# ifdef _LIBCPP_COMPILER_CLANG_BASED
-# define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("clang diagnostic push")
-# define _LIBCPP_DIAGNOSTIC_POP _Pragma("clang diagnostic pop")
-# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str) _Pragma(_LIBCPP_TOSTRING(clang diagnostic ignored str))
-# define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str)
-# elif defined(_LIBCPP_COMPILER_GCC)
-# define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("GCC diagnostic push")
-# define _LIBCPP_DIAGNOSTIC_POP _Pragma("GCC diagnostic pop")
-# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str)
-# define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str) _Pragma(_LIBCPP_TOSTRING(GCC diagnostic ignored str))
-# else
-# define _LIBCPP_DIAGNOSTIC_PUSH
-# define _LIBCPP_DIAGNOSTIC_POP
-# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str)
-# define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str)
-# endif
-
# if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_FAST
# define _LIBCPP_HARDENING_SIG f
# elif _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_EXTENSIVE
@@ -831,33 +810,16 @@ typedef __char32_t char32_t;
# define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
# endif
-// TODO: Remove this workaround once we drop support for Clang 16
-#if __has_warning("-Wc++23-extensions")
-# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED_CXX23_EXTENSION _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++23-extensions")
-#else
-# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED_CXX23_EXTENSION _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++2b-extensions")
-#endif
-
// Inline namespaces are available in Clang/GCC/MSVC regardless of C++ dialect.
// clang-format off
-# define _LIBCPP_BEGIN_NAMESPACE_STD _LIBCPP_DIAGNOSTIC_PUSH \
- _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++11-extensions") \
- _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++14-extensions") \
- _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++17-extensions") \
- _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++20-extensions") \
- _LIBCPP_CLANG_DIAGNOSTIC_IGNORED_CXX23_EXTENSION \
- _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wc++14-extensions") \
- _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wc++17-extensions") \
- _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wc++20-extensions") \
- _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wc++23-extensions") \
- namespace _LIBCPP_TYPE_VISIBILITY_DEFAULT std { \
+# define _LIBCPP_BEGIN_NAMESPACE_STD namespace _LIBCPP_TYPE_VISIBILITY_DEFAULT std { \
inline namespace _LIBCPP_ABI_NAMESPACE {
-# define _LIBCPP_END_NAMESPACE_STD }} _LIBCPP_DIAGNOSTIC_POP
+# define _LIBCPP_END_NAMESPACE_STD }}
# define _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM _LIBCPP_BEGIN_NAMESPACE_STD \
inline namespace __fs { namespace filesystem {
-# define _LIBCPP_END_NAMESPACE_FILESYSTEM }} _LIBCPP_END_NAMESPACE_STD
+# define _LIBCPP_END_NAMESPACE_FILESYSTEM _LIBCPP_END_NAMESPACE_STD }}
// clang-format on
# if __has_attribute(__enable_if__)
@@ -1294,6 +1256,23 @@ __sanitizer_verify_double_ended_contiguous_container(const void*, const void*, c
// the ABI inconsistent.
# endif
+# ifdef _LIBCPP_COMPILER_CLANG_BASED
+# define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("clang diagnostic push")
+# define _LIBCPP_DIAGNOSTIC_POP _Pragma("clang diagnostic pop")
+# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str) _Pragma(_LIBCPP_TOSTRING(clang diagnostic ignored str))
+# define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str)
+# elif defined(_LIBCPP_COMPILER_GCC)
+# define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("GCC diagnostic push")
+# define _LIBCPP_DIAGNOSTIC_POP _Pragma("GCC diagnostic pop")
+# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str)
+# define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str) _Pragma(_LIBCPP_TOSTRING(GCC diagnostic ignored str))
+# else
+# define _LIBCPP_DIAGNOSTIC_PUSH
+# define _LIBCPP_DIAGNOSTIC_POP
+# define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str)
+# define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str)
+# endif
+
// c8rtomb() and mbrtoc8() were added in C++20 and C23. Support for these
// functions is gradually being added to existing C libraries. The conditions
// below check for known C library versions and conditions under which these
diff --git a/libcxx/include/tuple b/libcxx/include/tuple
index f78db061b844b0..8e38dfe4d0eaa8 100644
--- a/libcxx/include/tuple
+++ b/libcxx/include/tuple
@@ -540,6 +540,10 @@ class _LIBCPP_TEMPLATE_VIS tuple {
public:
// [tuple.cnstr]
+ _LIBCPP_DIAGNOSTIC_PUSH
+ _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++20-extensions")
+ _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wc++20-extensions")
+
// tuple() constructors (including allocator_arg_t variants)
template <template <class...> class _IsImpDefault = __is_implicitly_default_constructible,
template <class...> class _IsDefault = is_default_constructible,
@@ -821,6 +825,8 @@ public:
: __base_(allocator_arg_t(), __alloc, std::move(__p)) {}
# endif // _LIBCPP_STD_VER >= 23
+ _LIBCPP_DIAGNOSTIC_POP
+
// [tuple.assign]
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 tuple&
operator=(_If<_And<is_copy_assignable<_Tp>...>::value, tuple, __nat> const& __tuple)
|
|
#79532 has details on the regression. It seems there are other ways to allow extensions broadly without using push/pop everywhere. But there hasn't been any movement there so far, and this is a large enough compile performance regression that we can't ship libc++. Contra #79532 (comment), a clean revert seems to pass tests, so I don't think this is heavily used yet. |
|
Fixed a failure that showed up presubmit: "pair" is using explicit(bool) and needs a suppression similar to the one in tuple. So this is no longer a pure revert. (not sure why I didn't see this locally). |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I would support the revert until we understand the issue better and how to fix it. 50% regression is... very serious. Especially given all the efforts we do to reduce compile times by a few percentage points. I'll let Nikolas have a look but that's my opinion. |
|
I see other solutions being mentioned in #79532 though, should we be exploring those instead? |
|
When building modules, it shouldn't be needed to suppress warnings on C++14/17/20 features, right? I'd expect that C++23 features are rarely used as extensions (or not yet used), so we might be able to suppress the warnings more locally. |
|
Thanks!
It seems like there are several options with tradeoffs, and I don't feel informed enough to push for one of them. If someone closer to the project can make the call then absolutely. If there's still some discussion to be had, I'd be grateful if we could revert back to a good state first.
Mostly. (You can use Clang modules without C++20, but I think the relatively-few who did that are on C++20 now anyway). You could reduce the suppression pragmas this way (could gate on language version rather than modules-enabled?). But it also feels like a short-term fix to what may simply be the wrong mechanism. |
|
I would also support a revert until we can found out what caused the regression. @sam-mccall what modules are you talking about Clang modules or C++20 modules? |
|
#85917 is an (IMO simpler) alternative to work around this problem. |
Our deployment is Clang modules (which would also count as C++20 header units IIUC). My reading of the code is that diagnostic mappings take the same codepath in both cases. I don't have a large-scale C++20 modules codebase with libc++ set up as named modules to test on.
I'm happy if that lands instead, that will also fix our compile time :-) It is important that it should fire when clang modules are on and c++20 modules are off (which (Personally I find this solution a bit iffy: we've added a bunch of load onto a compiler subsystem, and immediately found circumstances where that causes problems - it seems more robust to remove the load than to patch around those circumstances. But that's a concern for owners - this will solve my immediate problem) |
Thanks! I was just confused whether this is related to Clang or C++ modules. |
At the same time, we do want the ability to use extensions in the code base, which this revert prevents. |
|
Closing this, since #85917 has landed. |
This reverts commit 4d323e4.
The heavy use of push/pop directives causes a severe compile-time
regression (>50%) when building code that uses libc++ via modules.
See discussion on #79532