Skip to content

[libc++] Add assumption for align of begin and end pointers of vector. #108961

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 16, 2025
Merged

[libc++] Add assumption for align of begin and end pointers of vector. #108961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94c2ede
[libc++] Add assumption for align of begin and end pointers of vector.
fhahn Sep 17, 2024
3daa6c5
Address review comments
ldionne Jan 13, 2025
ddb387b
Handle C++03
ldionne Jan 13, 2025
a2cd12c
Add missing include
ldionne Jan 13, 2025
a9fb6aa
Avoid instantiating vector::begin() when declaring flat_map
ldionne Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions libcxx/include/__flat_map/key_value_iterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
#include <__config>
Copy link
Member

Choose a reason for hiding this comment

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

@huixie90 Do these changes look correct to you? Is there a reason why you used ranges::iterator_t in the first place?

#include <__iterator/iterator_traits.h>
#include <__memory/addressof.h>
#include <__ranges/access.h>
#include <__type_traits/conditional.h>
#include <__type_traits/maybe_const.h>
#include <__utility/move.h>
#include <__utility/pair.h>

Expand All @@ -41,9 +39,10 @@ _LIBCPP_BEGIN_NAMESPACE_STD
template <class _Owner, class _KeyContainer, class _MappedContainer, bool _Const>
struct __key_value_iterator {
private:
using __key_iterator _LIBCPP_NODEBUG = ranges::iterator_t<const _KeyContainer>;
using __mapped_iterator _LIBCPP_NODEBUG = ranges::iterator_t<__maybe_const<_Const, _MappedContainer>>;
using __reference _LIBCPP_NODEBUG = _If<_Const, typename _Owner::const_reference, typename _Owner::reference>;
using __key_iterator _LIBCPP_NODEBUG = typename _KeyContainer::const_iterator;
using __mapped_iterator _LIBCPP_NODEBUG =
_If<_Const, typename _MappedContainer::const_iterator, typename _MappedContainer::iterator>;
using __reference _LIBCPP_NODEBUG = _If<_Const, typename _Owner::const_reference, typename _Owner::reference>;

struct __arrow_proxy {
__reference __ref_;
Expand Down Expand Up @@ -71,8 +70,8 @@ struct __key_value_iterator {
_LIBCPP_HIDE_FROM_ABI __key_value_iterator() = default;

_LIBCPP_HIDE_FROM_ABI __key_value_iterator(__key_value_iterator<_Owner, _KeyContainer, _MappedContainer, !_Const> __i)
requires _Const && convertible_to<ranges::iterator_t<_KeyContainer>, __key_iterator> &&
convertible_to<ranges::iterator_t<_MappedContainer>, __mapped_iterator>
requires _Const && convertible_to<typename _KeyContainer::iterator, __key_iterator> &&
convertible_to<typename _MappedContainer::iterator, __mapped_iterator>
: __key_iter_(std::move(__i.__key_iter_)), __mapped_iter_(std::move(__i.__mapped_iter_)) {}

_LIBCPP_HIDE_FROM_ABI __key_value_iterator(__key_iterator __key_iter, __mapped_iterator __mapped_iter)
Expand Down
24 changes: 20 additions & 4 deletions libcxx/include/__vector/vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
#include <__type_traits/is_constructible.h>
#include <__type_traits/is_nothrow_assignable.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_pointer.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_trivially_relocatable.h>
#include <__type_traits/type_identity.h>
Expand Down Expand Up @@ -341,13 +342,17 @@ class _LIBCPP_TEMPLATE_VIS vector {
//
// Iterators
//
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator begin() _NOEXCEPT { return __make_iter(this->__begin_); }
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator begin() _NOEXCEPT {
return __make_iter(__add_alignment_assumption(this->__begin_));
}
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator begin() const _NOEXCEPT {
return __make_iter(this->__begin_);
return __make_iter(__add_alignment_assumption(this->__begin_));
}
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator end() _NOEXCEPT {
return __make_iter(__add_alignment_assumption(this->__end_));
}
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator end() _NOEXCEPT { return __make_iter(this->__end_); }
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator end() const _NOEXCEPT {
return __make_iter(this->__end_);
return __make_iter(__add_alignment_assumption(this->__end_));
}

_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reverse_iterator rbegin() _NOEXCEPT {
Expand Down Expand Up @@ -775,6 +780,17 @@ class _LIBCPP_TEMPLATE_VIS vector {
}

_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __move_assign_alloc(vector&, false_type) _NOEXCEPT {}

static _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI pointer __add_alignment_assumption(pointer __p) _NOEXCEPT {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly to #118837 (review) I think this one needs _LIBCPP_NO_CFI since we're potentially static_cast'ing uninitialized memory (for the end pointer).

(Apologies for the late notice, we're a bit behind on libc++.)

Copy link
Member

@ldionne ldionne Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ack. I'd be curious to see how this failure can be reproduced. Is it just a matter of adding a -fsanitize=cfi job to our pre-commit CI?

CF #124837

Copy link
Member

Choose a reason for hiding this comment

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

Also #124839 for adding NO_CFI

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't tried to reproduce it on the libc++ tests directly, but it should be possible. I think we're currently only using a subset of the cfi checks: -fsanitize=cfi-vcall -fsanitize=cfi-derived-cast -fsanitize=cfi-unrelated-cast. The annoying thing is that cfi requires (thin) lto.

#ifndef _LIBCPP_CXX03_LANG
if constexpr (is_pointer<pointer>::value) {
Comment on lines +785 to +786
Copy link
Contributor

Choose a reason for hiding this comment

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

@ldionne Are we using constexpr if in C++11/14 as a compiler extension here? I've also noticed several other usages of constexpr if in C++14 mode in libc++.
Does this imply that we can generally use constexpr if within the context of C++11/14 in libc++?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's fine to use compiler extensions in the library as long as our supported compilers implement them.

if (!__libcpp_is_constant_evaluated()) {
return static_cast<pointer>(__builtin_assume_aligned(__p, alignof(decltype(*__p))));
}
}
#endif
return __p;
}
};

#if _LIBCPP_STD_VER >= 17
Expand Down
Loading