[libc++] Allow the use of extensions in the implementation #79532

philnik777 · 2024-01-26T00:50:01Z

We've talked about allowing extensions on discourse and in a libc++ monthly meeting and agreed to test it out in the LLVM 18 release. We've done that with the tuple constructor overload set (using conditional explicit). Since we haven't heard about any breakages, it seems safe to do. This patch enables the use of extension from later C++ standards inside the versioned std namespaces. This should be good enough, since almost all of our code is inside that namespace. This approach also avoids the use of extensions inside the test std suite. That part of the code base should stay clean, since it's a test suite that is also used by other vendors to test their implementations.

llvmbot · 2024-01-26T00:50:35Z

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/79532.diff

2 Files Affected:

(modified) libcxx/include/__config (+32-20)
(modified) libcxx/include/tuple (-6)

diff --git a/libcxx/include/__config b/libcxx/include/__config
index 9fc608ee14320dc..aece8438e94207f 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -787,6 +787,23 @@ 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
@@ -881,14 +898,26 @@ typedef __char32_t char32_t;
 
 // Inline namespaces are available in Clang/GCC/MSVC regardless of C++ dialect.
 // clang-format off
-#  define _LIBCPP_BEGIN_NAMESPACE_STD namespace _LIBCPP_TYPE_VISIBILITY_DEFAULT std {                                  \
+#  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("-Wc++23-extensions")                           \
+                                      _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wc++26-extensions")                           \
+                                      _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")                             \
+                                      _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wc++26-extensions")                             \
+                                      namespace _LIBCPP_TYPE_VISIBILITY_DEFAULT std {                                  \
                                inline namespace _LIBCPP_ABI_NAMESPACE {
-#  define _LIBCPP_END_NAMESPACE_STD }}
+#  define _LIBCPP_END_NAMESPACE_STD }} _LIBCPP_DIAGNOSTIC_POP
 
 #  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__)
@@ -1322,23 +1351,6 @@ __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 0e5f0b4831b41e7..a2bacad183055f3 100644
--- a/libcxx/include/tuple
+++ b/libcxx/include/tuple
@@ -550,10 +550,6 @@ 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,
@@ -835,8 +831,6 @@ 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)

libcxx/include/__config

ldionne

We discussed it multiple times and this is the end goal we've been driving towards. I think this has the potential to greatly simplify our implementation in places.

However, let's be reasonable and only use these extensions when they are actually useful -- in particular we shouldn't start reimplementing stuff using extensions just because we can, we should only do it when there's a tangible benefit. Let's use our judgement.

var-const · 2024-02-01T20:55:13Z

This is awesome! Is there a list of which extensions are available in which language modes -- or perhaps we should create one? It doesn't have to be exhaustive -- just listing some of the major, most clearly useful extensions with the minimum language standard where it can be used would be really useful, IMO (we can also have a checkbox saying whether there's already precedent for using the extension in the codebase, it might help rolling this out more gradually).

mordante · 2024-02-02T17:32:47Z

We've talked about allowing extensions on discourse and in a libc++ monthly meeting and agreed to test it out in the LLVM 18 release. We've done that with the tuple constructor overload set (using conditional explicit). Since we haven't heard about any breakages, it seems safe to do.

Since we don't have a LLVM-18 release yet and presumably not a lot of usage except for users HEAD I feel this conclusion is premature. I really like to wait until after the release and see whether we get breakage before moving forward with this patch.

philnik777 · 2024-02-02T17:45:54Z

We've talked about allowing extensions on discourse and in a libc++ monthly meeting and agreed to test it out in the LLVM 18 release. We've done that with the tuple constructor overload set (using conditional explicit). Since we haven't heard about any breakages, it seems safe to do.

Since we don't have a LLVM-18 release yet and presumably not a lot of usage except for users HEAD I feel this conclusion is premature. I really like to wait until after the release and see whether we get breakage before moving forward with this patch.

We've had this in trunk for months now and nobody said anything. rc1 has been out for a week now and nobody said anything. If we caused any non-trivial breakage I'm pretty sure we would have heard about it by now. Everybody wants to start simplifying the code, and I don't think "but maybe" should stop us from doing so by now.

ldionne · 2024-02-02T20:47:32Z

@mordante @philnik777 I'd be fine to wait until after we have 18.1.0 released (maybe 1-2 weeks after that) before we actually merge this. At this point we've been waiting for long enough that a few weeks is not going to make much of a difference.

For what it's worth, I have been deploying LLVM 18 on a large code base internally and didn't see any issues around the use of these extensions, so I am not too concerned at this point. But I agree that a full point release would make sense before we actually merge this.

mordante · 2024-03-03T15:36:08Z

@mordante @philnik777 I'd be fine to wait until after we have 18.1.0 released (maybe 1-2 weeks after that) before we actually merge this. At this point we've been waiting for long enough that a few weeks is not going to make much of a difference.

For what it's worth, I have been deploying LLVM 18 on a large code base internally and didn't see any issues around the use of these extensions, so I am not too concerned at this point. But I agree that a full point release would make sense before we actually merge this.

