[libc++] Use __wrap_iter in string_view and array in the unstable ABI

[ldionne]

std::string_view and std::array iterators don't have to be raw pointers,
and in fact other implementations don't represent them as raw pointers.
Them being raw pointers in libc++ makes it easier for users to write
non-portable code. This is bad in itself, but this is even worse when
considering efforts like hardening where we want an easy ability to
swap for a different iterator type. If users depend on iterators being
raw pointers, this becomes a build break.

Hence, this patch enables the use of __wrap_iter in the unstable ABI,
creating a long term path towards making this the default. This patch
may break code that assumes these iterators are raw pointers for
people compiling with the unstable ABI.

This patch also removes several assumptions that array iterators are
raw pointers in the code base and in the test suite.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

std::string_view and std::array iterators don't have to be raw pointers, and in fact other implementations don't represent them as raw pointers. Them being raw pointers in libc++ makes it easier for users to write non-portable code. This is bad in itself, but this is even worse when considering efforts like hardening where we want an easy ability to swap for a different iterator type. If users depend on iterators being raw pointers, this becomes a build break.

Hence, this patch enables the use of __wrap_iter in the unstable ABI, creating a long term path towards making this the default. This patch may break code that assumes these iterators are raw pointers for people compiling with the unstable ABI.

---

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

3 Files Affected:

• (modified) libcxx/include/__config (+6)
• (modified) libcxx/include/array (+8-2)
• (modified) libcxx/include/string_view (+5-2)

diff --git a/libcxx/include/__config b/libcxx/include/__config
index 3de4d610e8cde..636f46b048554 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -168,6 +168,12 @@
 // pointer from 16 to 8. This changes the output of std::string::max_size,
 // which makes it ABI breaking
 #    define _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
+// Define std::array iterators to be __wrap_iters instead of raw pointers, which
+// prevents people from relying on a non-portable implementation detail.
+#    define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY
+// Define std::basic_string_view iterators to be __wrap_iters instead of raw pointers,
+// which prevents people from relying on a non-portable implementation detail.
+#    define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW
 #  elif _LIBCPP_ABI_VERSION == 1
 #    if !(defined(_LIBCPP_OBJECT_FORMAT_COFF) || defined(_LIBCPP_OBJECT_FORMAT_XCOFF))
 // Enable compiling copies of now inline methods into the dylib to support
diff --git a/libcxx/include/array b/libcxx/include/array
index fc5371ebae21a..988464844f866 100644
--- a/libcxx/include/array
+++ b/libcxx/include/array
@@ -120,6 +120,7 @@ template <size_t I, class T, size_t N> const T&& get(const array<T, N>&&) noexce
 #include <__config>
 #include <__fwd/array.h>
 #include <__iterator/reverse_iterator.h>
+#include <__iterator/wrap_iter.h>
 #include <__tuple/sfinae_helpers.h>
 #include <__type_traits/conditional.h>
 #include <__type_traits/is_array.h>
@@ -168,10 +169,15 @@ struct _LIBCPP_TEMPLATE_VIS array
     typedef _Tp                                   value_type;
     typedef value_type&                           reference;
     typedef const value_type&                     const_reference;
-    typedef value_type*                           iterator;
-    typedef const value_type*                     const_iterator;
     typedef value_type*                           pointer;
     typedef const value_type*                     const_pointer;
+#if defined(_LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY)
+    typedef __wrap_iter<pointer>                  iterator;
+    typedef __wrap_iter<const_pointer>            const_iterator;
+#else
+    typedef pointer                               iterator;
+    typedef const_pointer                         const_iterator;
+#endif
     typedef size_t                                size_type;
     typedef ptrdiff_t                             difference_type;
     typedef _VSTD::reverse_iterator<iterator>       reverse_iterator;
diff --git a/libcxx/include/string_view b/libcxx/include/string_view
index 881e321c507f6..8af67d5e5bb11 100644
--- a/libcxx/include/string_view
+++ b/libcxx/include/string_view
@@ -215,6 +215,7 @@ namespace std {
 #include <__iterator/concepts.h>
 #include <__iterator/iterator_traits.h>
 #include <__iterator/reverse_iterator.h>
+#include <__iterator/wrap_iter.h>
 #include <__memory/pointer_traits.h>
 #include <__ranges/concepts.h>
 #include <__ranges/data.h>
@@ -279,10 +280,12 @@ public:
     using const_pointer          = const _CharT*;
     using reference              = _CharT&;
     using const_reference        = const _CharT&;
-#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+#if defined(_LIBCPP_ABI_BOUNDED_ITERATORS)
     using const_iterator         = __bounded_iter<const_pointer>;
+#elif defined(_LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW)
+    using const_iterator         = __wrap_iter<const_pointer>;
 #else
-    using const_iterator         = const_pointer; // See [string.view.iterators]
+    using const_iterator         = const_pointer;
 #endif
     using iterator               = const_iterator;
     using const_reverse_iterator = _VSTD::reverse_iterator<const_iterator>;

[ldionne]

CC @llvm/libcxx-vendors since this can break some code. CC @EricWF for awareness in particular.

[philnik777]

LGTM.

[cjdb]

I very much like this change for its hardening improvements. I'm going to check how much code this impacts internally right now: would it be possible to hold off merging this for a day or so, so that I can get details on what we need to do, please?

[ldionne]

I very much like this change for its hardening improvements. I'm going to check how much code this impacts internally right now: would it be possible to hold off merging this for a day or so, so that I can get details on what we need to do, please?

Yup, of course, the intent was to upload this and let folks see how much impact it has. Please let me know once you have some information. I think Google is going to be the main way we can get information here since most vendors ship the stable ABI.

[cjdb]

I very much like this change for its hardening improvements. I'm going to check how much code this impacts internally right now: would it be possible to hold off merging this for a day or so, so that I can get details on what we need to do, please?

Yup, of course, the intent was to upload this and let folks see how much impact it has. Please let me know once you have some information. I think Google is going to be the main way we can get information here since most vendors ship the stable ABI.

Excellent, thank you! I'll give you an answer as soon as possible (likely tomorrow).

[github-actions]

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

[philnik777]

That got a lot bigger. Maybe we should split the array and string_view changes?

[ldionne]

That got a lot bigger. Maybe we should split the array and string_view changes?

You're right, I debated with myself and decided to keep it in the same patch, but it's cleaner not to. This is now #74624, I'll rebase this after landing that other patch (please take a look!).

[ldionne]

@cjdb I think this should be good to test now! Make sure to apply it on top of main, since it has one dependent patch.

[ldionne]

Pinging reviewers.

[ldionne]

@cjdb Do you think it'd be possible to take this for a run internally to see the impact? This is not a must for LLVM 18 but I'd like to unblock this.

[cjdb]

Sorry, I think this got lost in the haze that is December. I'll get an answer ASAP.

[ldionne]

Oh yeah, no worries! This is not an emergency, I just wanted to avoid forgetting this forever :)

