[libc++] fix std::to_string() crashing in debug mode

[llunak]

Summary

The to_string() functions are in the string.cpp source that is compiled
without debug, so the string is not registered, leading to a crash
later. Handle that by adding making to_string() to be a wrapper
in an inline namespace when debug mode is used, and register the string
there. This does not change ABI for non-debug mode while keeping
the function distinct in debug mode.

Handles https://github.com/llvm/llvm-project/issues/55328 .

[philnik]

You can't do this because it's ABI breaking. @ldionne is working on fixing the debug mode, but it will take some time. We plan to have a separate debug dylib, which should fix this problem and a lot of other problems related to the debug mode.

[llunak]

I see. Until that some time, how about at least this one then? It's admitedly ugly, but still less ugly then crashing later after the function has been called.

Also, while you'll be at reworking this stuff, I suggest also putting the debug versions of the affected classes in their own namespace, like libstdc++ AFAIK does. That would have made problems like this obvious, and it would also make obvious when using debug mode together with C++ libraries that have not been built that way (currently it'll just crash without any indication).

In fact, unless you for some strange reason care about ABI for debug mode, you could do that even now, and then you wouldn't need an extra library nor care about how libc++ has been built.

[philnik]

I don't think that this approach is better. Now if you pass in a long long it will crash , but no other integral type will. That's just as unexpected as the current state of things, maybe even more. If you want to know a bit more about the debug mode plans you can read D122941.

libcxx/include/string

4557

These functions should neither be _LIBCPP_ALWAYS_INLINE nor static. They should be marked _LIBCPP_HIDE_FROM_ABI.

[llunak]

This is getting too hackish, so I'm abandoning this.

In D125184#3499334, @philnik wrote:

If you want to know a bit more about the debug mode plans you can read D122941.

Seems reasonable after skimming through it. Fine with me as long there's a simple way to switch between debug and non-debug mode, and as long as mixing libraries using and not using debug mode is detected e.g. by link errors instead of by crashing/misbehaving.

[llunak]

I'd like to give this another try after all. AFAICT using an inline namespace solves the problem of ABI for non-debug mode, as this way it is not affected at all, while the debug mode std::to_string() is now properly a separate function.

[llunak]

Added to the abi check list, I hope I got it right.

[Mordante]

I agree this is worth fixing, but I'm not convinced that this solution is safe and doesn't lead to ODR violations when different translation units are compiled with different values of _LIBCPP_DEBUG_LEVEL. I'll leave the review to @ldionne.

libcxx/include/string

4551

Please use an __ugly_name.

[llunak]

In D125184#3514289, @Mordante wrote:

I agree this is worth fixing, but I'm not convinced that this solution is safe and doesn't lead to ODR violations when different translation units are compiled with different values of _LIBCPP_DEBUG_LEVEL.

As you can see in the abi check lists, the debug functions are in a different namespace, so technically they are different functions. (You actually cannot the see to_string() functions themselves there, only the helpers, but any out-of-line copies of those would get the same treatment.)

[Mordante]

In D125184#3514315, @llunak wrote:

In D125184#3514289, @Mordante wrote:

I agree this is worth fixing, but I'm not convinced that this solution is safe and doesn't lead to ODR violations when different translation units are compiled with different values of _LIBCPP_DEBUG_LEVEL.

As you can see in the abi check lists, the debug functions are in a different namespace, so technically they are different functions. (You actually cannot the see to_string() functions themselves there, only the helpers, but any out-of-line copies of those would get the same treatment.)

I'm concerned regarding the difference between`_LIBCPP_FUNC_VIS` and _LIBCPP_HIDE_FROM_ABI for the to_string function. I'm not sure whether that may cause an ODR violation. I don't expect issues with the new functions in the __debug namespace.

[philnik]

In D125184#3516419, @Mordante wrote:

In D125184#3514315, @llunak wrote:

In D125184#3514289, @Mordante wrote:

I agree this is worth fixing, but I'm not convinced that this solution is safe and doesn't lead to ODR violations when different translation units are compiled with different values of _LIBCPP_DEBUG_LEVEL.

As you can see in the abi check lists, the debug functions are in a different namespace, so technically they are different functions. (You actually cannot the see to_string() functions themselves there, only the helpers, but any out-of-line copies of those would get the same treatment.)

I'm concerned regarding the difference between`_LIBCPP_FUNC_VIS` and _LIBCPP_HIDE_FROM_ABI for the to_string function. I'm not sure whether that may cause an ODR violation. I don't expect issues with the new functions in the __debug namespace.

