Skip to content

[BUG] Should steer pybind11's internal ABI away from STL types to make them work cross-compiler #2773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EricCousineau-TRI opened this issue Jan 8, 2021 · 4 comments

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Jan 8, 2021

Shifting discussion from here:
#2772 (comment)

From @henryiii:

Also, I thought standard library templated classes were not valid in an ABI (@jpivarski)? I take it this is why we have so many issues with the ABI? Could we redesign this to just be simple classes and do our own memory management here?

His expansion on this:

These macros are evil; they force us to match extensions with exact compilers; while it would be much nicer to be able to interact with any extension that has a matching ABI flag. I think it's because we have lots of STL in our ABI, which causes it to be very picky; if we only interacted through standard types, and internally/externally handled the memory and such, then we could get rid of these macros entirely. That would be the direction to move, rather than adding more (like a string). But it's not "new" to a string.

PyTorch ignores these macros, because the internal JIT compiler may not match the host compiler (the JIT compiler just stores the host's definition of these macros and uses them, IIRC). At least one other user needed this to talk between different compilers, as well, for CUDA I think.

If we bump the ABI version, we'll need to coordinate with conda-forge, they pin on ABI versions and have to trigger a migration (I think) if it gets bumped.

TLDR: for now, if we can keep the ABI the same, that's best. Eventually we will need a bump. In the future, maybe we can design internals that work cross-compiler?

\cc @YannickJadoul @wjakob @rwgk

@EricCousineau-TRI
Copy link
Collaborator Author

FWIW I started hacking this out 🤷

/// Minimal container to have managed data for a C-string. This is done to
/// avoid having storage use `std::string` which might cause ABI issues across
/// different compiled "flavors" of the same version of pybind11 in the same
/// process.
class managed_cstr {
public:
    managed_cstr() = default;
    managed_cstr(const std::string& s)
        : len_(s.size()) {
        // Include null-terminator.
        c_str_ = new char[len_ + 1];
        std::memcpy(c_str_, s.data(), len_ + 1);
    }

    size_t hash() const {
        return std::hash<std::string>(c_str_);
    }
private:
    size_t len_{};
    char* c_str_{};
};

@EricCousineau-TRI
Copy link
Collaborator Author

See Yannick's post here: #2772 (comment)

@EricCousineau-TRI
Copy link
Collaborator Author

Re-opening. Added Henry's overview to this issue's top-level post

@EricCousineau-TRI EricCousineau-TRI changed the title [BUG] Will usages of std::string in py::detail::internals cause ABI issues? [BUG] Should steer pybind11's internal ABI away from STL types to make them work cross-compiler Jan 8, 2021
@bstaletic
Copy link
Collaborator

I've had a look at making py::detail::internals a C struct. Replacing a std::string with a managed_cstr is the easy part. (Well, if we ignore the strong exception safety, move semantics, type traits, char traits, allocators...). A much more difficult problem would be reinventing all those std::unordered_maps. I see three options:

  1. Still going for reinventing the standard containers/iterators/etc. Besides the gigantic effort, how much additional code would this put in pybind11/detail/containers/?
  2. Rewrite the uses of py::detail::internals to minimize the need for STL niceties, like range based for loops. Then reinvent the containers. If pybind doesn't use iterators, the reinvented containers don't need iterator traits, for example.
  3. Take a completely different approach, using the pimpl idiom. This one would be easy, but I have a feeling the inevitable performance overhead would be measurable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants