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

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added a comment.

In D142902#4119069 <https://reviews.llvm.org/D142902#4119069>, @ldionne wrote:

> Have libstdc++ and MSVC concerted to make sure their respective pmr implementations were ABI compatible?

It's not fully ABI compatible. As far as I know GNU libstdc++ doesn't have compat layer at all. Since Microsoft linker works differently comparing with GNU one, without some compat layer Microsoft linker would not choose the right symbol to link with. `memory_resource`, however, should be ABI compatible. They list the virtual functions in the same order (probably occasionally). So, if somebody passes it across translation units everything should work fine.

In D142902#4119069 <https://reviews.llvm.org/D142902#4119069>, @ldionne wrote:

> This sort of "guarantee" is extremely difficult to provide and maintain through years of software evolution, so it's not to be taken lightly. If we claim to be ABI compatible but we fail to do a good job at it, we won't be helping anyone, so that would be even worse than not providing it from the start.

Ok, how about making just `<get|set>_memory_resource` ABI compatible?

We could leave everything in `_LIBCPP_BEGIN_NAMESPACE_STD` and then use functions with different (mangled) names comparing with GNU and MSVC STL. In the implementation of compat layer for those functions we `reinterpret_cast` `memory_resource` similarly how it's done in exceptions ABI compatibility layer for `exception_ptr`. Since `memory_resource` is already ABI compatible (not name-wise but layout-wise) everything should work just fine. I could also understand your concern about potential further changes of `memory_resource` but I think changing the set of virtual methods is an ABI breaking change anyway because you cannot freely path this object anymore across the translation units after that. BTW, with the option of making just `<get|set>_memory_resource` ABI compatible I believe we could apply this patch even after the release because it would not be the ABI break. I need to think more about `new_delete_resource` and `null_memory_resource` though, to make sure they are also unaffected.

I often mention `<get|set>_terminate` as the example of similar behavior and I understand that things are more complicated here due to virtual table of `memory_resource` but I still believe in use-case importance where user could set global memory pool for the whole application and then all the calls just go to the right place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142902