Skip to content
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

<memory>: Fix atomic smart pointers array type interaction #1339

Merged
merged 12 commits into from
May 5, 2022
Merged

<memory>: Fix atomic smart pointers array type interaction #1339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
975ac24
Add test cases
xmas92 Oct 1, 2020
0fd8850
Fix atomic smart pointers with array elements
xmas92 Oct 1, 2020
6fa66d0
Remove extra newline
xmas92 Oct 2, 2020
3946766
Add compile test for multidimensional array
xmas92 Oct 2, 2020
f452d2d
Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray
xmas92 Oct 17, 2021
b078b52
Add wait/notify tests
xmas92 Oct 17, 2021
8a2e4bb
Increase waiting_duration assumption for test_notify_all_notifies_all…
xmas92 Oct 17, 2021
50cca4e
Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray
xmas92 Nov 7, 2021
0f3da1f
Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray
xmas92 Nov 17, 2021
dccfdc1
Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray
xmas92 Dec 16, 2021
44ef6a8
Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray
xmas92 Jan 2, 2022
af7874f
Merge remote-tracking branch 'origin/main' into pr/xmas92/1339
strega-nil May 3, 2022
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
27 changes: 14 additions & 13 deletions stl/inc/memory
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3850,9 +3850,10 @@ class alignas(2 * sizeof(void*)) _Atomic_ptr_base {
protected:
constexpr _Atomic_ptr_base() noexcept = default;

_Atomic_ptr_base(_Ty* const _Px, _Ref_count_base* const _Ref) noexcept : _Ptr(_Px), _Repptr(_Ref) {}
_Atomic_ptr_base(remove_extent_t<_Ty>* const _Px, _Ref_count_base* const _Ref) noexcept
: _Ptr(_Px), _Repptr(_Ref) {}

void _Wait(_Ty* _Old, memory_order) const noexcept {
void _Wait(remove_extent_t<_Ty>* _Old, memory_order) const noexcept {
for (;;) {
auto _Rep = _Repptr._Lock_and_load();
bool _Equal = _Ptr.load(memory_order_relaxed) == _Old;
Expand All @@ -3872,7 +3873,7 @@ protected:
_Ptr.notify_all();
}

atomic<_Ty*> _Ptr{nullptr};
atomic<remove_extent_t<_Ty>*> _Ptr{nullptr};
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: Regarding the ABI implications that @strega-nil-ms mentioned in #1339 (review) - while changing data members is always concerning from an ABI perspective, I agree that the actual bits being stored are the same. Additionally, the previous code would generally fail to compile for simple cases like store(), which indicates that we shouldn't have to worry about mismatch issues. (The one time we can make unrestricted changes to layout is when the code couldn't compile at all before - this is not quite "wouldn't compile at all" but close.)

mutable _Locked_pointer<_Ref_count_base> _Repptr;
};

Expand All @@ -3892,9 +3893,9 @@ public:

void store(shared_ptr<_Ty> _Value, const memory_order _Order = memory_order_seq_cst) noexcept {
_Check_store_memory_order(_Order);
const auto _Rep = this->_Repptr._Lock_and_load();
_Ty* const _Tmp = _Value._Ptr;
_Value._Ptr = this->_Ptr.load(memory_order_relaxed);
const auto _Rep = this->_Repptr._Lock_and_load();
remove_extent_t<_Ty>* const _Tmp = _Value._Ptr;
_Value._Ptr = this->_Ptr.load(memory_order_relaxed);
this->_Ptr.store(_Tmp, memory_order_relaxed);
this->_Repptr._Store_and_unlock(_Value._Rep);
_Value._Rep = _Rep;
Expand Down Expand Up @@ -3947,8 +3948,8 @@ public:
_Check_memory_order(_Order);
auto _Rep = this->_Repptr._Lock_and_load();
if (this->_Ptr.load(memory_order_relaxed) == _Expected._Ptr && _Rep == _Expected._Rep) {
_Ty* const _Tmp = _Desired._Ptr;
_Desired._Ptr = this->_Ptr.load(memory_order_relaxed);
remove_extent_t<_Ty>* const _Tmp = _Desired._Ptr;
_Desired._Ptr = this->_Ptr.load(memory_order_relaxed);
this->_Ptr.store(_Tmp, memory_order_relaxed);
_STD swap(_Rep, _Desired._Rep);
this->_Repptr._Store_and_unlock(_Rep);
Expand Down Expand Up @@ -4011,9 +4012,9 @@ public:

void store(weak_ptr<_Ty> _Value, const memory_order _Order = memory_order_seq_cst) noexcept {
_Check_store_memory_order(_Order);
const auto _Rep = this->_Repptr._Lock_and_load();
_Ty* const _Tmp = _Value._Ptr;
_Value._Ptr = this->_Ptr.load(memory_order_relaxed);
const auto _Rep = this->_Repptr._Lock_and_load();
remove_extent_t<_Ty>* const _Tmp = _Value._Ptr;
_Value._Ptr = this->_Ptr.load(memory_order_relaxed);
this->_Ptr.store(_Tmp, memory_order_relaxed);
this->_Repptr._Store_and_unlock(_Value._Rep);
_Value._Rep = _Rep;
Expand Down Expand Up @@ -4066,8 +4067,8 @@ public:
_Check_memory_order(_Order);
auto _Rep = this->_Repptr._Lock_and_load();
if (this->_Ptr.load(memory_order_relaxed) == _Expected._Ptr && _Rep == _Expected._Rep) {
_Ty* const _Tmp = _Desired._Ptr;
_Desired._Ptr = this->_Ptr.load(memory_order_relaxed);
remove_extent_t<_Ty>* const _Tmp = _Desired._Ptr;
_Desired._Ptr = this->_Ptr.load(memory_order_relaxed);
this->_Ptr.store(_Tmp, memory_order_relaxed);
_STD swap(_Rep, _Desired._Rep);
this->_Repptr._Store_and_unlock(_Rep);
Expand Down
37 changes: 36 additions & 1 deletion tests/std/include/test_atomic_wait.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ void test_notify_all_notifies_all(UnderlyingType old_value, const UnderlyingType
template <class UnderlyingType>
void test_notify_all_notifies_all_ptr(UnderlyingType old_value, const UnderlyingType new_value,
const std::chrono::steady_clock::duration waiting_duration) {
test_notify_all_notifies_all_impl<std::atomic, UnderlyingType>(old_value, new_value, waiting_duration);
// increased waiting_duration because timing assumption might not hold for atomic smart pointers
test_notify_all_notifies_all_impl<std::atomic, UnderlyingType>(old_value, new_value, 3 * waiting_duration);
Comment on lines +111 to +112
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly concerned about this change because the test suite should never contain timing assumptions that could lead to sporadic failures. That said, no change requested, because this is clearly a pre-existing issue.

}


Expand Down Expand Up @@ -228,6 +229,21 @@ inline void test_atomic_wait() {
test_atomic_wait_func_ptr(std::make_shared<int>('a'), std::make_shared<int>('a'), waiting_duration);
test_atomic_wait_func_ptr(
std::weak_ptr{std::make_shared<int>('a')}, std::weak_ptr{std::make_shared<int>('a')}, waiting_duration);
test_atomic_wait_func_ptr(std::make_shared<int[]>(0), std::make_shared<int[]>(0), waiting_duration);
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: I note that the zero-runtime-length arrays being used in the test cases are unusual; however, I believe this is conforming, and the code also tests one-element arrays, so I have no objection to this (even if I would have used non-zero numbers for all).

test_atomic_wait_func_ptr(
std::weak_ptr{std::make_shared<int[]>(0)}, std::weak_ptr{std::make_shared<int[]>(0)}, waiting_duration);
test_atomic_wait_func_ptr(std::make_shared<int[]>(1), std::make_shared<int[]>(1), waiting_duration);
test_atomic_wait_func_ptr(
std::weak_ptr{std::make_shared<int[]>(1)}, std::weak_ptr{std::make_shared<int[]>(1)}, waiting_duration);
test_atomic_wait_func_ptr(std::make_shared<int[2]>(), std::make_shared<int[2]>(), waiting_duration);
test_atomic_wait_func_ptr(
std::weak_ptr{std::make_shared<int[2]>()}, std::weak_ptr{std::make_shared<int[2]>()}, waiting_duration);
test_atomic_wait_func_ptr(std::make_shared<int[][2]>(2), std::make_shared<int[][2]>(2), waiting_duration);
test_atomic_wait_func_ptr(
std::weak_ptr{std::make_shared<int[][2]>(2)}, std::weak_ptr{std::make_shared<int[][2]>(2)}, waiting_duration);
test_atomic_wait_func_ptr(std::make_shared<int[2][2]>(), std::make_shared<int[2][2]>(), waiting_duration);
test_atomic_wait_func_ptr(
std::weak_ptr{std::make_shared<int[2][2]>()}, std::weak_ptr{std::make_shared<int[2][2]>()}, waiting_duration);

test_notify_all_notifies_all<char>(1, 2, waiting_duration);
test_notify_all_notifies_all<signed char>(1, 2, waiting_duration);
Expand All @@ -248,6 +264,25 @@ inline void test_atomic_wait() {
test_notify_all_notifies_all(three_chars{1, 1, 3}, three_chars{1, 2, 3}, waiting_duration);
test_notify_all_notifies_all(big_char_like{'a'}, big_char_like{'b'}, waiting_duration);

test_notify_all_notifies_all_ptr(std::make_shared<int>('a'), std::make_shared<int>('a'), waiting_duration);
test_notify_all_notifies_all_ptr(
std::weak_ptr{std::make_shared<int>('a')}, std::weak_ptr{std::make_shared<int>('a')}, waiting_duration);
Comment on lines +267 to +269
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: Nice catch, test_notify_all_notifies_all_ptr() was defined but never used before.

test_notify_all_notifies_all_ptr(std::make_shared<int[]>(0), std::make_shared<int[]>(0), waiting_duration);
test_notify_all_notifies_all_ptr(
std::weak_ptr{std::make_shared<int[]>(0)}, std::weak_ptr{std::make_shared<int[]>(0)}, waiting_duration);
test_notify_all_notifies_all_ptr(std::make_shared<int[]>(1), std::make_shared<int[]>(1), waiting_duration);
test_notify_all_notifies_all_ptr(
std::weak_ptr{std::make_shared<int[]>(1)}, std::weak_ptr{std::make_shared<int[]>(1)}, waiting_duration);
test_notify_all_notifies_all_ptr(std::make_shared<int[2]>(), std::make_shared<int[2]>(), waiting_duration);
test_notify_all_notifies_all_ptr(
std::weak_ptr{std::make_shared<int[2]>()}, std::weak_ptr{std::make_shared<int[2]>()}, waiting_duration);
test_notify_all_notifies_all_ptr(std::make_shared<int[][2]>(2), std::make_shared<int[][2]>(2), waiting_duration);
test_notify_all_notifies_all_ptr(
std::weak_ptr{std::make_shared<int[][2]>(2)}, std::weak_ptr{std::make_shared<int[][2]>(2)}, waiting_duration);
test_notify_all_notifies_all_ptr(std::make_shared<int[2][2]>(), std::make_shared<int[2][2]>(), waiting_duration);
test_notify_all_notifies_all_ptr(
std::weak_ptr{std::make_shared<int[2][2]>()}, std::weak_ptr{std::make_shared<int[2][2]>()}, waiting_duration);

#ifndef __clang__ // TRANSITION, LLVM-46685
test_pad_bits<with_padding_bits<2>>(waiting_duration);
test_pad_bits<with_padding_bits<4>>(waiting_duration);
Expand Down
Loading