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

xmas92 · 2020-10-02T09:24:16Z

Notes
Have not tested if it affects wait, notify_one and notify_all. Not that it should, but I am not well versed in the smart pointer implementations.

Same tests as for the normal atomic smart pointer with non-array element. The array pointers should behave exactly the same as the normal pointers.

Fix atomic smart pointers with array elements by making the internal type match that of [weak|shared]_ptr::element_type by removing the extent of the arrays.

ghost · 2020-10-02T09:24:31Z

All CLA requirements met.

tests/std/tests/P0718R2_atomic_smart_ptrs/test.cpp

StephanTLavavej · 2020-10-02T10:55:52Z

Thanks! Should there be test coverage for multidimensional arrays? (This is the first thing I think of when I see remove_extent_t.) It doesn't have to be as thorough as the single-dimensional array coverage, just enough to indicate that this is the correct type trait to use.

Add a test that checks that the atomic smart pointers with multidimensional arrays compiles.

xmas92 · 2020-10-02T11:50:18Z

Added a compile test for multidimensional arrays (member and non-member functions). If you also want to test the functionality I think the best thing is to just repeat the original test for the different types. Could refactor it into template functions.

As originally noted the test do not touch the wait/notify mechanism. Not even if it compiles.
Is there some other test that test these and could be used to check that it still works for array types.

Otherwise I could not think of a simple way to test whether it compiles except writing some producer/consumer type of functions which use wait/notify.

AlexGuteniev · 2020-11-26T07:17:55Z

STL/tests/std/tests/P1135R6_atomic_wait/test.cpp is where atomic wait is tested

Update P1135R6_atomic_wait test to include smart pointers holding arrays.

…_ptr Increase waiting_duration timing assumption for test_notify_all_notifies_all_ptr because it might have cause some flakiness for the test runners.

xmas92 · 2021-10-17T13:32:42Z

I noticed that this issue[#1332] was still not fixed and had some time over so added test for smart pointer array types in tests/P1135R6_atomic_wait.

I used the already created, but unused, test_notify_all_notifies_all_ptr to test atomic shared pointers. But had to increase the timing assumption as the original seem to cause deadlocks in at least two test runners.

Maybe it should be a separate low priority issue to rewrite tests/P1135R6_atomic_wait so that deadlocks are impossible. The current implementation relies on a timing assumption of a data race to avoid deadlocks. Maybe someone have better statistics for the flakiness of these tests.

As for #1332, what is required to close this issue?

strega-nil-ms

Notes:

This in theory changes ABI of _Atomic_ptr_base; however, it only changes name manglings of non-public names, and changes the members of _Atomic_ptr_base<T[]> from:

{
    atomic<T(*)[]> _Ptr;
    _Locked_pointer<_Ref_count_base> _Repptr;
}

to

{
    atomic<T*> _Ptr;
    _Locked_pointer<_Ref_count_base> _Repptr;
}

which should not change ABI/layout at all.

strega-nil-ms

_Seed should not be a template, since there's no reason for it to be a template. Additionally, an existing, correct, narrowing conversion now fails.

Bug in VS code ended up making a comment meant for 2693 on this PR

StephanTLavavej · 2022-05-05T02:26:36Z

tests/std/include/test_atomic_wait.hpp

+    // 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);


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.

StephanTLavavej · 2022-05-05T02:32:08Z

tests/std/include/test_atomic_wait.hpp

+    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);


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

StephanTLavavej · 2022-05-05T02:38:25Z

stl/inc/memory

@@ -3872,7 +3873,7 @@ protected:
        _Ptr.notify_all();
    }

-    atomic<_Ty*> _Ptr{nullptr};
+    atomic<remove_extent_t<_Ty>*> _Ptr{nullptr};


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.)

StephanTLavavej · 2022-05-05T02:40:58Z

tests/std/include/test_atomic_wait.hpp

@@ -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);


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).

StephanTLavavej · 2022-05-05T02:54:44Z

tests/std/tests/P0718R2_atomic_smart_ptrs/test.cpp

