Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[libc++] Set correct size at the end of growing std::string

Authored by AdvenamTacet on Apr 19 2023, 1:56 AM.



This commit deprecates std::basic_string::__grow_by, which is part of ABIv1. The function is replaced by std::basic_string:__grow_by_without_replace, which is not part of ABI.

  • The original function __grow_by is deprecated because it does not set the string size, therefore it may not update the size when the size is changed, and it may also not set the size at all when the string was short initially. This leads to unpredictable size value. It is not removed or changed to avoid breaking the ABI.
  • The commit adds _LIBCPP_HIDE_FROM_ABI guarded by _LIBCPP_ABI_VERSION >= 2 to __grow_by. This allows the function to be used in the dylib in ABIv1 without raising the [abi:v170000] error and removes it from future ABIs. _LIBCPP_HIDE_FROM_ABI_AFTER_V1 cannot be used.
  • Additionally, __grow_by has been removed from _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST in libcxx/include/__string/extern_template_lists.h.

This bugfix is necessary to implement string ASan annotations, because it mitigates the problems encountered in D132769.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This update:

  • adds a new function __grow_by_without_replace,
  • replaces calls to __grow_by with the new function,
  • adds new names to ABI (is it correct?).

Should I change something?

philnik added inline comments.May 17 2023, 3:47 PM

This is unfortunately also not possible (at least without some policy changes). Fortunately it's also not necessary for this patch.


Let's remove this when we add __grow_by_without_replace.


We should mark this as _LIBCPP_DEPRECATED_("use __grow_by_without_replace") to avoid calling it accidentally.


This should be something like _LIBCPP_HIDE_FROM_ABI_V1 or similar.


Please clang-format!


The template arguments shouldn't be necessary.

AdvenamTacet added inline comments.May 17 2023, 9:17 PM

I don't understand.

philnik added inline comments.May 17 2023, 9:27 PM

Adding a new symbol to the dylib requires having availability annotations to aid in back-porting to older systems. If we add a new symbol in string, essentially all users would be required to target the latest OS when they use std::string in their programs, which isn't really an option.

AdvenamTacet marked 7 inline comments as done.

This update fixes issues mentioned in the code review:

  • adds _LIBCPP_DEPRECATED_("use __grow_by_without_replace"),
  • changes deprecated call to __grow_by in __grow_by_without_replace with a copy-paste code of the function.
  • in libcxx/include/__string/extern_template_lists.h removes references to __grow_by and __grow_by_without_replace,
  • clang-format, I hope this time I used the correct config!

I hope I understood all comments. I'm in the middle of testing right now.


Ok, I removed all references to __grow_by_without_replace.


Removed __grow_by from the file.


Cannot call deprecated function, so I had to copy-paste the code.



To the best of my knowledge, that implementation is correct.

  • The same value as in __grow_by_and_replace.
  • D132769 is working with that patch.
  • I tested it with more than unit tests.

The patch seems to be ready to land.

ldionne added inline comments.Jun 12 2023, 12:21 PM

We can't remove this function from the explicit instantiation list, since that would be an ABI break.


I would be tempted to make this _LIBCPP_HIDE_FROM_ABI and not include it in the explicit instantiation list. So far, our explicit instantiation lists have only made our lives really hard with std::string (e.g. getting in the way of fixing bugs).


Please document the bug that this function has and why we're keeping it around.

AdvenamTacet marked 2 inline comments as done.

As suggested, this update:

  • reverts changes in libcxx/include/__string/extern_template_lists.h,
  • in a comment, describes the bug and explains why the function is preserved,
  • adds _LIBCPP_HIDE_FROM_ABI to __grow_by_without_replace (),
  • removes changes in libcxx/lib/abi.

I hope, the patch may land after that change.

AdvenamTacet marked an inline comment as done.Jun 13 2023, 6:14 PM
philnik added inline comments.Jun 14 2023, 9:21 AM

You can use _LIBCPP_SUPPRESS_DEPRECATED_PUSH and _LIBCPP_SUPPRESS_DEPRECATED_POP to ignore the diagnostic. IMO that's also what we should do to avoid code bloat.


I don't really see the problem of adding this to the ABIv2 list. It's not like we will freeze ABIv2 any time soon, and we probably want to reassess what we export from the dylib and what we don't before doing that anyways.