That's OK because they are in different namespaces. That's the point of inline namespaces - all the ODR and ABI rules are the same between inline and non-inline namespaces, but the API is the same as if all the content was in it's surrounding namespace. My main problem with this approach is that we add symbols to the dylib which we know will be obsolete in a few months, but we have to keep them forever due to ABI concerns.

[Mordante]

In D125184#3516573, @philnik wrote:

My main problem with this approach is that we add symbols to the dylib which we know will be obsolete in a few months, but we have to keep them forever due to ABI concerns.

I'm concerned about that too. That's why I think @ldionne should decide whether or not we want go in this direction.

[llunak]

In D125184#3516573, @philnik wrote:

My main problem with this approach is that we add symbols to the dylib which we know will be obsolete in a few months, but we have to keep them forever due to ABI concerns.

In D122941 you both say that debug mode does not have stable ABI. Moreover whatever it'll be exactly that you'll do to make these obsolete will presumably break ABI too.

[philnik]

In D125184#3516866, @llunak wrote:

In D125184#3516573, @philnik wrote:

My main problem with this approach is that we add symbols to the dylib which we know will be obsolete in a few months, but we have to keep them forever due to ABI concerns.

In D122941 you both say that debug mode does not have stable ABI. Moreover whatever it'll be exactly that you'll do to make these obsolete will presumably break ABI too.

We are saying that the new debug should use the unstable ABI. While we don't guarantee ABI stability for the current debug mode, the ABI has been stable for a long time. This means that we would break users who depend on this. The new debug mode won't break the ABI of the current debug mode AFAIK. I don't know if we would actually keep these functions in the dylib, but I fear that will be the case if we add them silently now.

[llunak]

In D125184#3516920, @philnik wrote:

The new debug mode won't break the ABI of the current debug mode AFAIK. I don't know if we would actually keep these functions in the dylib, but I fear that will be the case if we add them silently now.

Exactly the same applies to this patch. This patch does not break the ABI of the current debug mode either. It only affects newly compiled code, just like the new debug mode will. If you intend to not break the current debug mode ABI, you'll need to keep the debug functions in that case as well (and I doubt these tiny wrappers would make a difference there).

We are saying that the new debug should use the unstable ABI. While we don't guarantee ABI stability for the current debug mode, the ABI has been stable for a long time. This means that we would break users who depend on this.

I can't quite imagine who would actually require stable ABI for the debug mode, because I can't see why anyone would use debug mode for anything else than local test builds. The lack of namespacing for debug mode practically means everything using it has to be built from source, otherwise there'll be a risk of mixing such objects up and getting "mysterious" crashes. The debug mode makes code slow(er) and there are problems like this crash. I don't see why anyone would try to ship or deploy code built with debug mode enabled. Some vendors don't even ship debug mode enabled in libc++ itself. So who are supposed to be these users who depend on this?

But even if, what do you suggest then? Is the new debug mode going to be ready for LLVM15? It doesn't seem so. Are you going to ship an ABI-stable function that is known to crash?

[ldionne]

Thanks for the patch. While I appreciate the desire to fix the debug mode, I think this patch is too targeted and hence not the right way to go. I am working on a proposal to fix the debug mode and that would fix both this but also handle the ABI mismatch you discussed and much more.

libcxx/include/string

 _LIBCPP_FUNC_VIS unsigned long      stoul (const string& __str, size_t* __idx = nullptr, int __base = 10);
 _LIBCPP_FUNC_VIS long long          stoll (const string& __str, size_t* __idx = nullptr, int __base = 10);
 _LIBCPP_FUNC_VIS unsigned long long stoull(const string& __str, size_t* __idx = nullptr, int __base = 10);
 
 _LIBCPP_FUNC_VIS float       stof (const string& __str, size_t* __idx = nullptr);
 _LIBCPP_FUNC_VIS double      stod (const string& __str, size_t* __idx = nullptr);
 _LIBCPP_FUNC_VIS long double stold(const string& __str, size_t* __idx = nullptr);
 
