Skip to content

static allocation for PyModuleDef, to avoid leak check errors. #2413

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

Merged
merged 4 commits into from
Sep 27, 2020

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Aug 19, 2020

This PR solves the same issue as #2019 (rolled back), but in a way that is certain to portably work for any leak checker.

The Python 3 documentation suggests static allocation for PyModuleDef:

This PR changes the PYBIND11_MODULE macro accordingly: static PyModuleDef ...;

The pybind11::module_::module_ code is slightly refactored, with the idea to make the future removal of Python 2 support straightforward.

Note: Potential follow-up change:

- if (!def) def = new PyModuleDef();
+ if (!def) pybind11_fail("PyModuleDef *def must not be a nullptr.");

This is best left for a separate PR, because it is an API change, requiring a pointer that didn't have to be passed before.

This PR passed extensive testing in the Google environment (~370 extensions, several 10k indirect dependents).

@rwgk rwgk force-pushed the static_pymoduledef branch 2 times, most recently from 1b4ed95 to 0d0b37c Compare August 28, 2020 20:22
@rwgk rwgk marked this pull request as ready for review August 28, 2020 20:25
#define PYBIND11_MODULE(name, variable) \
PYBIND11_MAYBE_UNUSED \
static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module &); \
PYBIND11_PLUGIN_IMPL(name) { \
PYBIND11_CHECK_PYTHON_VERSION \
PYBIND11_ENSURE_INTERNALS_READY \
auto m = pybind11::module(PYBIND11_TOSTRING(name)); \
PYBIND11_MODULE_PY2_VS_PY3_SPECIFIC(name) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woudn't this be better not factored out into a macro when we remove Python 2 support? Then you would just drop the #else part and be done with it, rather than now having to remove the extra macro.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woudn't this be better not factored out into a macro when we remove Python 2 support?

Thoughts regarding duplicating the entire PBBIND11_MODULE macro:

  • Very slightly easier to purge Python 2 support. However, even someone who's never seen this code before can purge Python 2 support from the code as-is in less than 5 minutes (just move two lines down).

  • Code is read more often than it is written. Assume 100 people look at the 11 (out of 12 or 13) lines copied here. It will take most people at least a couple minutes to conclusively determine that the 11 lines are exactly identical. So the total time spent will be 200 minutes.

5 minutes << 200 minutes.
That doesn't even take into account time lost to oversights when this code needs to be modified in two places.

I feel strongly not duplicating this code is the right call here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a long macro, yes. Maybe the problem is the name that makes this look weird, @henryiii, since it containss PY2/PY3?
Why don't we just call this PYBIND11_CREATE_MODULE (which is what it does, so it's easier to read for those 100 people reading this part of the code without looking at the macro above). Then forgetting to remove the macro when dropping Python 2 wouldn't even be an issue, since it does still create a module.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the problem is that you can't have an #if statement in a macro. Yes, then @YannickJadoul solution is good, as it describes what does. Since macros are global, having a relatively easy and not clearly "detail" or "internal" name might be an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, the main problem with macros. If it's not documented, then using it is at the user's own risk, though?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like PYBIND11_NAMESPACE?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the problem is that you can't have an #if statement in a macro. Yes, then @YannickJadoul solution is good, as it describes what does. Since macros are global, having a relatively easy and not clearly "detail" or "internal" name might be an issue?

Thanks @YannickJadoul and @henryiii , I changed the helper macro name to PYBIND11_MODULE_DETAIL_CREATE, taking ideas from both of you.

Generally, I prefer very long macro names for private macros ("privacy by intentional obscurity"), e.g. this could be PYBIND11_MODULE_DETAIL_PY2_VS_PY3_SPECIFIC, but I get the sense shorter names are preferred here.

Copy link
Collaborator