Thanks for testing this. My main concern was never with users who we know about, but about the users we don't know about.

EricWF · 2024-03-08T20:31:58Z

Could you adjust the commit message to better describe which extensions we're enabling when?

sam-mccall · 2024-03-14T13:18:49Z

We're seeing major compile-time regressions (50%+) from this change in normal-looking code that uses libc++, using clang from head.

My best guess is that we're now adding lots of diagnostics-overridden regions to most TUs, and querying them (when clang checks: should we emit diagnostic X) is inefficient. I will dig a bit into this.

If this is not related to our use of modules (I need to check) then I think we should revert this and hopefully find a cheaper way. WDYT?
If it is modules-related, then it seems more complicated and will depend on the details.

sam-mccall · 2024-03-14T13:19:59Z

(Sorry this is coming a week after the patch landed: internally we do basic correctness testing within days when we pull in the code, but only do performance testing for releases)

EricWF · 2024-03-14T13:22:04Z

If it is because of the diagnostic overrides, we should be able to go back to simply using the pragma system header.

The diagnostic disabling is only really for our test suite (and people who enable -Wsystem-header, but they asking to see diagnostics). We should just be able to disable the warning entirely in the test suite. IDK Why we have it on when we allow the use of extensions.

@philnik777 Can you say why you chose to go this route?

sam-mccall · 2024-03-14T13:41:29Z

Thanks! If we can avoid this entirely that seems ideal for everyone.

We only see the regression in our modular builds. (Which unfortunately is our main mode).
We use clang header modules, but I'd guess this isn't specific to that mode.
libc++ has modules buildbots right? I'll see if I can scrape some timings out of those, 50% might be possible to see through noise.

EricWF · 2024-03-14T13:54:18Z

Is there a way to observe the issue or at least the structure that causes the issue in the AST?

…

On Thu, Mar 14, 2024 at 9:41 AM Sam McCall ***@***.***> wrote: Thanks! If we can avoid this entirely that seems ideal for everyone. We only see the regression in our modular builds. (Which unfortunately is our main mode). We use clang header modules, but I'd guess this isn't specific to that mode. libc++ has modules buildbots right? I'll see if I can scrape some timings out of those, 50% might be possible to see through noise. — Reply to this email directly, view it on GitHub <#79532 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI7G2Z3X5BCHZFJO5O3J2LYYGSKBAVCNFSM6AAAAABCLMZLJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJXGQ4DQOBYHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

EricWF · 2024-03-14T14:14:30Z

Actually, sorry I need to step back from this review.

philnik777 · 2024-03-14T14:21:01Z

We're seeing major compile-time regressions (50%+) from this change in normal-looking code that uses libc++, using clang from head.

My best guess is that we're now adding lots of diagnostics-overridden regions to most TUs, and querying them (when clang checks: should we emit diagnostic X) is inefficient. I will dig a bit into this.

I'm pretty sure I've checked compile times before and after and didn't see any significant regression. While it's been a bit more than noise, it didn't seem significant enough to be worrisome, especially since should be able to offset that by simplifying the code.

If this is not related to our use of modules (I need to check) then I think we should revert this and hopefully find a cheaper way. WDYT? If it is modules-related, then it seems more complicated and will depend on the details.

I don't think we can revert. We're already depending on this in parts of the code base. IMO we should fix forward instead if this is actually a problem with this approach.

If it is because of the diagnostic overrides, we should be able to go back to simply using the pragma system header.

The diagnostic disabling is only really for our test suite (and people who enable -Wsystem-header, but they asking to see diagnostics). We should just be able to disable the warning entirely in the test suite. IDK Why we have it on when we allow the use of extensions.

@philnik777 Can you say why you chose to go this route?

We're still using #pragma GCC system_header. I've done it this way to avoid introducing extensions in the test suite, since that shouldn't be using extensions as much as possible as a conformance test suite and it's used by other implementations.

sam-mccall · 2024-03-14T14:25:06Z

No worries @EricWF, thanks for your help.

The regression is time spent in ASTReader::ReadPragmaDiagnosticMappings, which explains why it's modules-specific.

here's the baseline

and a ridiculously-wide screenshot of just that function

