[libcxx] adds opt out mechanism for using __wrap_iter for array and s…

[cjdb]

…tring_view

__config was unconditionally opting users into this change, and to opt out required either a manual edit of the header, or getting users to manually define each unstable ABI macro they want to support. This patch strikes a middle-ground by adding an opt-out mechanism.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Christopher Di Bella (cjdb)

Changes

…tring_view

__config was unconditionally opting users into this change, and to opt out required either a manual edit of the header, or getting users to manually define each unstable ABI macro they want to support. This patch strikes a middle-ground by adding an opt-out mechanism.

---

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

2 Files Affected:

• (modified) libcxx/docs/ReleaseNotes/19.rst (+3)
• (modified) libcxx/include/__config (+8-2)

diff --git a/libcxx/docs/ReleaseNotes/19.rst b/libcxx/docs/ReleaseNotes/19.rst
index 04f16610f8117e..955cadaa914439 100644
--- a/libcxx/docs/ReleaseNotes/19.rst
+++ b/libcxx/docs/ReleaseNotes/19.rst
@@ -49,6 +49,9 @@ Improvements and New Features
 -----------------------------
 
 - The performance of growing ``std::vector`` has been improved for trivially relocatable types.
+- ``std::array`` and ``std::string_view`` are no longer pointers in the unstable ABI by default, and
+  can be reverted to raw pointers by defining the macros ``_LIBCPP_ABI_NO_USE_WRAP_ITER_IN_STD_ARRAY``
+  and ``_LIBCPP_ABI_NO_USE_WRAP_ITER_IN_STD_STRING_VIEW``.
 
 Deprecations and Removals
 -------------------------
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 3a438e85a7b819..70f15cce54a309 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -178,8 +178,14 @@
 // pointers, which prevents people from relying on a non-portable implementation
 // detail. This is especially useful because enabling bounded iterators hardening
 // requires code not to make these assumptions.
-#    define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY
-#    define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW
+
+#    if !defined(_LIBCPP_ABI_NO_USE_WRAP_ITER_IN_STD_ARRAY)
+#        define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY
+#    endif
+
+#    if !defined(_LIBCPP_ABI_NO_USE_WRAP_ITER_IN_STD_STRING_VIEW)
+#        define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW
+#    endif
 #  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

[ldionne]

It wasn't my impression that anybody was requesting a different mechanism though, so I'd rather not introduce this additional complexity. After all, the requirement of being able to tweak this value without a "vendor" modifying the headers seems arbitrary to me, at that point it's just as good as setting something in the __config_site.

[cjdb]

It wasn't my impression that anybody was requesting a different mechanism though, so I'd rather not introduce this additional complexity. After all, the requirement of being able to tweak this value without a "vendor" modifying the headers seems arbitrary to me, at that point it's just as good as setting something in the __config_site.

Some vendors have a "no custom patches" policy, meaning that they're unable to adjust this without opting into everything manually, which has problems of its own. It's a relatively small maintenance cost for us to make these opt-out, but a potentially large maintenance cost to ask vendors to constantly monitor what's added to the unstable ABI.

I can make it less arbitrary by adding opt-outs for everything, which was going to be my next patch. We could alternatively start it as a Discourse discussion and then move to GitHub if there's consensus.

A good analogy is Clang's warning policy: most are opt-in by default, and all are opt-out with -Wno-foo.

[ldionne]

Some vendors have a "no custom patches" policy, meaning that they're unable to adjust this without opting into everything manually, which has problems of its own.

This is an arbitrary policy, though. I totally get the desire to minimize custom patches, but eradicating them entirely is not realistic unless that vendor pushes a lot of its complexities upstream. I'm saying this irrespective of the specific issue we have right here.

It's a relatively small maintenance cost for us to make these opt-out, but a potentially large maintenance cost to ask vendors to constantly monitor what's added to the unstable ABI.

If a vendor finds the need to constantly monitor what's added to the unstable ABI, it means they simply shouldn't be opting into the unstable ABI wholesale by setting _LIBCPP_ABI_VERSION = 2. They should instead be cherry-picking ABI macros via LIBCXX_ABI_DEFINES in the __config_site as they see fit. This is my main point -- it doesn't make sense to opt into "all of the breaks" but then require opt-outs for some breaks. Just don't opt into everything in the first place.

I can make it less arbitrary by adding opt-outs for everything, which was going to be my next patch. We could alternatively start it as a Discourse discussion and then move to GitHub if there's consensus.

For the reason stated above, I don't think it makes sense to do that.

[cjdb]

at that point it's just as good as setting something in the __config_site.

It seems I misread this as __config, and thus unintentionally reframed the conversation. I'm not familiar with how to interact with __config_site: would it be possible to get some pointers on what one should do to customise this file please?

[ldionne]

at that point it's just as good as setting something in the __config_site.

It seems I misread this as __config, and thus unintentionally reframed the conversation. I'm not familiar with how to interact with __config_site: would it be possible to get some pointers on what one should do to customise this file please?

It is possible to set arbitrary ABI defines via LIBCXX_ABI_DEFINES here: https://github.com/llvm/llvm-project/blob/main/libcxx/CMakeLists.txt#L225

Those end up in the __config_site generated via this mechanism: https://github.com/llvm/llvm-project/blob/main/libcxx/include/__config_site.in#L48

So the canonical way of cherry-picking ABI flags to enable via e.g. a CMake cache is to set LIBCXX_ABI_DEFINES as desired and let the __config_site pick them up, without setting LIBCXX_ABI_VERSION=2. Setting LIBCXX_ABI_VERSION=2 is basically a way to say "I want to get all the ABI flags whatever they are and I am ready to live on the edge", which is presumably not what most people want.

[cjdb]

That's very clear, thank you for breaking it down!