@YannickJadoul YannickJadoul Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try figuring out a general approach, at some point. Is there a way projects tend to handle "private macros" (unfortunately, leading underscores are reserved in C++) ? (Curious, though: why is DETAIL inserted right in the very middle of the name; wouldn't PYBIND11_DETAIL make more sense as an emerging convention easy to use in future macros, similar to pybind11::detail?)

Then again, not documenting is good enough. I'm really not going to lose sleep over users using a non-documented macro; they shouldn't, and if they do, they should know what the risks are?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PYBIND11_MODULE_DETAIL_PY2_VS_PY3_SPECIFIC, but I get the sense shorter names are preferred here.

PS: My problem wasn't per se the long name (though not a fan, and I doubt if long, descriptive names will stop users for accessing it), but the thing that the name specifies the implementation. Suppose e.g. PyPy has a problem and needs a different way of handling the creation of a module, then you'd need to change the name; now you don't.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know my comments aren't strictly related to what you're doing, but pybind11 seems to be doing at least 1 bad thing when constructing a module.

#if PY_MAJOR_VERSION >= 3
PyModuleDef *def = new PyModuleDef();
explicit module(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
if (!def) def = new PyModuleDef();
std::memset(def, 0, sizeof(PyModuleDef));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this memset required? new T() is doing value initialization, so any C struct would already be set to zero.

Except there's the static one, which is default initialized, leaving everything in uninitialized state. Can we do static PyModuleDef def();? Trying...

As expected, "the most vexing parse" is making things difficult. static auto def = PyModuleDef(); works.

"But Boris... that's copy initialization! Copying is in the name... Copy bad!"

Short answer - check the assembly.

Long answer - Copy elision isn't a normal optimization. It has to be performed very early if it is going to be performed at all. That's why copy elision is "visible" at -O0.

 

Anyway, totally non-blocking.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, static auto def = PyModuleDef(); produces this directive:

function_name():
	; snip
	.type function_name()::def,@object
	.local function_name()::def
	.comm function_name()::def,104,32 ; common subsection of .data, 104 uninitialized bytes, 32byte alignment.
	                                  ; It's not in .bss, which is initialized to 0 for free

Therefore, we need the memset somewhere. The new PyModuleDef() does initialize the struct to zero.

#if PY_MAJOR_VERSION >= 3
PyModuleDef *def = new PyModuleDef();
explicit module(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
if (!def) def = new PyModuleDef();
std::memset(def, 0, sizeof(PyModuleDef));
def->m_name = name;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, zeroing def and then not setting def->base is broken, according to CPython docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm testing a new version, copying the commit message here for easy reference:

Initializing PyModuleDef object with PyModuleDef_HEAD_INIT.

Python 3.8 documentation: m_base - Always initialize this member to PyModuleDef_HEAD_INIT.

Long-standing (since first github commit in 2015), inconsequential bug.

Also removing inconsequential Py_INCREF(def): PyModule_Create() resets the reference count to 1.

I already tested locally with ASAN & MSAN. I'll report the result from the Google global tests when they are available.

Remark: I think the bug was inconsequential because the only non-zero member is the reference count, which is reset by PyModule_Create() anyway.

/* m_name */ name,
/* m_doc */ options::show_user_defined_docstrings() ? doc : nullptr,
/* m_size */ -1,
/* m_methods */ NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not nullptr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed, thanks Boris. — It was just what I happened to find in tests/cross_module_gil_utils.cpp. I agree nullptr is better, especially to avoid mixing uses of NULL and nullptr so close together.

Py_INCREF(def);
explicit module(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
if (!def) def = new PyModuleDef();
new (def) PyModuleDef { // Placement new (not an allocation).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that you need def = new (def) PyModuleDef { here. Placement new ends the lifetime of the previously existing object, so you have two options to avoid UB - Assign to def to reuse the pointer or use PyModule_Create(std::launder(def)). Latter of which is a C++17 utility.

I know this is a nitpicky comment about a formal UB that shouldn't cause problems, but are we willing to take that chance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Changed. I didn't know that detail, I'm glad you knew it is UB without the re-assignment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, this is an insane requirement, caused by the complex issues C++ imposed on itself by divorcing storage from lifetime. A sane language should not require std::launder...

This is why I love C.

@rwgk rwgk force-pushed the static_pymoduledef branch from 519c505 to 730be55 Compare September 1, 2020 00:12
Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be completely honest, the UB I mentioned may not be UB. I'm not sure, but what I said might not apply when the "buffer" is the same type as the return value and is not const, which is the case with def as the buffer for placement new.

However, the assignment is always correct and I'd rather not rely on even more esoteric knowledge of botched C++ concepts. And this is coming from someone who loves C++.

@rwgk rwgk force-pushed the static_pymoduledef branch from 730be55 to 664a868 Compare September 1, 2020 18:54
Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm left wondering if there's a better solution than a static variable (which basically lives just as long a non-freed new, but just doesn't show up in a leak checker): the Python docs only say that "There is usually only one statically initialized variable of this type for each module.", not that that's the only way).
Python itself is full of such "leaks" as well, so another option would be to provide a suppression file, but OK, always nice to plumb a leak.

Apart from the remarks, let's also ask @T045T, who wrote the original PR #2019, and a follow-up PR #2024. (#2024 somehow wasn't mentioned or considered in this PR? Any reason why?)
Any thoughts or input, @T045T? :-)

def->m_doc = doc;
def->m_size = -1;
Py_INCREF(def);
explicit module(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the overload with PyModuleDef * private, or something like that?

pybind11 has tried (and succeeded) to keep the raw C API types away from its public interface (except maybe handle::ptr() and related, but there it's unavoidable). This is especially the case for high-level classes like class_, enum_, or module.
I'm also not a huge fan on an API that changes between between Python 2 and 3.

So can't we hide this away better?
Writing this, I'm realizing we can't make the init-function a friend (because the function name of PYBIND11_PLUGIN_IMPL(name) depends on name), but we could make pybind11::detail::create_top_level_module and make that a friend of py::module.

Maybe there's even a better way, but I'd really like to keep our public interface free of C API types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that's making the implementation overly complicated for a very minor gain. Let's ask Wenzel if he thinks it's worth this complication.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Let's get some extra input :-)

I don't think it would be that complicated, actually. I can try out some things and work out a proof of concept, if you wish?

And keeping the users away from the C API (also with potential future changes to that same C API in mind) doesn't sound all too minor to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A first proof-of-concept shows it's almost trivial (most changes are moving that constructor down to be private):

diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h
index 84e13e27..efa0a377 100644
--- a/include/pybind11/detail/common.h
+++ b/include/pybind11/detail/common.h
@@ -309,7 +309,8 @@ extern "C" {
 #if PY_MAJOR_VERSION >= 3
 #define PYBIND11_MODULE_DETAIL_CREATE(name)                                    \
         static PyModuleDef mdef;                                               \
-        auto m = pybind11::module(PYBIND11_TOSTRING(name), nullptr, &mdef);
+        auto m = pybind11::detail::create_top_level_module(                    \
+            PYBIND11_TOSTRING(name), nullptr, &mdef);
 #else
 #define PYBIND11_MODULE_DETAIL_CREATE(name)                                    \
         auto m = pybind11::module(PYBIND11_TOSTRING(name));
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index d0799b4e..8e47fa2d 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -821,6 +821,12 @@ protected:
     }
 };
 
+class module;
+
+PYBIND11_NAMESPACE_BEGIN(detail)
+module create_top_level_module(const char *name, const char *doc, PyModuleDef *def);
+PYBIND11_NAMESPACE_END(detail)
+
 /// Wrapper for Python extension modules
 class module : public object {
 public:
@@ -828,29 +834,16 @@ public:
 
     /// Create a new top-level Python module with the given name and docstring
 #if PY_MAJOR_VERSION >= 3
-    explicit module(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr) {
-        if (!def) def = new PyModuleDef();
-        def = new (def) PyModuleDef {  // Placement new (not an allocation).
-            /* m_base */     PyModuleDef_HEAD_INIT,
-            /* m_name */     name,
-            /* m_doc */      options::show_user_defined_docstrings() ? doc : nullptr,
-            /* m_size */     -1,
-            /* m_methods */  nullptr,
-            /* m_slots */    nullptr,
-            /* m_traverse */ nullptr,
-            /* m_clear */    nullptr,
-            /* m_free */     nullptr
-        };
-        m_ptr = PyModule_Create(def);
+    explicit module(const char *name, const char *doc = nullptr) : module(name, doc, new PyModuleDef()) {}
 #else
     explicit module(const char *name, const char *doc = nullptr) {
         m_ptr = Py_InitModule3(
             name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
-#endif
         if (m_ptr == nullptr)
             pybind11_fail("Internal error in module::module()");
         inc_ref();
     }
+#endif
 
     /** \rst
         Create Python binding for a new function within the module scope. ``Func``
@@ -915,8 +908,37 @@ public:
 
         PyModule_AddObject(ptr(), name, obj.inc_ref().ptr() /* steals a reference */);
     }
+
+private:
+#if PY_MAJOR_VERSION >= 3
+    friend module detail::create_top_level_module(const char *, const char *, PyModuleDef *);
+
+    explicit module(const char *name, const char *doc, PyModuleDef *def) {
+        def = new (def) PyModuleDef {  // Placement new (not an allocation).
+            /* m_base */     PyModuleDef_HEAD_INIT,
+            /* m_name */     name,
+            /* m_doc */      options::show_user_defined_docstrings() ? doc : nullptr,
+            /* m_size */     -1,
+            /* m_methods */  nullptr,
+            /* m_slots */    nullptr,
+            /* m_traverse */ nullptr,
+            /* m_clear */    nullptr,
+            /* m_free */     nullptr
+        };
+        m_ptr = PyModule_Create(def);
+        if (m_ptr == nullptr)
+            pybind11_fail("Internal error in module::module()");
+        inc_ref();
+    }
+#endif
 };
 
+PYBIND11_NAMESPACE_BEGIN(detail)
+inline module create_top_level_module(const char *name, const char *doc, PyModuleDef *def){
+    return module(name, doc, def);
+}
+PYBIND11_NAMESPACE_END(detail)
+
 /// \ingroup python_builtins
 /// Return a dictionary representing the global variables in the current execution frame,
 /// or ``__main__.__dict__`` if there is no frame (usually when the interpreter is embedded).

I'm sure it can still be cleaned up a bit, though.
The only ugly part (IMO) is that C++ doesn't seem to allow a friend function in the detail namespace without forward declaration. Not sure if that can be circumvented.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I took a look at this and after a bit of thinking came to the same solution as @YannickJadoul.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I took a look at this and after a bit of thinking came to the same solution as @YannickJadoul.

And it's worth the bit of extra complexity to you? (I mainly still wonder if if (m_ptr == nullptr) pybind11_fail("Internal error in module::module()"); inc_ref(); could easily be unduplicated again, but I also don't believe it's a big issue.)

I realized afterwards I probably still forgot two pairs of #if PY_MAJOR_VERSION >= 3/#endif (around the forward declaration and implementation of detail::create_top_level_module. Detail, though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think it's too much.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Sep 1, 2020

(#2024 somehow wasn't mentioned or considered in this PR? Any reason why?)

@bstaletic actually also was involved in those two PRs. Given that you've reviewed this, you believe this is a better solution than the one proposed in #2024, then, @bstaletic?

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 1, 2020

(#2024 somehow wasn't mentioned or considered in this PR? Any reason why?)

@bstaletic actually also was involved in those two PRs. Given that you've reviewed this, you believe this is a better solution than the one proposed in #2024, then, @bstaletic?

I wasn't aware of #2024. I had the idea behind this PR when I saw leak check issues testing in the Google environment.

I think the static allocation is best: follows the recommendation in the Python documentation, and not the slightest chance any non-buggy leak checker will flag this.

@YannickJadoul
Copy link
Collaborator

I think the static allocation is best:

It might be, yes. Just curious about the comparison, and what we can maybe learn from the other one.
The current version of #2024 also has static PyModuleDefs, but they're in a vector, and not handled from outside of module. That's both uglier and nicer :-)

follows the recommendation in the Python documentation

Python says "usually", though, so I'm unclear whether this is a recommendation or observation of something that's good enough for 99% of extension modules (but maybe not per se for pybind11).

Not saying I oppose this approach, though, but trying to make a comparison and learn about the differences (hence asking @T045T and @bstaletic) :-)

@bstaletic
Copy link
Collaborator

From the perspective of memory management, this does look good. However, I and @T045T tried about half a dozen different approaches and none of them worked correctly. Some ideas worked on some versions of Python while others worked other versions and yet other ideas failed completely. Python, at least pre-3.8, seems to assume that PyModuleDef object have static lifetime. Indeed, dynamic allocation with cleanup only ever worked on 3.8 (back then 3.9 wasn't a thing). I'm pretty confident that this is the right approach when it comes to memory management, but I'm staying slightly reserved due to differing behaviour between python versions.

Reloadability however is also a concern and not an imaginary one as @rwgk seems to imply. Right now we have an open bug that talks about how py::register_exception completely breaks apart when a module is reloaded and, I think, a different user submitted a pull request to fix that. People do rely on being able to restart the interpreter. I'm not saying this PR is a regression in this respect, but I am saying that it is very much worth making sure.

@bstaletic
Copy link
Collaborator

Reloadability however is also a concern and not an imaginary one as @rwgk seems to imply. Right now we have an open bug that talks about how py::register_exception completely breaks apart when a module is reloaded and, I think, a different user submitted a pull request to fix that. People do rely on being able to restart the interpreter. I'm not saying this PR is a regression in this respect, but I am saying that it is very much worth making sure.

Actually, I take this part back. Reloading modules after the interpreter was reinitialized is much more broken that I thought.

#include <pybind11/pybind11.h>
#include <pybind11/embed.h>

int main(void)
{
        pybind11::initialize_interpreter();
        auto reloadable = pybind11::module::import("reloadable");
        pybind11::finalize_interpreter();
        reloadable = pybind11::module::import("reloadable");
        pybind11::finalize_interpreter();
        return 0;
}

Whether reloadable is this:

def print():
    __import__("builtins").print(1)

or this:

#include <pybind11/pybind11.h>

PYBIND11_MODULE(reloadable, m) {
        pybind11::print(1);
}

The result is the same:

Reading symbols from ./a.out...
(gdb) r
Starting program: /home/bstaletic/work/test/cpp/a.out
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
1

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7d856f7 in PyImport_Import () from /usr/lib/libpython3.8.so.1.0
(gdb) bt
#0  0x00007ffff7d856f7 in PyImport_Import () from /usr/lib/libpython3.8.so.1.0
#1  0x00007ffff7df819d in PyImport_ImportModule () from /usr/lib/libpython3.8.so.1.0
#2  0x0000555555565447 in pybind11::module::import (name=0x55555557bebc "reloadable") at /usr/include/pybind11/pybind11.h:849
#3  0x0000555555559b12 in main () at main.cpp:9

The above is from pybind11 2.5.0. Am I missing something?

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 2, 2020

Actually, I take this part back. Reloading modules after the interpreter was reinitialized is much more broken that I thought.

A few months ago I did a simple experiment simply restarting the interpreter, just using the plain C API. Even just that is already unreliable (to put it in the mildest way possible):

https://github.com/rwgk/stuff/blob/ad24dcc503af060ef109f7bc7e54f8a8e64518d4/noddy/embedded_noddy_main_run.c

Back then I found a stackoverflow post concluding (paraphrasing, I didn't keep the URL): only ever call PyInitialize once in a given process (and the same for PyFinalize).

Unless CPython makes a major leap forward, I think that's the only good advice.

@YannickJadoul
Copy link
Collaborator

The above is from pybind11 2.5.0. Am I missing something?

You never reinitialized. You did "initialize, import, finalize, import, finalize", right?

But all I asked was basically what @bstaletic just was checking: "Does this affect the way in which pybind11 modules get reloaded? Do things crash, and do they crash differently/worse than before?" It's important enough to know when we do get question, issues and PRs. Thanks for actually thinking along, trying out some stuff, and confirming things don't get worse because of this PR, @bstaletic!

@YannickJadoul
Copy link
Collaborator

I'd still want some input on the API issue, btw. Keeping our API free from C API types and thus not exposing PyModuleDef to the users, and not diverging the Python 2 from the Python 3 module constructor signature, seems important enough to at the very least discuss. Especially if there is a reasonably simple solution.

@bstaletic
Copy link
Collaborator

You never reinitialized. You did "initialize, import, finalize, import, finalize", right?

D'oh! The following works with 2.5.0 and this PR:

// bstaletic@Gallifrey cpp % cat extension.cpp
#include <pybind11/pybind11.h>
struct S{};
PYBIND11_MODULE(extension, m) {
        pybind11::class_<S>(m, "S");
        pybind11::print(1);
}
# bstaletic@Gallifrey cpp % cat pure_python.py
def p():pass
// bstaletic@Gallifrey cpp % cat main.cpp
#include <pybind11/pybind11.h>
#include <pybind11/embed.h>

int main(void)
{
        {
                pybind11::scoped_interpreter g;
                auto extension = pybind11::module::import("extension");
                auto pure_python = pybind11::module::import("pure_python");
                pure_python.attr("p")();
        }
        {
                pybind11::scoped_interpreter g;
                auto extension = (pybind11::module::import("extension"));
                auto pure_python = pybind11::module::import("pure_python");
                pure_python.attr("p")();
        }
}

So, since there's no regression here, I believe this is safe to be merged. One last thing:

Can we make the overload with PyModuleDef * private, or something like that?

I think that would be nice, I don't think it's mandatory. I'll see if I can come up with a nicer way of doing it than @YannickJadoul.

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 8, 2020

@bstaletic wrote ca. Sep 1:

So, since there's no regression here, I believe this is safe to be merged. One last thing:

Can we make the overload with PyModuleDef * private, or something like that?

I think that would be nice, I don't think it's mandatory. I'll see if I can come up with a nicer way of doing it than @YannickJadoul.

For easy reference, here is Yannick's attempt at making the new module constructor private: #2413 (comment)

Question for Wenzel: Is it worth the added complexity, to hide that PyModuleDef is used?

It seems really unlikely that anyone would want to use the new constructor, by-passing the PYBIND11_MODULE macro. Could we just explain in a comment? Something like:

// Entry point for PYBIND11_MODULE macro, not intended for other uses.
explicit module(const char *name, const char *doc = nullptr, PyModuleDef *def = nullptr);

@wjakob
Copy link
Member

wjakob commented Sep 15, 2020

Using a static allocation here to avoid a leak in memory checkers sounds good to me.

I am not so concerned about having a module constructor that takes a CPython API pointer as argument -- this interface is for setting up pybind11 stuff internally, and I don't think that users will really need to construct modules much themselves. So both of the solutions pitched here sound good, I have no major preference.

There is one thing I don't like about those changes though, and it's how we now have static PyModuleDef within the module entry point.

Compilers generate very pessimistic code for this sort of thing because they can't know that this function is only called a single time in a single-threaded environment (GCC/Clang will e.g. add an "is-initialized" field and wrap the whole thing into a mutex).

Can you move this static declaration to the global scope (still static though)? I would also suggest giving it a name that is less likely to collide with something (e.g. pybind11_mdef_${MODULENAME})

@bstaletic: reloading extension modules is a disaster. I don't think that ever worked, other than accidentally via undefined behavior.

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 15, 2020

Can you move this static declaration to the global scope (still static though)? I would also suggest giving it a name that is less likely to collide with something (e.g. pybind11_mdef_${MODULENAME})

Will do as soon as I get a chance (fleeing wildfire smoke atm, no joke sadly). Thanks for the review!

@YannickJadoul
Copy link
Collaborator

Will do as soon as I get a chance (fleeing wildfire smoke atm, no joke sadly). Thanks for the review!

Oh, no. Take care!

So both of the solutions pitched here sound good, I have no major preference.

Go ahead and merge this, then, I'd say, @rwgk. I'll create a PR shortly after with my solution, which should give a better indication of how intrusive the changes actually are, as well as an opportunity for discussion and deciding whether it's worth merging or not.

NB:

@bstaletic: reloading extension modules is a disaster. I don't think that ever worked, other than accidentally via undefined behavior.

We do know this. But this doesn't stop users from trying and complaining about it in issues or on Gitter. So it was useful to figure out if the current behavior changes through this PR (but @bstaletic managed to check that it doesn't seem to :-) ).

@bstaletic
Copy link
Collaborator

Will do as soon as I get a chance (fleeing wildfire smoke atm, no joke sadly). Thanks for the review!

Holy fuck... Take care, @rwgk!

@rwgk rwgk force-pushed the static_pymoduledef branch 2 times, most recently from d832c1e to 296657e Compare September 16, 2020 23:02
@YannickJadoul YannickJadoul added this to the v2.6.0 milestone Sep 22, 2020
@henryiii
Copy link
Collaborator

What's the status on this?

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 24, 2020

What's the status on this?

Sorry, just struggling to find the time to work on this.

@henryiii henryiii marked this pull request as draft September 25, 2020 13:21
@henryiii
Copy link
Collaborator

Please mark as ready-for-review when it is, then! :)

Python 3.8 documentation: m_base - Always initialize this member to PyModuleDef_HEAD_INIT.

Long-standing (since first github commit in 2015), inconsequential bug.

Also removing inconsequential Py_INCREF(def): PyModule_Create() resets the reference count to 1.
@rwgk rwgk force-pushed the static_pymoduledef branch from 296657e to 62bfaf8 Compare September 25, 2020 20:39
@rwgk rwgk marked this pull request as ready for review September 25, 2020 21:21
@rwgk
Copy link
Collaborator Author

rwgk commented Sep 25, 2020

Please mark as ready-for-review when it is, then! :)

@henryiii @YannickJadoul

Ready! (Finally, sorry for the delay.)

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks OK to me!

The only minor nitpick I have is that PYBIND11_MODULE_DETAIL_STATIC_DEF feels like a weird order (I'd suggest PYBIND11_DETAIL_STATIC_MODULE_DEF or so, which reads easier IMO, and maps to namespace pybind11 { namespace detail { static auto module_def; }}). But fine if you think the current naming is better.

Once this is merged, I'll create a PR to judge if the workaround to have a consistent public API (between Python 2 and 3) is worth it.

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 27, 2020

Thanks Boris, Henry, Wenzel, Yannick for the reviews!

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 27, 2020

PYBIND11_DETAIL_STATIC_MODULE_DEF

I renamed the two new macros so that DETAIL comes first, and the first part is the same for both:

PYBIND11_DETAIL_MODULE_STATIC_DEF
PYBIND11_DETAIL_MODULE_CREATE

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

Successfully merging this pull request may close these issues.

5 participants