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.

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

Fix the _Choose_pocca_v branch in basic_string::operator=(const&) #4031

Merged
merged 12 commits into from
Sep 21, 2023

Conversation

achabense
Copy link
Contributor

@achabense achabense commented Sep 15, 2023

Fixes #3997; also adds test coverages for non-trivial pointers.
---update---
Fixes another lifetime problem in _Take_contents. Hopefully fixes all the lifetime issue of _Ptr in basic_string. Possibly related to #3949.

Thanks to @frederick-vs-ja for suggesting adding test coverage for non-trivial pointers! Also thanks for implementing VSO_0000000_fancy_pointers, that test helps me a lot

---update (solved with workaround)---
The updated test meets another problem in static_assert(test_sequence<vector>)'s test_sequence_move_ctor... (see #4031 (comment))

@achabense achabense requested a review from a team as a code owner September 15, 2023 12:20
_Swap_proxy_and_iterators(_Right);

_Destroy_in_place(_Right_data._Bx._Ptr);
Copy link
Contributor Author

@achabense achabense Sep 15, 2023

Choose a reason for hiding this comment

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

_Tidy_init assumes that the pointer is already deactivated. (Perhaps we need more cleanups for the relevant logics in the future.)

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 15, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 15, 2023
@StephanTLavavej StephanTLavavej removed their assignment Sep 19, 2023
@@ -3052,8 +3052,9 @@ private:

if (_Right_data._Large_mode_engaged()) { // steal buffer
_Construct_in_place(_My_data._Bx._Ptr, _Right_data._Bx._Ptr);
_Right_data._Bx._Ptr = nullptr;
Copy link
Contributor Author

@achabense achabense Sep 20, 2023

Choose a reason for hiding this comment

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

emm, for clarity it might be better just to put the correction _Destroy_in_place(_Right_data._Bx._Ptr); at the original place.

Update: (Not planned in this pr)

_Destroy_in_place is typically followed by _Activate_SSO_buffer; however in this function, the _Activate_SSO_buffer call is put off to _Right._Tidy_init(); at the end. This suggests highly flaky maintenance state. I think we need to re-investigate the usages of _Activate_SSO_buffer / _Tidy_init / ... in the codebase.

Especially, I believe there is no need to call _Activate_SSO_buffer at basic_string's constructor, as the _String_val already initialized the buffer in its ctor. However, a lot of basic_string's ctor calls a _Tidy_init, which includes a _Activate_SSO_buffer.

_CONSTEXPR20 _Bxty() noexcept : _Buf() {} // user-provided, for fancy pointers

_Destroy_in_place is closely related to _Activate_SSO_buffer. When switching to SSO mode both function must be called in order. However there are several places that the they are dangerously separated into different function calls. Especially, I've found another place that lacks _Destroy_in_place. (in next comment)
In short, I think we should do _Activate_SSO_buffer and _Destroy_in_place only when switching back to SSO mode, and the two functions are so closely related that they should not be separated into different functions. As this pr shows, the separation can result in maintenance burden and has become a source of bugs.

@achabense
Copy link
Contributor Author

achabense commented Sep 20, 2023

(I think it's ok not to fix this in this pr)

STL/stl/inc/xstring

Lines 2963 to 2966 in 8f67ece

if (_My_data._Large_mode_engaged()) {
_Result._Ptr = _My_data._Bx._Ptr;
_Result._Actual_allocation_size = _My_data._Myres + 1;
} else {

It seems this place also lacks _Destroy_in_place. Not caught by the test as it is interaction part with basic_stringbuf.
(Notice that this function also calls _Tidy_init(); at the end.)
_Tidy_init();

@StephanTLavavej
Copy link
Member

The product code changes are very clear and the new test coverage provides sufficient confidence that this is correct; I think we can merge this without a second maintainer approval.

@StephanTLavavej StephanTLavavej self-assigned this Sep 20, 2023
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 69ceca3 into microsoft:main Sep 21, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for thoroughly fixing this bug! 🐞 🛠️ ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<xstring>: Fix basic_string::operator=(const&) thoroughly
4 participants