Module-local bindings #949
Conversation
|
I think this is a great idea, and your implementation looks good to me. |
There was a problem hiding this comment.
I believe there is an ODR violation here. See the comments for details. It should be possible to resolve.
I'd suggest renaming py::local to py::module_local in order to be more specific and avoid possible confusion with Python's locals/globals builtins. We already have a py::globals which mimics one of those on the C++ side. If the need arises for something like py::locals, local/locals will be easy to mix up.
The extra test module is a good idea. This can definitely be used for more cross-module testing, e.g. #951 could also use it, and there have been some internals issues in the past that could be covered with a cross-module test. I'd suggest renaming this new module to something along the line of pybind11_cross_module_tests to reflect that it's not only for module-local binding tests.
include/pybind11/cast.h
Outdated
| @@ -111,6 +128,14 @@ PYBIND11_NOINLINE inline internals &get_internals() { | |||
| return *internals_ptr; | |||
| } | |||
|
|
|||
| namespace { | |||
| // Works like internals.registered_types_cpp, but for module-local registered types: | |||
| inline type_map<void *> ®istered_local_types_cpp() { | |||
There was a problem hiding this comment.
A function inside an anonymous namespace in a header is a likely ODR violation. Explanation and suggestion in the following comment.
| if (it != types.end()) | ||
| return (detail::type_info *) it->second; | ||
| auto &locals = registered_local_types_cpp(); |
There was a problem hiding this comment.
I'm reasonably sure this is an ODR violation. registered_local_types_cpp() is located inside an anonymous namespace inside a header which means there's going to be one unique function per translation unit, i.e.:
a.cpp -> pybind11::detail::<unique_name_a>::registered_local_types_cpp()
b.cpp -> pybind11::detail::<unique_name_b>::registered_local_types_cpp()
On the other hand, get_type_info() is not in an anonymous namespace, so per ODR we are promising that there is just one of those for all transaltion units. But because of the call highlighted above, the body of get_type_info() will be defined differently in different translalation units, i.e. one will call <unique_name_a>::registered_local_types_cpp(), another <unique_name_b>::, etc.
A possible solution could be to move the definition of registered_local_types_cpp into the PYBIND11_MODULE macro (without the anon namespace) which would ensure a single definition per module. Although, I'm not sure if GCC will consider the function-local static variable as unique or shared among different modules.
Putting it into the |
|
There is one thorny issue though—what do do about embedding that doesn't have a macro at all? |
Yeah, that does throw a wrench in the works. Technically, there is the This also made me realize that the bindings are also made local to a Do you think it would be possible to place the local type map inside a capsule and attach it to a |
|
I can't see how storing a capsule in the module would work. Storing the capsule is fine, but I don't see an obvious way to actually get the module (to pass it in to How about if we simply drop the anonymous namespace, and go back to the local-to-the-current-so approach? (and perhaps name it |
|
That definitely solves the ODR issue, but I was under the impression that GCC shared function-scope statics across modules -- the issues with |
|
A bit of further investigation: gcc only seems to behave this way when not compiling with With |
|
It also doesn't show up under |
|
ac21545 should fix the gcc visibility issue (I also simplified |
|
Nice investigative work! The solution with the function attribute looks ideal (as opposed to the compiler flag, which we have no way of enforcing). Right now, you've included the attribute for gcc but not clang. I'd suggest including it for any compiler which defines |
As far as I can tell this only affects g++. It can't hurt to turn it on for the others, though. |
This commit adds a PYBIND11_UNSHARED_STATIC_LOCALS macro that forces a function to have hidden visibility under gcc and gcc-compatible compilers. gcc, in particular, needs this to to avoid sharing static local variables across modules (which happens even under a RTLD_LOCAL dlopen()!). clang doesn't appear to have this issue, but the forced visibility on internal pybind functions certainly won't hurt it and icc. This updates the workaround from pybind#862 to use this rather than the version-specific template.
This adds the infrastructure for a separate test plugin for cross-module tests. (This commit contains no tests that actually use it, but the following commits do; this is separated simply to provide a cleaner commit history).
The builtin exception handler currently doesn't work across modules
under clang/libc++ for builtin pybind exceptions like
`pybind11::error_already_set` or `pybind11::stop_iteration`: under
RTLD_LOCAL module loading clang considers each module's exception
classes distinct types. This then means that the base exception
translator fails to catch the exceptions and the fall through to the
generic `std::exception` handler, which completely breaks things like
`stop_iteration`: only the `stop_iteration` of the first module loaded
actually works properly; later modules raise a RuntimeError with no
message when trying to invoke their iterators.
For example, two modules defined like this exhibit the behaviour under
clang++/libc++:
z1.cpp:
#include <pybind11/pybind11.h>
#include <pybind11/stl_bind.h>
namespace py = pybind11;
PYBIND11_MODULE(z1, m) {
py::bind_vector<std::vector<long>>(m, "IntVector");
}
z2.cpp:
#include <pybind11/pybind11.h>
#include <pybind11/stl_bind.h>
namespace py = pybind11;
PYBIND11_MODULE(z2, m) {
py::bind_vector<std::vector<double>>(m, "FloatVector");
}
Python:
import z1, z2
for i in z2.FloatVector():
pass
results in:
Traceback (most recent call last):
File "zs.py", line 2, in <module>
for i in z2.FloatVector():
RuntimeError
This commit fixes the issue by adding a new exception translator each
time the internals pointer is initialized from python builtins: this
generally means the internals data was initialized by some other
module. (The extra translator(s) are skipped under libstdc++).
This commit adds a `py::module_local` attribute that lets you confine a
registered type to the module (more technically, the shared object) in
which it is defined, by registering it with:
py::class_<C>(m, "C", py::module_local())
This will allow the same C++ class `C` to be registered in different
modules with independent sets of class definitions. On the Python side,
two such types will be completely distinct; on the C++ side, the C++
type resolves to a different Python type in each module.
This applies `py::module_local` automatically to `stl_bind.h` bindings
when the container value type looks like something global: i.e. when it
is a converting type (for example, when binding a `std::vector<int>`),
or when it is a registered type itself bound with `py::module_local`.
This should help resolve potential future conflicts (e.g. if two
completely unrelated modules both try to bind a `std::vector<int>`.
Users can override the automatic selection by adding a
`py::module_local()` or `py::module_local(false)`.
Note that this does mildly break backwards compatibility: bound stl
containers of basic types like `std::vector<int>` cannot be bound in one
module and returned in a different module. (This can be re-enabled with
`py::module_local(false)` as described above, but with the potential for
eventual load conflicts).
jagerman
force-pushed
the
local-bindings
branch
from
7f0552b
to
c8208ac
Compare
|
The revised implementation here looks excellent. Sorry, I've been busy as of late and fallen behind on the reviews. Just one quick question: what happens if a type is registered as both local and global? I didn't find any tests for that case, but I may have missed it. |
If a type is already registered globally, you'll get a failure if you try to register it locally. If you go the other way, there will be no failure, but the global type will take precedence (local types are only checked if the global type isn't found). |
|
Now that I think about it, I wonder if that's the right behaviour. Perhaps it would make more sense to make the local type take precedence. |
|
Appleclang seems to be broken in debug mode. Strangely, the older appleclang 7.3.0 on Travis and the latest 8.1.0 fail with different errors: 7.3.0 8.1.0 The key difference in debug mode is that |
|
I think the issue is coming about because the functions calling the functions with the static local are not becoming hidden visibility, and so we're back into the ODR issue. It seems to me that the same issue is going to apply under appleclang for What if we just force hidden visibility for everything under the Obviously we'd need to rename the macro (e.g. |
|
Adding the visibility attribute to the namespace seems good to me. The only other options are visibility pragmas or function-level attributes, but neither are very convenient. |
|
There is one potential downside to this in that it leads to a warning under gcc for code like this (from struct PythonCallInDestructor {
// ...
py::dict d;
};That's probably minor enough to ignore, but we can at least mention it in the release notes. |
|
What about limiting the visibility attribute to the |
|
I'm not confident that that's going to fix the issue under macOS: e.g. some non-hidden, non- I'm not particular familiar with the OS X |
|
Indeed I just verified--it's not enough to just force it on |
|
The same issue can be reproduced on Linux by changing Python's if sys.platform.startswith("linux"):
sys.setdlopenflags(os.RTLD_NOW | os.RTLD_GLOBAL)This reproduces the issue when compiled without On the macOS side, this can't be configured at run time, but there's a compile-time option which looks to be enabled by default (at least with the homebrew build; haven't looked into others yet). So it seems like there are two possible solutions here:
Right now I'm leaning toward solution 1. It seems a bit cleaner and it would only require user intervention for custom build systems in conjunction with |
|
I don't see why it would only affect The warning that comes up under 2. only comes up if you're not using Edit: what I mean is, if we require 1., it's harmless to do 2., but doing 2. allows things to still work if the user ignores 1. anyway (this wouldn't be officially supported, but in most cases would still work). |
Good point. Although, the versioning issues have a possible alternative solution of using an inline namespace (which could similarly be added to the namespace macro), but this isn't an option for One other thing to consider with the namespace attribute is that it would need to be applied consistently, but there are cases where the namespace macro isn't used, like user-defined casters and also a couple of places internally. That could also result in mixed visibility issues. My main worry is that it could be fragile.
So you're suggesting to do both the compiler flag and namespace attribute? |
Yeah. Then you'll only actually run into warnings if you forget the |
|
Ideally we would add a warning ourselves if not compiling under |
OK, that sounds good. As far as I've gathered, you've already been experimenting with the namespace attributes. Would you be willing to turn that into a PR along with the debug visibility flag change? |
This adds a PYBIND11_NAMESPACE macro that expands to the `pybind11` namespace with hidden visibility under gcc-type compilers, and otherwise to the plain `pybind11`. This then forces hidden visibility on everything in pybind, solving the visibility issues discussed at end end of pybind#949.
This adds a PYBIND11_NAMESPACE macro that expands to the `pybind11` namespace with hidden visibility under gcc-type compilers, and otherwise to the plain `pybind11`. This then forces hidden visibility on everything in pybind, solving the visibility issues discussed at end end of pybind#949.
Done in #995. We can carry on discussion there. |
This adds a PYBIND11_NAMESPACE macro that expands to the `pybind11` namespace with hidden visibility under gcc-type compilers, and otherwise to the plain `pybind11`. This then forces hidden visibility on everything in pybind, solving the visibility issues discussed at end end of pybind#949.
This adds a PYBIND11_NAMESPACE macro that expands to the `pybind11` namespace with hidden visibility under gcc-type compilers, and otherwise to the plain `pybind11`. This then forces hidden visibility on everything in pybind, solving the visibility issues discussed at end end of #949.
This PR adds a
py::localattribute that lets one confine a type registration to the module (more technically, the shared object) in which it is defined, by registering it with:py::class_<C>(m, "C", py::local())This will allow the C++ class
Cto be registered in different modules with independent sets of class definitions. On the Python side, two such types will be completely distinct; on the C++ side, the C++ resolves to a different Python types in each module.This also fixes #919 / #439 by making the
stl_bind.hbinding code applypy::local()by default (but passing inpy::local(false)can be used in cases where you want to use the stl-bound type across module boundaries).Tests are included showing this working; I haven't yet written up the documentation as I wanted feedback/input on the idea and approach first.