This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by llunak on May 8 2022, 1:51 AM.

Details

Reviewers
philnik
ldionne
Group Reviewers
Restricted Project
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 .

Diff Detail

Event Timeline

llunak created this revision.May 8 2022, 1:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2022, 1:51 AM
llunak requested review of this revision.May 8 2022, 1:51 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 8 2022, 1:51 AM
philnik requested changes to this revision.May 8 2022, 2:01 AM
philnik added subscribers: ldionne, 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.

This revision now requires changes to proceed.May 8 2022, 2:01 AM
llunak updated this revision to Diff 427924.May 8 2022, 4:28 AM
llunak retitled this revision from [libc++] fix std::to_string() crashing with _LIBCPP_DEBUG == 1 to [libc++] work around std::to_string() and _LIBCPP_DEBUG == 1 crashing.
llunak edited the summary of this revision. (Show Details)

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.

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
4535

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

llunak abandoned this revision.May 8 2022, 5:45 AM

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

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 updated this revision to Diff 429514.May 14 2022, 10:20 PM
llunak retitled this revision from [libc++] work around std::to_string() and _LIBCPP_DEBUG == 1 crashing to [libc++] fix std::to_string() crashing in debug mode.
llunak edited the summary of this revision. (Show Details)

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 updated this revision to Diff 429520.May 15 2022, 1:42 AM

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

Mordante added a subscriber: 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
4529

Please use an __ugly_name.

llunak marked 2 inline comments as done.EditedMay 15 2022, 9:06 AM

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.)

llunak updated this revision to Diff 429542.May 15 2022, 9:07 AM

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.

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.

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.

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.

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.

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 requested changes to this revision.May 31 2022, 8:53 AM

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.

This revision now requires changes to proceed.May 31 2022, 8:53 AM
llunak abandoned this revision.Jun 8 2022, 5:27 AM