Fix __split_buffer_pointer_layout compatibility with swift's C++ interop

[junov-google]

When the swift compiler generates a bridging module for importing C++ symbols, it instantiates all methods of the class template instances used by the symbols imported from C++. This is because it cannot determine which methods will be called from swift at that stage. This means that it may attempt to generate invalid instantiations (due to missing type traits) that would not be generated in a normal C++ build.

The default constructor of __split_buffer_pointer_layout has this issue because it does not initialize __alloc_ which may or may not be a reference depending on the allocator_type template parameter. The initialization of class members that are references is mandatory. vector uses a reference type for the allocator and deque does not. Therefore, only deque is allowed to use the default constructor of __split_buffer_pointer_layout.

When the swift compiler's C++ importer generates an instantiation of std::vector, it will also attempt to instantiate the default constructor of __split_buffer_pointer_layout, which fails to compile.

This change fixes the issue by adding a requires clause to suppress the instantiation of the problematic constructor whenever its instatiation would be invalid.

There are no tests included in this change because the requires statement has no observable effect on pure C++ builds. Test coverage will be automatically assured downstream in swift-project once it updates its fork of libc++.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: None (junov-google)

Changes

When the swift compiler generates a bridging module for importing C++ symbols, it instantiates all methods of the class template instances used by the symbols imported from C++. This is because it cannot determine which methods will be called from swift at that stage. This means that it may attempt to generate invalid instantiations (due to missing type traits) that would not be generated in a normal C++ build.

The default constructor of __split_buffer_pointer_layout has this issue because it does not initialize __alloc_ which may or may not be a reference depending on the allocator_type tempalte parameter. The initialization of class members that are references is mandatory. vector uses a reference type for the allocator and deque does not. Therefore, only deque is allowed to use the default constructor of __split_buffer_pointer_layout.

When the swift compiler's C++ importer generates an instantiation of std::vector, it will also attempt to instantiate the default constructor of __split_buffer_pointer_layout, which fails to compile.

This change fixes the issue by adding a requires clause to suppress the instantiation of the problematic constructor whenever its instatiation would be invalid.

There are no tests included in this change because the requires statement has no observable effect on pure C++ builds. Test coverage will be automatically assured downstream in swift-project once it updates its fork of libc++.

---

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

1 Files Affected:

• (modified) libcxx/include/__split_buffer (+10-2)

diff --git a/libcxx/include/__split_buffer b/libcxx/include/__split_buffer
index 1e05e4df8ba0f..5e4e265b10296 100644
--- a/libcxx/include/__split_buffer
+++ b/libcxx/include/__split_buffer
@@ -30,6 +30,9 @@
 #include <__type_traits/integral_constant.h>
 #include <__type_traits/is_nothrow_assignable.h>
 #include <__type_traits/is_nothrow_constructible.h>
+#if _LIBCPP_STD_VER >= 20
+#  include <__type_traits/is_reference.h>
+#endif
 #include <__type_traits/is_swappable.h>
 #include <__type_traits/is_trivially_destructible.h>
 #include <__type_traits/is_trivially_relocatable.h>
@@ -68,8 +71,13 @@ protected:
 
 public:
   // Can't be defaulted due to _LIBCPP_COMPRESSED_PAIR not being an aggregate in C++03 and C++11.
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI __split_buffer_pointer_layout() : __back_cap_(nullptr) {}
-
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI __split_buffer_pointer_layout()
+#if _LIBCPP_STD_VER >= 20
+      // Prevents Swift compiler's C++ interop from implicitly instantiating this ctor when it's not supported.
+      requires (!is_reference_v<allocator_type>)
+#endif
+      : __back_cap_(nullptr) {}
+  
   _LIBCPP_CONSTEXPR_SINCE_CXX20
   _LIBCPP_HIDE_FROM_ABI explicit __split_buffer_pointer_layout(const allocator_type& __alloc)
       : __back_cap_(nullptr), __alloc_(__alloc) {}

[cjdb]

Thanks for the patch! LGTM once CI is green.

Do you have permission to merge, or do you need someone to do that on your behalf?

[junov-google]

@cjdb Wrote:

Do you have permission to merge?

I do not have any privileges in this repo. I just rebased the branch and now I have workflows that need approval again.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[philnik777]

We generally require testing for changes. Here specifically, this is making a use-case work which we're not supporting so far, so IMO this definitely requires and RFC.

[junov-google]

This is making a use-case work which we're not supporting so far, so IMO this definitely requires and RFC.

RFC created: https://discourse.llvm.org/t/rfc-adding-requires-statement-in-split-buffer-h-for-compatibility-with-swift-compiler/89092

[junov-google]

We generally require testing for changes.

I tried really hard to find a way to test this but I have not found a way that does not have the swift compiler in the loop. The "requires" constraint does not appear to have any observable effects in pure C++ builds. Are we okay with only having tests for this in the downstream (swift-project) repo?

[ldionne]

I think there are two interesting issues here:

