[libc++] Introduce _LIBCPP_ABI_BOUNDED_ITERATORS_IN_{STRING_VIEW,SPAN} #143172
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesFor consistency with other containers, introduce per-container macros to enable bounded iterators in span and string_view. This patch also keeps the original macro around for backwards compatibility, although we probably want to deprecate and eventually remove it in a separate patch since the name can lead to confusion. Full diff: https://github.com/llvm/llvm-project/pull/143172.diff 10 Files Affected:
diff --git a/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake b/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake
index 699d3f8866861..66b964bf27b3f 100644
--- a/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake
+++ b/libcxx/cmake/caches/Generic-hardening-mode-fast-with-abi-breaks.cmake
@@ -1,7 +1,8 @@
set(LIBCXX_HARDENING_MODE "fast" CACHE STRING "")
set(_defines
- _LIBCPP_ABI_BOUNDED_ITERATORS
+ _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
+ _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW
_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
_LIBCPP_ABI_BOUNDED_UNIQUE_PTR
_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY
diff --git a/libcxx/docs/Hardening.rst b/libcxx/docs/Hardening.rst
index 17808841bd9ec..6d4e0883f6b1a 100644
--- a/libcxx/docs/Hardening.rst
+++ b/libcxx/docs/Hardening.rst
@@ -314,22 +314,28 @@ itself) to enable additional hardening checks. This is done by passing these
macros as ``-DLIBCXX_ABI_DEFINES="_LIBCPP_ABI_FOO;_LIBCPP_ABI_BAR;etc"`` at
CMake configuration time. The available options are:
-- ``_LIBCPP_ABI_BOUNDED_ITERATORS`` -- changes the iterator type of select
- containers (see below) to a bounded iterator that keeps track of whether it's
- within the bounds of the original container and asserts valid bounds on every
- dereference.
+- ``_LIBCPP_ABI_BOUNDED_ITERATORS`` -- historical equivalent to defining both
+ ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW`` and ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN``.
- ABI impact: changes the iterator type of the relevant containers.
+- ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW`` -- changes the iterator type of
+ ``basic_string_view`` to a bounded iterator that keeps track of whether it's within
+ the bounds of the original container and asserts it on every dereference and
+ when performing iterator arithmetic.
+
+ ABI impact: changes the iterator type of ``basic_string_view`` and its
+ specializations, such as ``string_view`` and ``wstring_view``.
- Supported containers:
+- ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN`` -- changes the iterator type of ``span``
+ to a bounded iterator that keeps track of whether it's within the bounds of the
+ original container and asserts it on every dereference and when performing iterator
+ arithmetic.
- - ``span``;
- - ``string_view``.
+ ABI impact: changes the iterator type of ``span``.
- ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING`` -- changes the iterator type of
``basic_string`` to a bounded iterator that keeps track of whether it's within
the bounds of the original container and asserts it on every dereference and
- when performing iterator arithmetics.
+ when performing iterator arithmetic.
ABI impact: changes the iterator type of ``basic_string`` and its
specializations, such as ``string`` and ``wstring``.
@@ -337,7 +343,7 @@ CMake configuration time. The available options are:
- ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR`` -- changes the iterator type of
``vector`` to a bounded iterator that keeps track of whether it's within the
bounds of the original container and asserts it on every dereference and when
- performing iterator arithmetics. Note: this doesn't yet affect
+ performing iterator arithmetic. Note: this doesn't yet affect
``vector<bool>``.
ABI impact: changes the iterator type of ``vector`` (except ``vector<bool>``).
diff --git a/libcxx/docs/ReleaseNotes/21.rst b/libcxx/docs/ReleaseNotes/21.rst
index 6cbc0baf29487..7d8fe07d7ca27 100644
--- a/libcxx/docs/ReleaseNotes/21.rst
+++ b/libcxx/docs/ReleaseNotes/21.rst
@@ -76,6 +76,10 @@ Improvements and New Features
- The ``bitset::to_string`` function has been optimized, resulting in a performance improvement of up to 8.3x for bitsets
with uniformly distributed zeros and ones, and up to 13.5x and 16.1x for sparse and dense bitsets, respectively.
+- The ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW`` and ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN`` macros were added.
+ These macros control bounded iterators in ``string_view`` and ``span`` respectively. This was previously controled by
+ the single macro ``_LIBCPP_ABI_BOUNDED_ITERATORS``.
+
Deprecations and Removals
-------------------------
diff --git a/libcxx/include/__configuration/abi.h b/libcxx/include/__configuration/abi.h
index cc4b930b3cf4a..f7ee77de5a786 100644
--- a/libcxx/include/__configuration/abi.h
+++ b/libcxx/include/__configuration/abi.h
@@ -145,24 +145,38 @@
// The macro below is used for all classes whose ABI have changed as part of fixing these bugs.
#define _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS __attribute__((__abi_tag__("llvm18_nua")))
-// Changes the iterator type of select containers (see below) to a bounded iterator that keeps track of whether it's
-// within the bounds of the original container and asserts it on every dereference.
+// Historical equivalent to defining `_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW` and
+// `_LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN`.
+#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifndef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW
+# define _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW
+# endif
+# ifndef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
+# define _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
+# endif
+#endif
+
+// Changes the iterator type of `basic_string_view` to a bounded iterator that keeps track of whether it's within the
+// bounds of the original container and asserts it on every dereference and when performing iterator arithmetic.
//
-// ABI impact: changes the iterator type of the relevant containers.
+// ABI impact: changes the iterator type of `basic_string_view` and its specializations, such as `string_view` and
+// `wstring_view`.
+// #define _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW
+
+// Changes the iterator type of `span` to a bounded iterator that keeps track of whether it's within the
+// bounds of the original container and asserts it on every dereference and when performing iterator arithmetic.
//
-// Supported containers:
-// - `span`;
-// - `string_view`.
-// #define _LIBCPP_ABI_BOUNDED_ITERATORS
+// ABI impact: changes the iterator type of `span`.
+// #define _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
// Changes the iterator type of `basic_string` to a bounded iterator that keeps track of whether it's within the bounds
-// of the original container and asserts it on every dereference and when performing iterator arithmetics.
+// of the original container and asserts it on every dereference and when performing iterator arithmetic.
//
// ABI impact: changes the iterator type of `basic_string` and its specializations, such as `string` and `wstring`.
// #define _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
// Changes the iterator type of `vector` to a bounded iterator that keeps track of whether it's within the bounds of the
-// original container and asserts it on every dereference and when performing iterator arithmetics. Note: this doesn't
+// original container and asserts it on every dereference and when performing iterator arithmetic. Note: this doesn't
// yet affect `vector<bool>`.
//
// ABI impact: changes the iterator type of `vector` (except `vector<bool>`).
diff --git a/libcxx/include/span b/libcxx/include/span
index 3d4f9e4ba7831..90afa278d0a70 100644
--- a/libcxx/include/span
+++ b/libcxx/include/span
@@ -240,7 +240,7 @@ public:
using const_pointer = const _Tp*;
using reference = _Tp&;
using const_reference = const _Tp&;
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
using iterator = __bounded_iter<pointer>;
# else
using iterator = __wrap_iter<pointer>;
@@ -383,14 +383,14 @@ public:
// [span.iter], span iterator support
_LIBCPP_HIDE_FROM_ABI constexpr iterator begin() const noexcept {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
return std::__make_bounded_iter(data(), data(), data() + size());
# else
return iterator(data());
# endif
}
_LIBCPP_HIDE_FROM_ABI constexpr iterator end() const noexcept {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
return std::__make_bounded_iter(data() + size(), data(), data() + size());
# else
return iterator(data() + size());
@@ -423,7 +423,7 @@ public:
using const_pointer = const _Tp*;
using reference = _Tp&;
using const_reference = const _Tp&;
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
using iterator = __bounded_iter<pointer>;
# else
using iterator = __wrap_iter<pointer>;
@@ -548,14 +548,14 @@ public:
// [span.iter], span iterator support
_LIBCPP_HIDE_FROM_ABI constexpr iterator begin() const noexcept {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
return std::__make_bounded_iter(data(), data(), data() + size());
# else
return iterator(data());
# endif
}
_LIBCPP_HIDE_FROM_ABI constexpr iterator end() const noexcept {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
return std::__make_bounded_iter(data() + size(), data(), data() + size());
# else
return iterator(data() + size());
diff --git a/libcxx/include/string_view b/libcxx/include/string_view
index 861187c0640e1..cad0e12538d04 100644
--- a/libcxx/include/string_view
+++ b/libcxx/include/string_view
@@ -287,7 +287,7 @@ public:
using const_pointer = const _CharT*;
using reference = _CharT&;
using const_reference = const _CharT&;
-# if defined(_LIBCPP_ABI_BOUNDED_ITERATORS)
+# if defined(_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW)
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>;
@@ -365,7 +365,7 @@ public:
_LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_iterator end() const _NOEXCEPT { return cend(); }
_LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_iterator cbegin() const _NOEXCEPT {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW
return std::__make_bounded_iter(data(), data(), data() + size());
# else
return const_iterator(__data_);
@@ -373,7 +373,7 @@ public:
}
_LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_iterator cend() const _NOEXCEPT {
-# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
+# ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW
return std::__make_bounded_iter(data() + size(), data(), data() + size());
# else
return const_iterator(__data_ + __size_);
diff --git a/libcxx/test/libcxx/bounded-iterator-macro.compile.pass.cpp b/libcxx/test/libcxx/bounded-iterator-macro.compile.pass.cpp
new file mode 100644
index 0000000000000..91a38d75081ca
--- /dev/null
+++ b/libcxx/test/libcxx/bounded-iterator-macro.compile.pass.cpp
@@ -0,0 +1,22 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// This test ensures that setting _LIBCPP_ABI_BOUNDED_ITERATORS enabled bounded
+// iterators in std::span and std::string_view, for historical reasons.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ABI_BOUNDED_ITERATORS
+
+#include <version>
+
+#ifndef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN
+# error _LIBCPP_ABI_BOUNDED_ITERATORS should enable bounded iterators in std::span
+#endif
+
+#ifndef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW
+# error _LIBCPP_ABI_BOUNDED_ITERATORS should enable bounded iterators in std::string_view
+#endif
diff --git a/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp b/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
index d4dacb1f2f1c7..1e4cfd3dcc096 100644
--- a/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
+++ b/libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
@@ -9,7 +9,7 @@
// Make sure that std::span's iterators check for OOB accesses when the debug mode is enabled.
-// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators
+// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators-in-span
// UNSUPPORTED: libcpp-hardening-mode=none
#include <span>
diff --git a/libcxx/test/libcxx/strings/string.view/string.view.iterators/assert.iterator-indexing.pass.cpp b/libcxx/test/libcxx/strings/string.view/string.view.iterators/assert.iterator-indexing.pass.cpp
index 5043a88cbc3da..c9eec9cbee30b 100644
--- a/libcxx/test/libcxx/strings/string.view/string.view.iterators/assert.iterator-indexing.pass.cpp
+++ b/libcxx/test/libcxx/strings/string.view/string.view.iterators/assert.iterator-indexing.pass.cpp
@@ -8,7 +8,7 @@
// Make sure that std::string_view's iterators check for OOB accesses when the debug mode is enabled.
-// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators
+// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators-in-string-view
// UNSUPPORTED: libcpp-hardening-mode=none
#include <iterator>
diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index 74746e37d3bc4..f5778fb78a898 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -365,8 +365,9 @@ def _mingwSupportsModules(cfg):
macros = {
"_LIBCPP_NO_VCRUNTIME": "libcpp-no-vcruntime",
"_LIBCPP_ABI_VERSION": "libcpp-abi-version",
- "_LIBCPP_ABI_BOUNDED_ITERATORS": "libcpp-has-abi-bounded-iterators",
"_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING": "libcpp-has-abi-bounded-iterators-in-string",
+ "_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING_VIEW": "libcpp-has-abi-bounded-iterators-in-string-view",
+ "_LIBCPP_ABI_BOUNDED_ITERATORS_IN_SPAN": "libcpp-has-abi-bounded-iterators-in-span",
"_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR": "libcpp-has-abi-bounded-iterators-in-vector",
"_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY": "libcpp-has-abi-bounded-iterators-in-std-array",
"_LIBCPP_ABI_BOUNDED_UNIQUE_PTR": "libcpp-has-abi-bounded-unique_ptr",
|
|
I think I'd rather go the other direction, i.e. merge all of the macros into a single one. I don't see why anybody would enable only some of the utilities in this list while ignoring others. Or is there any known configuration that is already ABI stable that doesn't follow this practice? |
The issue with merging all of these ABI macros into one is that we can't start hardening new iterators without making it an ABI break. It's also easier to roll out these changes one by one since these iterators can trigger new runtime failures. One thing we could do is actually keep the umbrella macro and document it as enabling all of the hardening, but I don't really see a downside in having the per-container macros. |
For consistency with other containers, introduce per-container macros to enable bounded iterators in span and string_view. This patch also keeps the original macro around for backwards compatibility, although we probably want to deprecate and eventually remove it in a separate patch since the name can lead to confusion.
ldionne
force-pushed
the
review/bounded-iterators-macro
branch
from
43f6163 to
564729f
Compare
The main downside I see is that it makes it a lot easier to have a very weird ABI configuration. I understand that there can be some roll-out challenges, but at this point I think it would be better to allow opting out of specific additions for a set period of time rather than having to opt in to every single one individually. |
There is a fundamental problem with bundling all of these ABI changes under the same setting. We can't claim that each ABI configuration gives rise to a stable ABI of its own and then plan to have ABI breaking changes from release to release when you enable one of these ABI configurations. That's just not a consistent model. Sure, we can have a pre-set that allows enabling all of the bounded iterators (as a shortcut), but that can't be the only way of enabling these. |
I don't think all the ABI macros should be ABI stable necessarily, and AFAIK we don't claim anywhere that all ABI macros in of themselves will be ABI stable. I'm rather on the opposite side: we shouldn't try to keep a macro ABI stable unless someone told us they rely on the stability of that macro. Otherwise we're potentially restricting ourselves in significant ways without a benefit to anybody. Is there any disagreement that in a perfect world there would be just two configurations throughout the library, namely bounded iterators and non-bounded iterators? |
For consistency with other containers, introduce per-container macros to enable bounded iterators in span and string_view. This patch also keeps the original macro around for backwards compatibility, although we probably want to deprecate and eventually remove it in a separate patch since the name can lead to confusion.