[ldionne]

Gentle ping @cjdb , did you have any luck running this internally?

[ldionne]

I spoke to @cjdb offline who told me there was a non-insignificant number of build failures when enabling this. However, this change can easily be disabled temporarily (while the code base migrates) by not defining _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY and _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW.

Our ABI flags have that benefit that one can pick and choose what's opted into, so let's use this here. Hence, I think it is reasonable to move forward with the change, and vendors who envision breakage should simply disable these ABI macros until they are ready to transition.

CC @llvm/libcxx-vendors again

[rnk]

However, this change can easily be disabled temporarily (while the code base migrates) by not defining _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY and _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW.

Thanks for the migration pathway!

[steelannelida]

@rnk @ldionne could you please provide a more detailed way to work around this?

Is the suggestion to modify __config and undefine _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY and _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW? Unfortunately this cannot be done in __config_site because the macros are defined after its inclusion.

[ldionne]

You can either modify __config to undefine those macros in the source, or you can start opting into individual ABI flags instead of opting into all of them by switching from LIBCXX_ABI_VERSION = 2 (in CMake) to LIBCXX_ABI_DEFINES (in CMake). Depending on your use case, it may be simpler to just undefine the two macros locally for your transition period or to switch to a purely opt-in approach for ABI flags, but both work.

[cjdb]

If it's causing lots of pain, would it be possible to make this opt-in for two releases, then switch it to opt-out (and document that we'll be doing this no later than two releases time)? That'll give folks time to address this issue without being broken today.

To avoid having users need to either modify __config or need to maintain an unbounded set of macros, what do you think about flipping the macro? Then users who don't want/can't yet use this only need to add one macro instead of many.

#if defined(_LIBCPP_ABI_BOUNDED_ITERATORS)
  using const_iterator = __bounded_iter<const_pointer>;
#elif defined(_LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW)
  using const_iterator = __wrap_iter<const_pointer>;
#else
  using const_iterator = const_pointer;
#endif

to

#if defined(_LIBCPP_ABI_BOUNDED_ITERATORS)
  using const_iterator = __bounded_iter<const_pointer>;
#elif !defined(_LIBCPP_ABI_NO_USE_WRAP_ITER_IN_STD_STRING_VIEW) // macro adds "NO"
  using const_iterator = const_pointer;
#else
  using const_iterator = __wrap_iter<const_pointer>;
#endif

Apologies for not thinking of this when we chatted last week.

[cjdb]

I have a patch that's slightly different to the forward-fix above, and I think lets us all win here.

[PiJoules]

If it's causing lots of pain, would it be possible to make this opt-in for two releases, then switch it to opt-out (and document that we'll be doing this no later than two releases time)? That'll give folks time to address this issue without being broken today.

To avoid having users need to either modify __config or need to maintain an unbounded set of macros, what do you think about flipping the macro? Then users who don't want/can't yet use this only need to add one macro instead of many.

#if defined(_LIBCPP_ABI_BOUNDED_ITERATORS)
  using const_iterator = __bounded_iter<const_pointer>;
#elif defined(_LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW)
  using const_iterator = __wrap_iter<const_pointer>;
#else
  using const_iterator = const_pointer;
#endif

to

#if defined(_LIBCPP_ABI_BOUNDED_ITERATORS)
  using const_iterator = __bounded_iter<const_pointer>;
#elif !defined(_LIBCPP_ABI_NO_USE_WRAP_ITER_IN_STD_STRING_VIEW) // macro adds "NO"
  using const_iterator = const_pointer;
#else
  using const_iterator = __wrap_iter<const_pointer>;
#endif

Apologies for not thinking of this when we chatted last week.

Do you have a PR for something like this? I think it would also be nice to have a macro we can set in our build that doesn't involve editing the __config file or opting out of LIBCXX_ABI_VERSION = 2.

[ldionne]

@PiJoules Please see #84226 (comment).