[libc++] Fix wrongly non-inline basic_string::shrink_to_fit

[ldionne]

Summary

As explained in https://stackoverflow.com/a/70339311/627587, the fact
that shrink_to_fit wasn't defined as inline lead to issues when explicitly
instantiating basic_string. While explicit instantiations are always
somewhat brittle, this one was clearly a bug on our end.

[• Quuxplusone]

libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.explicit_instantiation.sh.cpp

13

FYI, this link isn't reachable from outside Apple. (What ever happened to the rdar:// URL scheme, anyway? :))

[Mordante]

libcxx/include/string

3287

Just curious but there are more similar functions not marked as inline. Are they also an issue and should they be fixed?
It would be odd if all these functions are "broken".

I wonder whether this is really the proper fix. Looking at include/__string it contains
_Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::reserve(size_type)) \
But no entry for shrink_to_fit.
Does adding shrink_to_fit to that list solve the issue.

(I didn't look for more omissions in include/__string.)

[ldionne]

libcxx/include/string

3287

Just curious but there are more similar functions not marked as inline. Are they also an issue and should they be fixed?

These other non-inline functions are not marked with _LIBCPP_HIDE_FROM_ABI or _LIBCPP_INLINE_VISIBILITY AFAICT. Did you see some that were not inline but had _LIBCPP_HIDE_FROM_ABI?

I wonder whether this is really the proper fix. Looking at include/__string it contains

_Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::reserve(size_type)) \

But no entry for shrink_to_fit.

reserve is different, it is not marked with _LIBCPP_HIDE_FROM_ABI, so it doesn't get internal linkage. If we made it _LIBCPP_HIDE_FROM_ABI, we'd have the same issue.

Does adding shrink_to_fit to that list solve the issue?

No it doesn't, since we'd be explicitly instantiating a function with internal linkage, so it wouldn't be available from outside the string.cpp.o TU. And these lists are used only for the char and wchar_t explicit instantiations anyways, so that wouldn't solve Chromium's issue, which is with their own custom char16 type.

libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.explicit_instantiation.sh.cpp

13

Hmm, I was pretty sure this was publicly accessible if you have a developer account? This is supposed to be the public version of the radar link, which is private.

I'd be keen to leave the link there since I think it should be accessible if you have an Apple developer account. If not, it will probably become accessible after the Xcode open beta is done. In all cases, it should be no less accessible than a rdar:// link.

[ldionne]

Switch object files and link flags to fix GCC. GCC's linker is the most ridiculous thing.

[ldionne]

Cherry-picking this to release/13.x as f3394dc82c20feea943293eda162b5aff2930ac9 since this is affecting open source software.

[Mordante]

libcxx/include/string

3287

I see.
The current requirements for when using inline and when not feel quite error-prone.
I wonder whether we need to add a better way to tag these functions. WDYT?

[ldionne]

libcxx/include/string

3287

Indeed -- it's completely crazy.

IMO, we want all functions to be inline except when they are explicitly defined in and exported from the shared library. We've been moving towards that by defining functions in their class declarations more and more, and we should slowly keep moving towards that.

Instead of using internal_linkage, we could also look into using an inline namespace that gets bumped with every version of the library. That would reduce code bloat (due to internal_linkage functions being duplicated in each TU) and remove the need for using _LIBCPP_HIDE_FROM_ABI everywhere.

libcxx/include/string

 #ifdef _LIBCPP_BIG_ENDIAN
     return (__m <= ~__long_mask ? __m : __m/2) - __alignment;
 #else
     return __m - __alignment;
 #endif
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 void
 basic_string<_CharT, _Traits, _Allocator>::reserve(size_type __requested_capacity)
 {
     if (__requested_capacity > max_size())
         this->__throw_length_error();
 
 #if _LIBCPP_STD_VER > 17
     // Reserve never shrinks as of C++20.
     if (__requested_capacity <= capacity()) return;
 #endif
 
     size_type __target_capacity = _VSTD::max(__requested_capacity, size());
     __target_capacity = __recommend(__target_capacity);
     if (__target_capacity == capacity()) return;
 
     __shrink_or_extend(__target_capacity);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
+inline
 void
 basic_string<_CharT, _Traits, _Allocator>::shrink_to_fit() _NOEXCEPT
 {
     size_type __target_capacity = __recommend(size());
     if (__target_capacity == capacity()) return;
 
     __shrink_or_extend(__target_capacity);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
+inline
 void
 basic_string<_CharT, _Traits, _Allocator>::__shrink_or_extend(size_type __target_capacity)
 {
     size_type __cap = capacity();
     size_type __sz = size();
 
     pointer __new_data, __p;
     bool __was_long, __now_long;

libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.explicit_instantiation.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// This test checks that we can explicitly instantiate std::string with a custom
+// character type and traits and then use `shrink_to_fit`. In particular, this is
+// a regression test for the bug that was reported at https://stackoverflow.com/q/69520633/627587
+// and https://seedcentral.apple.com/sm/feedback_collector/radar/85053279.
+
+// RUN: %{cxx} %{flags} %{compile_flags} %s %{link_flags} -DTU1 -c -o %t.tu1.o
+// RUN: %{cxx} %{flags} %{compile_flags} %s %{link_flags} -DTU2 -c -o %t.tu2.o
+// RUN: %{cxx} %{flags} %t.tu1.o %t.tu2.o %{link_flags} -o %t.exe
+
+// UNSUPPORTED: libcpp-has-no-localization
+
+#include <cstdint>
+#include <ios>
+#include <string>
+
+typedef std::uint16_t char16;
+
+struct string16_char_traits {
+  typedef char16 char_type;
+  typedef int int_type;
+
+  typedef std::streamoff off_type;
+  typedef std::mbstate_t state_type;
+  typedef std::fpos<state_type> pos_type;
+
+  static void assign(char_type&, const char_type&) { }
+  static bool eq(const char_type&, const char_type&) { return false; }
+  static bool lt(const char_type&, const char_type&) { return false; }
+  static int compare(const char_type*, const char_type*, size_t) { return 0; }
+  static size_t length(const char_type*) { return 0; }
+  static const char_type* find(const char_type*, size_t, const char_type&) { return nullptr; }
+  static char_type* move(char_type*, const char_type*, size_t) { return nullptr; }
+  static char_type* copy(char_type*, const char_type*, size_t) { return nullptr; }
+  static char_type* assign(char_type*, size_t, char_type) { return nullptr; }
+  static int_type not_eof(const int_type&) { return 0; }
+  static char_type to_char_type(const int_type&) { return char_type(); }
+  static int_type to_int_type(const char_type&) { return int_type(); }
+  static bool eq_int_type(const int_type&, const int_type&) { return false; }
+  static int_type eof() { return int_type(); }
+};
+
+#if defined(TU1)
+template class std::basic_string<char16, string16_char_traits>;
+#else
+extern template class std::basic_string<char16, string16_char_traits>;
+
+int main() {
+    std::basic_string<char16, string16_char_traits> s;
+    s.shrink_to_fit();
+}
+#endif