It's not like we would care about the old behavior otherwise.

AdvenamTacet marked an inline comment as done.

This update improves a comment.

AdvenamTacet added inline comments.Jun 14 2023, 11:39 AM

Thx! I tried it, but __grow_by doesn't work because of _LIBCPP_HIDE_FROM_ABI_AFTER_V1:

undefined reference to `std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__grow_by[abi:v170000](unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long)'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

Is there one more work-around?

It works when I remove _LIBCPP_HIDE_FROM_ABI_AFTER_V1, but I believe it's worse solution than a few more copy-pasted lines.
Let me know what you think!


If you decide that I should revert the revert, let me know, I don't have an opinion about it. But also I don't see any real advantage of adding this function to ABI.

AdvenamTacet edited the summary of this revision. (Show Details)Jun 30 2023, 6:00 PM
ldionne accepted this revision.Jul 5 2023, 6:08 AM

LGTM with a suggestion to rephrase a comment.


Just rewording, IMO this is easier to read. WDYT?

AdvenamTacet marked an inline comment as done.
AdvenamTacet edited the summary of this revision. (Show Details)

This update:

  • applies comment change suggested by @ldionne (thank you!),
  • resolves conflicts.
AdvenamTacet marked an inline comment as not done.Jul 5 2023, 4:54 PM
AdvenamTacet added inline comments.

Hey @philnik, does it look ok for you? If yes, please, accept the review. If you want me to work on something, please, let me know how I should improve it.


Looks good, thank you! I applied that change.

A fix for rebase.

Removing std::__debug_db_invalidate_all(this); from __grow_by_without_replace as it was removed form __grow_by.

philnik added inline comments.Jul 10 2023, 12:57 PM

Where does that happen? The inline version should always be emitted, so there should never be a linker error with [abi:v170000]. IMO code duplication isn't the right thing here, since the code is quite complex and will get out of sync in the future.

To test the issue on CI, this update does:

  • rebase,

I hope I did it correctly.

AdvenamTacet added inline comments.Jul 10 2023, 8:45 PM

Hey @philnik, I recreated this error on CI. Let me know if I used _LIBCPP_SUPPRESS_DEPRECATED_POP correctly and if you know how to fix it. I agree that code duplication is far from ideal, but I don't know how to avoid it, keeping everything other requested. Possibly I'm just incorrectly using a macro.

philnik added inline comments.Jul 11 2023, 2:17 PM

Ah, I understand the problem now. I think we can just do

#if _LIBCPP_ABI_VERSION >= 2 // We want to use the function in the dylib in ABIv1

That should solve the problem.

Testing a potential fix.

This patch reverts previous attempt and is fixing the preoblem.

The solution is like suggested, using _LIBCPP_ABI_VERSION but around _LIBCPP_HIDE_FROM_ABI_AFTER_V1. Locally works.

philnik accepted this revision.Jul 12 2023, 10:22 PM

LGTM with green CI % nit.


This is already guarded; no need to make it more confusing.

This revision is now accepted and ready to land.Jul 12 2023, 10:22 PM
AdvenamTacet marked 4 inline comments as done.

This update:

  • adds a comment explaining why we guard _LIBCPP_HIDE_FROM_ABI,

I will land the patch, if CI stays green.

AdvenamTacet edited the summary of this revision. (Show Details)Jul 13 2023, 8:10 PM

This update removes __grow_by from _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST in libcxx/include/__string/extern_template_lists.h.

Hey @philnik, I made additional self-review and realized that the commit does not remove the function from _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST, so I fix it in this update. Unfortunatelly, _LIBCPP_HIDE_FROM_ABI_AFTER_V1 still does not work.

AdvenamTacet added inline comments.Jul 13 2023, 8:29 PM

Thank you for suggestions! It works!

But fact that we cannot use _LIBCPP_HIDE_FROM_ABI_AFTER_V1 made me ask a question, if we are doing everything correctly, which led to realization that I don't remove __grow_by function from ABIv2. I fixed it by removing it from _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST.

Unfortunately, _LIBCPP_HIDE_FROM_ABI_AFTER_V1 still does not work.
Now I believe everything it correct and ready to land. I will land it Tomorrow.