Skip to content

[libc++] Fix strict aliasing violation for deque::const_iterator #136067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

frederick-vs-ja
Copy link
Contributor

When the allocators use fancy pointers, the internal map of deque stores fancy_ptr<T> objects, and the previous strategy accessed these objects via const fancy_ptr<const T> lvalues, which usually caused core language undefined behavior. Now const_iterator stores fancy_ptr<const fancy_ptr<T>> instead of fancy_ptr<const fancy_ptr<const T>>, and ABI break can happen when such two types have incompatible layouts.

This is necessary for reducing undefined behavior and constexpr support for deque in C++26, and I currently don't want to provide any way to opt-out of that behavior.

Also removes redundant identity static_cast before and after type change.

The existing test coverage seems to be sufficient.

When the allocators use fancy pointers, the internal map of `deque`
stores `fancy_ptr<T>` objects, and the previous strategy accessed these
objects via `const fancy_ptr<const T>` lvalues, which usually caused
core language undefined behavior. Now `const_iterator` stores
`fancy_ptr<const fancy_ptr<T>>` instead of
`fancy_ptr<const fancy_ptr<const T>>`, and ABI break can happen when
such two types have incompatible layouts.

This is necessary for reducing undefined behavior and `constexpr`
support for `deque` in C++26, and I currently don't want to provide any
way to opt-out of that behavior.

Also removes redundant identity `static_cast` before and after type
change.

The existing test coverage seems to be sufficient.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner April 17, 2025 01:13
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 17, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 17, 2025

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

When the allocators use fancy pointers, the internal map of deque stores fancy_ptr&lt;T&gt; objects, and the previous strategy accessed these objects via const fancy_ptr&lt;const T&gt; lvalues, which usually caused core language undefined behavior. Now const_iterator stores fancy_ptr&lt;const fancy_ptr&lt;T&gt;&gt; instead of fancy_ptr&lt;const fancy_ptr&lt;const T&gt;&gt;, and ABI break can happen when such two types have incompatible layouts.

This is necessary for reducing undefined behavior and constexpr support for deque in C++26, and I currently don't want to provide any way to opt-out of that behavior.

Also removes redundant identity static_cast before and after type change.

The existing test coverage seems to be sufficient.


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

2 Files Affected:

  • (modified) libcxx/docs/ReleaseNotes/21.rst (+8)
  • (modified) libcxx/include/deque (+8-9)
diff --git a/libcxx/docs/ReleaseNotes/21.rst b/libcxx/docs/ReleaseNotes/21.rst
index 2091a713ea200..84d7d8cf7aab3 100644
--- a/libcxx/docs/ReleaseNotes/21.rst
+++ b/libcxx/docs/ReleaseNotes/21.rst
@@ -114,6 +114,14 @@ ABI Affecting Changes
   comparison between shared libraries, since all RTTI has the correct visibility now. There is no behaviour change on
   Clang.
 
+- The ``const_iterator`` member type of ``std::deque`` is now corrected to hold a (possibly fancy) pointer to the
+  (possibly fancy) pointer allocated in the internal map. E.g. when the allocators use fancy pointers, the internal map
+  stores ``fancy_ptr<T>`` objects, and the previous strategy accessed these objects via ``const fancy_ptr<const T>``
+  lvalues, which usually caused core language undefined behavior. Now ``const_iterator`` stores
+  ``fancy_ptr<const fancy_ptr<T>>`` instead of ``fancy_ptr<const fancy_ptr<const T>>``, and ABI break can happen when
+  such two types have incompatible layouts. This is necessary for reducing undefined behavior and ``constexpr`` support
+  for ``deque`` in C++26, so we do not provide any way to opt-out of that behavior.
+
 
 Build System Changes
 --------------------
diff --git a/libcxx/include/deque b/libcxx/include/deque
index e9846af5e5848..8df2a046e618d 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -504,13 +504,12 @@ public:
   using pointer       = typename __alloc_traits::pointer;
   using const_pointer = typename __alloc_traits::const_pointer;
 
-  using __pointer_allocator _LIBCPP_NODEBUG       = __rebind_alloc<__alloc_traits, pointer>;
-  using __const_pointer_allocator _LIBCPP_NODEBUG = __rebind_alloc<__alloc_traits, const_pointer>;
-  using __map _LIBCPP_NODEBUG                     = __split_buffer<pointer, __pointer_allocator>;
-  using __map_alloc_traits _LIBCPP_NODEBUG        = allocator_traits<__pointer_allocator>;
-  using __map_pointer _LIBCPP_NODEBUG             = typename __map_alloc_traits::pointer;
-  using __map_const_pointer _LIBCPP_NODEBUG       = typename allocator_traits<__const_pointer_allocator>::const_pointer;
-  using __map_const_iterator _LIBCPP_NODEBUG      = typename __map::const_iterator;
+  using __pointer_allocator _LIBCPP_NODEBUG  = __rebind_alloc<__alloc_traits, pointer>;
+  using __map _LIBCPP_NODEBUG                = __split_buffer<pointer, __pointer_allocator>;
+  using __map_alloc_traits _LIBCPP_NODEBUG   = allocator_traits<__pointer_allocator>;
+  using __map_pointer _LIBCPP_NODEBUG        = typename __map_alloc_traits::pointer;
+  using __map_const_pointer _LIBCPP_NODEBUG  = typename allocator_traits<__pointer_allocator>::const_pointer;
+  using __map_const_iterator _LIBCPP_NODEBUG = typename __map::const_iterator;
 
   using reference       = value_type&;
   using const_reference = const value_type&;
@@ -721,7 +720,7 @@ public:
   }
 
   _LIBCPP_HIDE_FROM_ABI const_iterator begin() const _NOEXCEPT {
-    __map_const_pointer __mp = static_cast<__map_const_pointer>(__map_.begin() + __start_ / __block_size);
+    __map_const_pointer __mp = __map_.begin() + __start_ / __block_size;
     return const_iterator(__mp, __map_.empty() ? 0 : *__mp + __start_ % __block_size);
   }
 
@@ -733,7 +732,7 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI const_iterator end() const _NOEXCEPT {
     size_type __p            = size() + __start_;
-    __map_const_pointer __mp = static_cast<__map_const_pointer>(__map_.begin() + __p / __block_size);
+    __map_const_pointer __mp = __map_.begin() + __p / __block_size;
     return const_iterator(__mp, __map_.empty() ? 0 : *__mp + __p % __block_size);
   }
 

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can do this as-is. AFAICT this changes the type of deque::const_iterator, which is way too big of an ABI break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants