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

Lifetime cleanups for basic_string #4047

Merged
merged 9 commits into from Oct 14, 2023

Conversation

achabense
Copy link
Contributor

@achabense achabense commented Sep 22, 2023

Resolves the comments (1) (2) in pr 4031. Hope that is the last lifetime bug of basic_string using fancy pointers (3.1.).


Sometimes the pointer's destruction and buffer's activation are dangerously separated into different functions, in an obscure way (mainly relevant to the usage of _Tidy_init).
This pr introduces a helper function _String_val::_Bxty::_Switch_to_buf to make the transition state (where both _Ptr and _Buf are inactive) invisible to basic_string's ctors and methods.


Remaining issue after this pr:

STL/stl/inc/xstring

Lines 2282 to 2284 in 48eedd3

// This constructor previously initialized _Ptr. Don't rely on the new behavior without
// renaming `_String_val` (and fixing the visualizer).
_CONSTEXPR20 _Bxty() noexcept : _Buf() {} // user-provided, for fancy pointers

As _String_val already initializes the buffer on construction, and every switch back to small mode is now protected by the helper function, in basic_string's ctor and methods, when we can decide a string is in small mode, we can also assume that elements in _Buf are activated. Therefore, every remaining usage of _Activate_SSO_buffer (including the one in newly added _Construct_empty) becomes redundant...
Or, Don't rely on the new behavior? Does this mean we should not assume that _Buf is initialized in ctor, even if it does so now? Can we remove the remaining _Activate_SSO_buffer then? I'm highly horrified by this; especially, what's its relation with the lifetime issue of fancy pointers?

Related: #1735 #3235 and #3273.

@achabense achabense requested a review from a team as a code owner September 22, 2023 16:14
@github-actions github-actions bot added this to Initial Review in Code Reviews Sep 22, 2023
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 22, 2023
auto& _Al = _Getal();
_Destroy_in_place(_My_data._Bx._Ptr);
_My_data._Activate_SSO_buffer();
_Deallocate_for_capacity(_Al, _Ptr, _My_data._Myres);
}

Copy link
Contributor Author

@achabense achabense Sep 22, 2023

Choose a reason for hiding this comment

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

Slightly irrelevant; it also seems that these assignments in this function(_Tidy_deallocate) are actually useless, as _Tidy_deallocate seems always to be followed by methods that just rewrite these values. (based on current codebase; as like "remaining issue" part in the description, I'm uncertain about ABI impact of this change :(

        _My_data._Mysize = 0;
        _My_data._Myres  = _Small_string_capacity;
        // the _Traits::assign is last so the codegen doesn't think the char write can alias this
        _Traits::assign(_My_data._Bx._Buf[0], _Elem());

@StephanTLavavej StephanTLavavej self-assigned this Sep 22, 2023
@@ -946,6 +946,20 @@ _CONSTEXPR20 void test_string_swap(const size_t id1, const size_t id2) {
assert(dst.get_allocator().id() == id1);
}

#if _HAS_CXX20
Copy link
Contributor Author

@achabense achabense Sep 23, 2023

Choose a reason for hiding this comment

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

This doesn't have to be guarded by _HAS_CXX20, as strbuf(move(str)) will still call copy constructor in pre-cpp20 mode. However, I think adding _HAS_CXX20 will make it more obvious that the leak is relevant to basic_stringbuf(basic_string&&,...) introduced in C++20.

@github-actions github-actions bot moved this from Initial Review to Work In Progress in Code Reviews Sep 23, 2023
@github-actions github-actions bot moved this from Work In Progress to Initial Review in Code Reviews Sep 23, 2023
stl/inc/xstring Outdated Show resolved Hide resolved
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej removed their assignment Sep 28, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Sep 28, 2023
@StephanTLavavej
Copy link
Member

Thanks for these fine-grained commits, they made reviewing very straightforward! After thinking about each one, I believe that this PR has no ABI impact, and we should proceed as-is since it fixes the lifetime bug and generally makes it harder to introduce such mistakes in the future. Each commit is a transformation that doesn't introduce new inter-function assumptions (as far as I can tell).

This PR is definitely complicated enough and changing a highly-used, risky area of the codebase that we'll need a second maintainer signoff.

It definitely seems like any followup PR to remove _Activate_SSO_buffer needs to also rename _String_val, otherwise it'll be making the same ABI break as we suffered previously.

@CaseyCarter

This comment was marked as resolved.

1 similar comment
@CaseyCarter

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

1 similar comment
@azure-pipelines

This comment was marked as resolved.

@CaseyCarter CaseyCarter removed their assignment Oct 11, 2023
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Oct 11, 2023
@StephanTLavavej StephanTLavavej self-assigned this Oct 13, 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 8d6922a into microsoft:main Oct 14, 2023
69 checks passed
Code Reviews automation moved this from Ready To Merge to Done Oct 14, 2023
@StephanTLavavej
Copy link
Member

Thanks for making basic_string more maintainable! 🎉 😻 🧶

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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants