[libcxx-commits] [PATCH] D125184: [libc++] fix std::to_string() crashing in debug mode

Mon May 16 10:56:14 PDT 2022

philnik added a comment.

In D125184#3516419 <https://reviews.llvm.org/D125184#3516419>, @Mordante wrote:

> In D125184#3514315 <https://reviews.llvm.org/D125184#3514315>, @llunak wrote:
>
>> In D125184#3514289 <https://reviews.llvm.org/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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125184/new/

https://reviews.llvm.org/D125184