1. The Swift compiler tries to instantiate all methods of a C++ type because it cannot tell in advance which ones are going to be used. I think that is something we need to find a solution for on the Swift compiler side, since C++ classes (in general, not just libc++) very often have inline methods that won't compile with a given specialization of the template. Those work fine in C++ (whether or not that's good design), so it's pretty common. I do not think that it is desirable, realistic or even feasible to resolve all such issues in libc++. I'm pretty sure there are places where we need to write our code that way in order to be conforming. At the very least, we can't commit to fixing all such cases.
2. The specific issue at hand where __split_buffer is being instantiated with _Allocator&. Honestly, I think that's just not a great way of writing our code in std::vector. I would prefer that we refactor the uses of __split_buffer to use an allocator adaptor that holds the local allocator by reference (or by pointer) internally. We never instantiate a container with a reference to an allocator (AFAICT) except in this case, and it feels weird to do it here.

So, bottom line, I think we should fix the libc++ code by ensuring that we don't use this pattern in std::vector. However, I think the underlying issue persists and is not something we can work around in libc++ -- we'll need to figure out a way for Swift interop to work with that quirkiness of C++ for both the standard library and for user code.

[cjdb]

We generally require testing for changes.

I tried really hard to find a way to test this but I have not found a way that does not have the swift compiler in the loop. The "requires" constraint does not appear to have any observable effects in pure C++ builds. Are we okay with only having tests for this in the downstream (swift-project) repo?

libc++ is very on-board with pre-commit CI, which means we can't carry this test outside of libc++ and be "okay" with it (sorry). The good news is that we don't need the Swift compiler for this. Clang supports compile-time verification checks, which allow you to test that the diagnostic you get is based on the requires-clause that you've added.

I'm not sure how to propose a new file to someone else's PR without directly committing to it (and I want you to run this locally), so here's the test in question.

//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include <__split_buffer>

std::__split_buffer_pointer_layout<void, int, std::allocator<int>&> sb;
// expected-error@*:*{{no matching constructor for initialization of 'std::__split_buffer_pointer_layout<void, int, std::allocator<int>&>'}}
// expected-note@*:*{{candidate constructor not viable: constraints not satisfied}}
// expected-note@*:*{{because '!is_reference_v<allocator_type>' evaluated to false}}
// expected-note N {{.*}}

You'll need to replace the N on the last line with the number of excess notes that are generated (delete if none). Unless someone else thinks there's a better place, I'd put this under libcxx/test/containers/sequences/__split_buffer/pointer_layout/constructor_default.verify.cpp.

[cjdb]

The specific issue at hand where __split_buffer is being instantiated with _Allocator&. Honestly, I think that's just not a great way of writing our code in std::vector. I would prefer that we refactor the uses of __split_buffer to use an allocator adaptor that holds the local allocator by reference (or by pointer) internally. We never instantiate a container with a reference to an allocator (AFAICT) except in this case, and it feels weird to do it here.

I'm happy to do that work after #155330 lands, but this PR is probably a good fix until that lands, since users are feeling pain right now.

[junov-google]

std::__split_buffer_pointer_layout<void, int, std::allocator&> sb;
// expected-error@:{{no matching constructor for initialization of 'std::__split_buffer_pointer_layout<void, int, std::allocator&>'}}
// expected-note@:{{candidate constructor not viable: constraints not satisfied}}
// expected-note@:{{because '!is_reference_v<allocator_type>' evaluated to false}}
// expected-note N {{.*}}

This is exactly the kind of test I was trying to write but I wasn't able to get it to detect the the change from this fix. The expected-error part works fine, but that part passes with and without this fix. The expected-note expectations don't work. I'm guessing there a special incantation of // RUN: that will make this work... I'll keep trying.

[ldionne]

The issue I see with landing this patch as-is is that it creates a precedent for trying to support something that we don't officially support right now. Whether this constructor can be instantiated or not shouldn't be an observable property of the library. It only ends up being observable because the Swift compiler is asking Clang to instantiate a function that would normally never be instantiated, which is not something we can realistically support. Hence, it doesn't make sense to even start adding band-aid fixes in libc++ to fix these kinds of issues, instead I'd rather fix the underlying issue.

That being said, I do agree that we should unblock this issue, so I've opened #171651 which I believe should also fix this problem. #171651 is a pure refactoring that simplifies our code, so it doesn't introduce a precedent for something we don't want to and can't support in the general case.

I've also spoken to @egorzhdan who manages the Swift interop effort and he's told me they would look into the best way of fixing this general problem going forward. But obviously there's a longer time line for that.

Edit: To give more context around why I don't think this is something we can support in the general case, we'll have similar bug reports when reflection is implemented and people start using it. There will be new ways to introspect and rely on things that were previously completely non-observable, and in many cases it won't make sense or be realistic to support those use cases. This use case falls into that bucket, except the Swift compiler is the one doing "the introspection" via the Clang compiler, by asking it to instantiate something it shouldn't.

[junov-google]

@ldionne Understood. I'm closing this PR then. Thanks for PR #171651 That should unblock us.