+    }
+    if (instance.compare_exchange_strong(loaded, constInstance)) {


No change requested: We conventionally add newlines between non-chained if-statements. However, this was copied from pre-existing code immediately above, and it's not worth resetting testing by pushing a commit.

StephanTLavavej · 2022-05-05T03:04:21Z

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej · 2022-05-05T08:58:40Z

Thanks for fixing this bug, and congratulations on your first microsoft/STL commit! 🎉 😸 🚀

(And apologies for taking so long to review this! 🙀)

xmas92 added 2 commits October 1, 2020 12:51

Add test cases

975ac24

Same tests as for the normal atomic smart pointer with non-array element. The array pointers should behave exactly the same as the normal pointers.

Fix atomic smart pointers with array elements

0fd8850

Fix atomic smart pointers with array elements by making the internal type match that of [weak|shared]_ptr::element_type by removing the extent of the arrays.

xmas92 requested a review from a team as a code owner October 2, 2020 09:24

miscco reviewed Oct 2, 2020

View reviewed changes

tests/std/tests/P0718R2_atomic_smart_ptrs/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/P0718R2_atomic_smart_ptrs/test.cpp Outdated Show resolved Hide resolved

StephanTLavavej added the bug Something isn't working label Oct 2, 2020

xmas92 added 2 commits October 2, 2020 13:09

Remove extra newline

6fa66d0

Add compile test for multidimensional array

3946766

Add a test that checks that the atomic smart pointers with multidimensional arrays compiles.

StephanTLavavej self-assigned this Oct 7, 2020

This comment has been minimized.

Sign in to view

Base automatically changed from master to main January 28, 2021 00:35

xmas92 added 3 commits October 17, 2021 07:10

Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray

f452d2d

Add wait/notify tests

b078b52

Update P1135R6_atomic_wait test to include smart pointers holding arrays.

Increase waiting_duration assumption for test_notify_all_notifies_all…

8a2e4bb

…_ptr Increase waiting_duration timing assumption for test_notify_all_notifies_all_ptr because it might have cause some flakiness for the test runners.

xmas92 added 4 commits November 7, 2021 23:41

Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray

50cca4e

Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray

0f3da1f

Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray

dccfdc1

Merge branch 'microsoft:main' into atomic-shared_ptr-Tarray

44ef6a8

strega-nil-ms self-assigned this May 3, 2022

Merge remote-tracking branch 'origin/main' into pr/xmas92/1339

af7874f

strega-nil-ms approved these changes May 3, 2022

View reviewed changes

StephanTLavavej unassigned strega-nil-ms May 4, 2022

strega-nil-ms previously requested changes May 4, 2022

View reviewed changes

strega-nil-ms approved these changes May 4, 2022

View reviewed changes

StephanTLavavej approved these changes May 5, 2022

View reviewed changes

StephanTLavavej removed their assignment May 5, 2022

StephanTLavavej self-assigned this May 5, 2022

StephanTLavavej merged commit 24b899e into microsoft:main May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xmas92 commented Oct 2, 2020

ghost commented Oct 2, 2020 •

edited by ghost

Loading

StephanTLavavej commented Oct 2, 2020

xmas92 commented Oct 2, 2020

This comment has been minimized.

This comment has been minimized.

AlexGuteniev commented Nov 26, 2020

xmas92 commented Oct 17, 2021

strega-nil-ms left a comment

strega-nil-ms left a comment

StephanTLavavej May 5, 2022

StephanTLavavej May 5, 2022

StephanTLavavej May 5, 2022

StephanTLavavej May 5, 2022

StephanTLavavej May 5, 2022

StephanTLavavej commented May 5, 2022

StephanTLavavej commented May 5, 2022

		// 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);

		}
		if (instance.compare_exchange_strong(loaded, constInstance)) {

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

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

Conversation

xmas92 commented Oct 2, 2020

ghost commented Oct 2, 2020 • edited by ghost Loading

StephanTLavavej commented Oct 2, 2020

xmas92 commented Oct 2, 2020

This comment has been minimized.

This comment has been minimized.

AlexGuteniev commented Nov 26, 2020

xmas92 commented Oct 17, 2021

strega-nil-ms left a comment

Choose a reason for hiding this comment

strega-nil-ms left a comment

Choose a reason for hiding this comment

StephanTLavavej May 5, 2022

Choose a reason for hiding this comment

StephanTLavavej May 5, 2022

Choose a reason for hiding this comment

StephanTLavavej May 5, 2022

Choose a reason for hiding this comment

StephanTLavavej May 5, 2022

Choose a reason for hiding this comment

StephanTLavavej May 5, 2022

Choose a reason for hiding this comment

StephanTLavavej commented May 5, 2022

StephanTLavavej commented May 5, 2022

ghost commented Oct 2, 2020 •

edited by ghost

Loading