[libcxx-commits] [PATCH] D142902: [libcxx][pmr] Make std::pmr::memory_resource ABI-compatible with GNU libstdc++ and Microsoft STL implementation.

Mon Jan 30 14:02:28 PST 2023

philnik added a comment.

In D142902#4091857 <https://reviews.llvm.org/D142902#4091857>, @rarutyun wrote:

> In D142902#4090931 <https://reviews.llvm.org/D142902#4090931>, @philnik wrote:
>
>> Our implementation isn't ABI compatible with libstdc++ or the MSVL STL. Putting them in the same namespace just results in subtle bugs.
>
> Is which sense?
>
> - If we are talking about the full C++ standard library implementation - of course, it's not, but some parts were intentionally done compatible. For example: `exception_ptr`, `<get|set>_terminate` and other basic stuff.
> - If we are talking about the full `<memory_resource>` implementation it also would not be fully ABI compatible. But probably it should not be.
> - If we are talking just about `std::pmr::memory_resource` (and `<get|set>_terminate`), I believe it should be ABI compatible. The virtual methods are listed in the same order in libcxx file as in GNU and LLVM. So, to the best of my knowledge (and also based on experiments) the compiler correctly calculates the offset of the virtual table itself and the offset of the called method. That allows to pass memory resource from one binary (or translation unit) and interpret it correctly in other translation unit.
>
> I think `get|set_default_resource` are also possible to make ABI compatible. Memory allocation is pretty basic operation and (as far as I know) there were attempts (probably successful, but as far as I remember something was broken with Windows toolchain) to make `new` ABI compatible with GNU and MSVC STL as well (`set_new_handler`, etc.) The behavior we want here is having the opportunity to have one default resource being set and then use it application-wide (for example if libcxx is built in compat mode with GNU). The similar scenario works with `<get|set_terminate`.
>
> Other classes from `pmr` can potentially bring problems. If that's the concern probably the solution might be to put some APIs to `__1` namespace. And I am not sure if those are bugs or not because nobody guarantees that two different C++ standard library implementations would work in one application (or I misunderstood the context). It might result in submitted bugs in bug tracker, though :) But still libcxx makes some effort to be ABI compatible with GNU and Microsoft for basic APIs. To me this use-cases as important as other examples I've provided above.

I'm not convinced it's a good idea to guarantee ABI compatibility in any way here, but my main concern right now is that the changes in this patch don't reflect what is claimed in the commit message. `monotonic_buffer_resource`, etc. are clearly not ABI compatible and `polymorphic_resource` should probably also be in the versioned namespace. I see no reason to change that. The aliases also don't belong in the unversioned namespace. It doesn't make any real difference, but you look twice every time you come across it. (BTW it would have been nice to see this patch a few months ago, not after the release branch where the ABI is set in stone to have some time to discuss this)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142902