+#if _LIBCPP_DEBUG_LEVEL != 2 || !defined(_LIBCPP_ALWAYS_INLINE)
 _LIBCPP_FUNC_VIS string to_string(int __val);
 _LIBCPP_FUNC_VIS string to_string(unsigned __val);
 _LIBCPP_FUNC_VIS string to_string(long __val);
 _LIBCPP_FUNC_VIS string to_string(unsigned long __val);
 _LIBCPP_FUNC_VIS string to_string(long long __val);
 _LIBCPP_FUNC_VIS string to_string(unsigned long long __val);
 _LIBCPP_FUNC_VIS string to_string(float __val);
 _LIBCPP_FUNC_VIS string to_string(double __val);
 _LIBCPP_FUNC_VIS string to_string(long double __val);
+#else
+// HACK: These crash in debug mode and cannot be changed without changing ABI.
+// Keep only the ones with the largest types and wrap the rest to fix the problem.
+_LIBCPP_FUNC_VIS string to_string(long long __val);
+_LIBCPP_FUNC_VIS string to_string(unsigned long long __val);
+_LIBCPP_FUNC_VIS string to_string(long double __val);
+
+static _LIBCPP_ALWAYS_INLINE string to_string(int __val)
+{
+    string __s = to_string(static_cast<long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE string to_string(unsigned __val)
+{
+    string __s = to_string(static_cast<unsigned long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE string to_string(long __val)
+{
+    string __s = to_string(static_cast<long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE string to_string(unsigned long __val)
+{
+    string __s = to_string(static_cast<unsigned long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE string to_string(float __val)
+{
+    string __s = to_string(static_cast<long double>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE string to_string(double __val)
+{
+    string __s = to_string(static_cast<long double>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+#endif
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 _LIBCPP_FUNC_VIS int                stoi  (const wstring& __str, size_t* __idx = nullptr, int __base = 10);
 _LIBCPP_FUNC_VIS long               stol  (const wstring& __str, size_t* __idx = nullptr, int __base = 10);
 _LIBCPP_FUNC_VIS unsigned long      stoul (const wstring& __str, size_t* __idx = nullptr, int __base = 10);
 _LIBCPP_FUNC_VIS long long          stoll (const wstring& __str, size_t* __idx = nullptr, int __base = 10);
 _LIBCPP_FUNC_VIS unsigned long long stoull(const wstring& __str, size_t* __idx = nullptr, int __base = 10);
 
 _LIBCPP_FUNC_VIS float       stof (const wstring& __str, size_t* __idx = nullptr);
 _LIBCPP_FUNC_VIS double      stod (const wstring& __str, size_t* __idx = nullptr);
 _LIBCPP_FUNC_VIS long double stold(const wstring& __str, size_t* __idx = nullptr);
 
+#if _LIBCPP_DEBUG_LEVEL != 2 || !defined(_LIBCPP_ALWAYS_INLINE)
 _LIBCPP_FUNC_VIS wstring to_wstring(int __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(unsigned __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(long __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(unsigned long __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(long long __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(unsigned long long __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(float __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(double __val);
 _LIBCPP_FUNC_VIS wstring to_wstring(long double __val);
+#else
+// HACK: These crash in debug mode and cannot be changed without changing ABI.
+// Keep only the ones with the largest types and wrap the rest to fix the problem.
+_LIBCPP_FUNC_VIS wstring to_wstring(long long __val);
+_LIBCPP_FUNC_VIS wstring to_wstring(unsigned long long __val);
+_LIBCPP_FUNC_VIS wstring to_wstring(long double __val);
+
+static _LIBCPP_ALWAYS_INLINE wstring to_wstring(int __val)
+{
+    wstring __s = to_wstring(static_cast<long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE wstring to_wstring(unsigned __val)
+{
+    wstring __s = to_wstring(static_cast<unsigned long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE wstring to_wstring(long __val)
+{
+    wstring __s = to_wstring(static_cast<long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE wstring to_wstring(unsigned long __val)
+{
+    wstring __s = to_wstring(static_cast<unsigned long long>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE wstring to_wstring(float __val)
+{
+    wstring __s = to_wstring(static_cast<long double>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+
+static _LIBCPP_ALWAYS_INLINE wstring to_wstring(double __val)
+{
+    wstring __s = to_wstring(static_cast<long double>(__val));
+    std::__debug_db_insert_c(&__s);
+    return __s;
+}
+#endif
 #endif // _LIBCPP_HAS_NO_WIDE_CHARACTERS
 
 template<class _CharT, class _Traits, class _Allocator>
 _LIBCPP_TEMPLATE_DATA_VIS
 const typename basic_string<_CharT, _Traits, _Allocator>::size_type
                basic_string<_CharT, _Traits, _Allocator>::npos;
 
 template <class _CharT, class _Allocator>