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.
Details
Diff Detail
Unit Tests
Event Timeline
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.
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. |
This is getting too hackish, so I'm abandoning this.
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.
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.
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.
I'm concerned about that too. That's why I think @ldionne should decide whether or not we want go in this direction.
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.
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?
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.
Please use an __ugly_name.