This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on Jan 30 2023, 8:30 AM.

Details

Reviewers
Quuxplusone
zoecarver
philnik
rarutyun
Group Reviewers
Restricted Project
Summary

The idea of this patch to make libcxx implementation of std::pmr::memory_resource and some of its utilities ABI-compatible with GNU and Microsoft implementation of standard library.

Since both GNU and Microsoft libraries have the name without namespace versioning we would like to remove that as well in libcxx to result in the same mangled name with GNU implementation. It also allows to let's say set memory_resource via std::pmr::set_default_resource in some library compiled with GNU/Microsoft and then read it with std::pmr::<get|set>_default_resource it in the library/application that is built with libcxx.

The similar technic as applied (for example) for std::<get|set>_terminate functions.

I believe it's worth committing this patch before release because now the latest libcxx has no implementation of memory resource. Introducing it later would be the ABI breaking change.

Please advise what tests should we add

Co-authored-by: Konstantin Boyarinov <konstantin.boyarinov@intel.com>

Diff Detail

Event Timeline

rarutyun created this revision.Jan 30 2023, 8:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 8:30 AM
rarutyun requested review of this revision.Jan 30 2023, 8:30 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 30 2023, 8:30 AM
rarutyun edited the summary of this revision. (Show Details)Jan 30 2023, 8:31 AM
philnik requested changes to this revision.Jan 30 2023, 8:36 AM

Our implementation isn't ABI compatible with libstdc++ or the MSVL STL. Putting them in the same namespace just results in subtle bugs.

This revision now requires changes to proceed.Jan 30 2023, 8:36 AM
rarutyun updated this revision to Diff 493391.Jan 30 2023, 1:20 PM

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.

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)

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.

Ok, I basically agree that we should not mix things. But I do believe that making std::pmr::<get|set>_default_resourse as well as std::pmr::memory_resource itself still worth our efforts because as I said it's basic global API for default memory allocation source and might be used application-wide. I definitely agree that monotonic_buffer_resource (+ other resources) and polymorphic_allocator (I believe, you meant that) are not ABI compatible and I am not arguing they should be. Unfortunately, we can not mix namespace std::pmr and namespace std::_1::pmr where _1 is inline namespace because in that case pmr name becomes ambiguous (https://godbolt.org/z/YPEY6Kd8o). What we could do to prevent that ambiguity is

either (Actually I don't believe anybody would want that because it's not consistent with the rest of the library implementation and is not something how it is supposed to introduce the versioning namespace):

namespace std {
namespace pmr
{
inline namespace _1 {
// Non-ABI compatible pmr stuff
}
}

Or something like (That option is more viable in my opinion):

namespace std {

    namespace pmr {
        class memory_resource {};
    }

    inline namespace _1 {
        namespace detail { // whatever name you prefer
            // and other non ABI-compatible APIs
            class monotonic_buffer_resource {};
        }
    }

    namespace pmr {
        // and other pmr non ABI-compatible API names
        using detail::monotonic_buffer_resource;
    }  
}

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.

I completely agree with that. They just don't affect ABI anyhow. So, I don't care in which namespace they are. The reason why it was changed is exactly because of pmr name ambiguity issue I am talking about above.

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

Sorry about that but I occasionally found out that <memory_resource> was implemented in libc++ (at the end of December IIRC) and unfortunately I am not familiar when the release branch is created and frozen. The end of December doesn't sound like "a few months ago" anyway but I do believe if we still have some opportunity window to make it happen in the upcoming release it would be great because later it would mean ABI breaking change that (I think) makes it impossible to introduce in future.

P.S. buildbot is broken with these changes right now but I want to have an understanding if we do that or not and how we do that before fixing every error in the CI.

I don't think this is a guarantee we want to provide at all. Have libstdc++ and MSVC concerted to make sure their respective pmr implementations were ABI compatible? FWIW that's the only thing that would make me think this is a good guarantee to provide (but then we should get it enshrined into the Standard). 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.

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.

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.

ldionne commandeered this revision.Aug 31 2023, 7:56 AM
ldionne edited reviewers, added: rarutyun; removed: ldionne.

[Github PR transition cleanup]

I don't know how to say this other than to say that we are reluctant to increase the complexity and the "API surface" (in the general sense of having a contract with users) of the library in order to provide an impression of ABI-compatibility with MSVC. If we wanted to be truly ABI compatible with MSVC in some regards, we would want to have a much more holistic plan than just making sure these function names match and a few data structures happen to have the same layout. That's just too brittle and it creates a haunted graveyard where we'll forever wonder how much leeway we have in making changes to our code because of this presumed ABI compatibility. I also question how much benefit this would provide to users, since I expect the number of people that will try to mix libc++ and the MSVC STL in a way that requires ABI compatibility to be extremely tiny.

Also, generally speaking, mixing stdlibs makes it super easy for users to get into sketchy situations without understanding why. We generally frown upon making changes that suggest it's a good idea. For all these reasons, I believe this patch is not in line with where we want to take the library so I'm going to close it. (Sorry, that requires commandeering and then abandoning which may seem a bit rude, but it's how Phab works)

ldionne abandoned this revision.Aug 31 2023, 7:56 AM

[Github PR transition cleanup]

I don't know how to say this other than to say that we are reluctant to increase the complexity and the "API surface" (in the general sense of having a contract with users) of the library in order to provide an impression of ABI-compatibility with MSVC. If we wanted to be truly ABI compatible with MSVC in some regards, we would want to have a much more holistic plan than just making sure these function names match and a few data structures happen to have the same layout. That's just too brittle and it creates a haunted graveyard where we'll forever wonder how much leeway we have in making changes to our code because of this presumed ABI compatibility. I also question how much benefit this would provide to users, since I expect the number of people that will try to mix libc++ and the MSVC STL in a way that requires ABI compatibility to be extremely tiny.

Also, generally speaking, mixing stdlibs makes it super easy for users to get into sketchy situations without understanding why. We generally frown upon making changes that suggest it's a good idea. For all these reasons, I believe this patch is not in line with where we want to take the library so I'm going to close it. (Sorry, that requires commandeering and then abandoning which may seem a bit rude, but it's how Phab works)

Hi Louis,

No worries. I am not considering it rude. As I said I see the point of making this patch since you already have some compatibility layer. On the other hand, I can completely understand your arguments. So, to me, it's totally fine to abandon this review if you guys are not interested in those changes.