(sorry I don't have a reasonable way to share the underlying data)

sam-mccall · 2024-03-14T14:37:54Z

My approximate understanding is that any query of diagnostic overrides ends up loading all the mappings (which is a bunch of work), and in order to understand the region they apply to it has to page in source-level information (since they are source-location based).
Maybe this can be fixed eventually in clang, but we need some solution meanwhile.

@philnik777 thanks for the context!

I'm pretty sure I've checked compile times before and after and didn't see any significant regression

(I would assume this is using libc++ textually rather than as modules, but just want to check)

We're already depending on this in parts of the code base. IMO we should fix forward instead if this is actually a problem with this approach.

Can you tell me how to check how much would break? (I know how to test llvm but not libcxx specifically).
As a short-term fix, we could enable extensions for just those regions/files until we find a way to do it globally that scales well.

I've done it this way to avoid introducing extensions in the test suite, since that shouldn't be using extensions as much as possible as a conformance test suite and it's used by other implementations

It seems nice to enforce this, but if we can't revert and there isn't another obvious fix-forward, is it possible to live without this enforcement? Or move the pragmas out of libc++ proper and into the testsuite?

philnik777 · 2024-03-14T15:33:27Z

My approximate understanding is that any query of diagnostic overrides ends up loading all the mappings (which is a bunch of work), and in order to understand the region they apply to it has to page in source-level information (since they are source-location based). Maybe this can be fixed eventually in clang, but we need some solution meanwhile.

@philnik777 thanks for the context!

I'm pretty sure I've checked compile times before and after and didn't see any significant regression

(I would assume this is using libc++ textually rather than as modules, but just want to check)

Yes. I did not at all expect there to be a significant difference between normal header inclusion and clang modules.

We're already depending on this in parts of the code base. IMO we should fix forward instead if this is actually a problem with this approach.

Can you tell me how to check how much would break? (I know how to test llvm but not libcxx specifically). As a short-term fix, we could enable extensions for just those regions/files until we find a way to do it globally that scales well.

You should be able to run the test suite with ninja check-libcxx.

I've done it this way to avoid introducing extensions in the test suite, since that shouldn't be using extensions as much as possible as a conformance test suite and it's used by other implementations

It seems nice to enforce this, but if we can't revert and there isn't another obvious fix-forward, is it possible to live without this enforcement? Or move the pragmas out of libc++ proper and into the testsuite?

I'd really like to avoid not enforcing this, since I expect there to be a significant amount of clean-up to pile up over a few months, maybe even making it almost prohibitive to revert. I also don't see a way to move the pragmas to the test suite.

While it's not pretty, I think we could disable the pragmas when using clang modules with #if __has_feature(modules). We already suppress all diagnostics in the modules configuration anyways. Since the problem seems to be modules-specific this should make everyone happy. WDYT?

sam-mccall · 2024-03-14T15:53:56Z

Yes, that solves the problems I know of.

Would need other libc++ maintainers to weigh in on what the right tradeoff is here.

…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

philnik777 requested a review from a team as a code owner January 26, 2024 00:50

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 26, 2024

philnik777 force-pushed the allow_extensions branch from b134c7c to 033ba90 Compare January 26, 2024 10:53

ldionne reviewed Jan 30, 2024

View reviewed changes

libcxx/include/__config Show resolved Hide resolved

libcxx/include/__config Show resolved Hide resolved

ldionne approved these changes Feb 1, 2024

View reviewed changes

philnik777 force-pushed the allow_extensions branch 2 times, most recently from b7e858c to 470e009 Compare February 2, 2024 16:31

philnik777 force-pushed the allow_extensions branch from 470e009 to dc770a3 Compare February 2, 2024 17:50

philnik777 force-pushed the allow_extensions branch from dc770a3 to fc36df2 Compare February 28, 2024 20:45

philnik777 force-pushed the allow_extensions branch from fc36df2 to d8aaa30 Compare March 7, 2024 11:30

[libc++] Allow the use of extensions in the implementation

806411c

philnik777 force-pushed the allow_extensions branch from d8aaa30 to 806411c Compare March 8, 2024 20:07

philnik777 merged commit 4d323e4 into llvm:main Mar 9, 2024

philnik777 deleted the allow_extensions branch March 9, 2024 00:09

sam-mccall mentioned this pull request Mar 19, 2024

Revert "[libc++] Allow the use of extensions in the implementation (#79532)" #85792

Closed

frederick-vs-ja mentioned this pull request Oct 22, 2024

[libc++] __uglify [[clang::noescape]] #113280

Merged

frederick-vs-ja mentioned this pull request Feb 11, 2025

some libc++ 19.1.0 headers no longer compile with C++11 #126733

Closed

[libc++] Allow the use of extensions in the implementation #79532

[libc++] Allow the use of extensions in the implementation #79532

Uh oh!

Conversation

philnik777 commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 26, 2024

Uh oh!

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

var-const commented Feb 1, 2024

Uh oh!

mordante commented Feb 2, 2024

Uh oh!

philnik777 commented Feb 2, 2024

Uh oh!

ldionne commented Feb 2, 2024

Uh oh!

mordante commented Mar 3, 2024

Uh oh!

EricWF commented Mar 8, 2024

Uh oh!

sam-mccall commented Mar 14, 2024

Uh oh!

sam-mccall commented Mar 14, 2024

Uh oh!

EricWF commented Mar 14, 2024

Uh oh!

sam-mccall commented Mar 14, 2024

Uh oh!

EricWF commented Mar 14, 2024 via email

Uh oh!

EricWF commented Mar 14, 2024

Uh oh!

philnik777 commented Mar 14, 2024

Uh oh!

sam-mccall commented Mar 14, 2024

Uh oh!

sam-mccall commented Mar 14, 2024

Uh oh!

philnik777 commented Mar 14, 2024

Uh oh!

sam-mccall commented Mar 14, 2024

Uh oh!

Uh oh!

philnik777 commented Jan 26, 2024 •

edited

Loading