Skip to content

Problem with constructor for a class with custom operator new #948

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

Closed
dimar opened this issue Jul 18, 2017 · 14 comments
Closed

Problem with constructor for a class with custom operator new #948

dimar opened this issue Jul 18, 2017 · 14 comments

Comments

@dimar
Copy link

dimar commented Jul 18, 2017

Hello, all!

I have a problem exposing a class with custom operator new (custom allocation in a heap book).
Here is an example:

#include <pybind11/pybind11.h>
namespace py = pybind11;

class Example {
public:
    static void*    operator new(std::size_t size);
    static void     operator delete(void* p, std::size_t size);
    Example() {}
};

PYBIND11_MODULE(test, m) {
    m.doc() = "test";
    py::class_<Example>(m, "Example")
            .def(py::init<>())
    ;
}

The problem arises due to the fact that this statement

 .def(py::init<>())

is a shorthand for

.def("__init__", [](Example &instance) {new (&instance) Example();}

which results in the next error:
pybind11/pybind11.h:1320:60: error: no matching function for call to ‘Example::operator new(sizetype, Base*&)’ cl.def("__init__", [](Base *self_, Args... args) { new (self_) Base(args...); }, extra...);

It worked in Boost.Python out-of-the-box, because they have, apparently, different way of creating a constructor's bindings.

How it is possible to resolve this problem?

@wjakob
Copy link
Member

wjakob commented Jul 18, 2017

Does it work if you also add dummy placement new operator, i.e.

void *operator new(size_t, void *ptr) { return ptr; }
void *operator new[](size_t, void *ptr) { return ptr; }

@dimar
Copy link
Author

dimar commented Jul 18, 2017

Thank you for the answer, Wenzel!
This code compiles:

static void*		operator new(size_t, void *ptr) { return ptr; }
static void*		operator new[](size_t, void *ptr) { return ptr; }

And it works even without dummy operator, just the first line. The difference with code in the question is the presence of void* ptr.
But I won't be able to change code in the library I am exporting. I can write a simple wrapper class, in which I can put overload of new and delete, it compiles. But I don't think that it is a nice workaround. I just wonder may be there is another way, since it worked in boost.python without any changes.

@jagerman
Copy link
Member

jagerman commented Jul 18, 2017

Edit for future readers: #805 also includes changes that makes the workaround described below unnecessary.


PR #805 can help with a class that doesn't support placement new: it can handle a pointer, so you could write:

cl.def(py::init([](Arg a, int b) { return new Class(a, b); }))

We could, in theory, automatically map a py::init<Arg, int>() to that when the class doesn't support placement new initialization, but it might also be simpler to just document the above as the approach when a class doesn't support placement new.

@dimar
Copy link
Author

dimar commented Jul 19, 2017

Thank you, Jason!
Sorry, haven't found this discussion.
I checked the branch you are referring to (with factory), and added the code you suggested:

.def(py::init([]() { return new Example(); }))

It compiles, but when I try to import this module into python (v.2.7), I get the next error
ImportError: ./test.so: undefined symbol: _ZN7ExamplenwEm
which is due to the placement new, the same will be for the delete.

jagerman added a commit to jagerman/pybind11 that referenced this issue Jul 19, 2017
Added a test for a factory init workaround for a type without placement
new support (issue pybind#948).
@jagerman
Copy link
Member

I think that error is happening because your operator new and operator delete in your original example are missing an actual implementation.

I've added a test to the PR #805 branch where this is working.

@dimar
Copy link
Author

dimar commented Jul 20, 2017

Thank you, Jason!
Yes, you are right - I forgot about the implementation.
If placement new and delete are implemented as in your test it works fine, but in the library for which I am writing bindings they are implemented differently - they allocate and de-allocate heap pages in a heap book. It compiles, but I have an error while trying to create an instance of the class in Python (v.2.7).
I do not understand what causes the problem and for now I do not have an access to the source code of the library, but only to headers.

So the example looks like this:

class Example {
public:
    static void*    operator new(std::size_t size);
    static void     operator delete(void* p, std::size_t size);
    Example() {}
private:
    static MyHeapBook heapBook;
};

#define PAGE_SIZE 4096
MyHeapBook Example::heapBook(sizeof(Example), PAGE_SIZE);

void *Example::operator new(std::size_t size) {
    py::print("operator new called");
    return heapBook.allocate();
}
void Example::operator delete(void *p, std::size_t size) {
    py::print("operator delete called");
    heapBook.free(p);
}

PYBIND11_MODULE(test, m) {
    m.doc() = "test";
    py::class_<Example>(m, "Example")
            .def(py::init([]() { return new Example(); }))
    ;
}

where MyHeapBook is defined like this (I am omitting a lot of stuff, because I do not have an access to the source code):

class MyHeapPage;

class MyHeapBook {
public:
    // Constructors and destructors
    MyHeapBook(std::size_t objectSize, unsigned int numberOfObjectsInPage);
    virtual ~MyHeapBook();
    // Memory management
    void*       allocate();                 // Allocates memory for one object
    void        free(void* objectPointer);  // Frees the memory at address objectPointer
    /*...*/
private:
    MyHeapPage*     firstPage;              // The first page
    void*           firstFreeObject;        // The address of the first free object
    std::size_t     objectSize;             // The number of bytes per object
    unsigned int    numberOfObjectsInPage;  // The number of objects in the page
    std::mutex      *mutex;
    /*...*/
};
void* MyHeapBook::allocate() {
    mutex->lock();
    if (firstFreeObject == 0) {
        firstPage = new MyHeapPage(objectSize,
                           numberOfObjectsInPage, firstPage); // add a new memory page
        firstFreeObject = firstPage->objectArray;
    }
    void* objectPointer = (char*)firstFreeObject + sizeof(void*); // skip the header
    firstFreeObject = (void*)(*((std::size_t*)firstFreeObject)); // move to the next free object
    mutex->unlock();
    return objectPointer;
}
void MyHeapBook::free(void* objectPointer) {
    mutex->lock();
    // the location of the header of the freed object
    char* headerLocation = (char*)objectPointer - sizeof(void*);
    // make the previous "first free object" the next object of the freed object
    *((std::size_t*)headerLocation) = (std::size_t)(firstFreeObject);
    // the freed object becomes the first free object
    firstFreeObject = (void*)headerLocation;
    mutex->unlock();
}

class MyHeapPage {
public:
    // Constructors and destructors
    MyHeapPage(std::size_t objectSize,
                unsigned int numberOfObjectsInPage, MyHeapPage* nextPage);
    virtual ~MyHeapPage();
    /*...*/
private:
    friend class MyHeapBook;
    void*          objectArray;    // The object array
    MyHeapPage*    nextPage;       // The next page in the heap book
    /*...*/
};

And the error in Python is:

In [1]: import test
In [2]: test.Example()
operator new called
operator new called
*** Error in `python': free(): invalid pointer: 0x000000000159bda8 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7fe0e6fc97e5]
/lib/x86_64-linux-gnu/libc.so.6(+0x8037a)[0x7fe0e6fd237a]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7fe0e6fd653c]
./test.so(_ZN8pybind116detail20call_operator_deleteEPv+0x18)[0x7fe0e2f5c9af]
./test.so(_ZN8pybind116class_I7ExampleIEE7deallocERKNS_6detail16value_and_holderE+0x49)[0x7fe0e2f65334]
./test.so(_ZN8pybind116detail14clear_instanceEP7_object+0x118)[0x7fe0e2f5751f]
./test.so(_ZN8pybind116detail18init_factory_resetEPNS0_8instanceE+0xe1)[0x7fe0e2f5d7a3]
./test.so(+0x71983)[0x7fe0e2f4e983]
./test.so(+0x7169d)[0x7fe0e2f4e69d]
./test.so(+0x715d9)[0x7fe0e2f4e5d9]
./test.so(+0x726b3)[0x7fe0e2f4f6b3]
./test.so(+0x72642)[0x7fe0e2f4f642]
./test.so(+0x721c7)[0x7fe0e2f4f1c7]
./test.so(+0x7223d)[0x7fe0e2f4f23d]
./test.so(_ZN8pybind1112cpp_function10dispatcherEP7_objectS2_S2_+0xa5a)[0x7fe0e2f5ae55]
...
...

Any ideas on the source of the error or on a workaround are appreciated!
Again, it worked in Boost.Python with def(boost::python::init<>()), so the problem is due to the usage of placement new and with pointers, I guess.

@jagerman
Copy link
Member

What pybind with the factory PR is essentially:

  • When the new type is created, a new pointer is is allocated with T::operator new(sizeof(T)) and stored in the part of the python object that pybind manages.
  • When the factory init is called, it gets a new pointer, calls T::operator delete(oldptr); on the existing pointer in the object, then stores the new pointer in the object instead.

Ah, I see the issue: your class doesn't have T::operator delete(oldptr), and we aren't handling that.

@dimar
Copy link
Author

dimar commented Jul 20, 2017

Thank you, Jason!
Yes, you are right, it is because of the second argument in operator delete(void* p, std::size_t size). If I use operator delete(void* p) instead, it works, no errors.
The second argument in operator delete is used for some internal checking and cannot be omitted. Is it possible to include support for, maybe, variadic operator delete in pybind11?

@jagerman
Copy link
Member

#952 should solve the delete problem.

@dimar
Copy link
Author

dimar commented Jul 20, 2017

Thank you very much, Jason!
Checked on the example and on a real code - works like a charm!

@dimar dimar closed this as completed Jul 21, 2017
@jagerman
Copy link
Member

Thanks for the report. (I'm reopening for bookkeeping just until the PRs get merged).

@jagerman jagerman reopened this Jul 21, 2017
jagerman added a commit to jagerman/pybind11 that referenced this issue Jul 29, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
jagerman added a commit to jagerman/pybind11 that referenced this issue Jul 29, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
jagerman added a commit to jagerman/pybind11 that referenced this issue Jul 29, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
jagerman added a commit to jagerman/pybind11 that referenced this issue Aug 1, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
@jagerman
Copy link
Member

jagerman commented Aug 1, 2017

Fixed via PR #952. oops, never mind, this also depends on #805.

I've updated #805 with some performance optimizations that make the factory constructors just as efficient as preallocation + placement new, so switched the default py::init<...> to use the factory constructors instead of placement new. What this means is that the workaround shouldn't be necessary anymore once #805 is merged.

@jagerman jagerman closed this as completed Aug 1, 2017
@jagerman jagerman reopened this Aug 1, 2017
@dimar
Copy link
Author

dimar commented Aug 1, 2017

Great! Thank you, Jason!

jagerman added a commit to jagerman/pybind11 that referenced this issue Aug 2, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
jagerman added a commit to jagerman/pybind11 that referenced this issue Aug 13, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
jagerman added a commit to jagerman/pybind11 that referenced this issue Aug 14, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
jagerman added a commit to jagerman/pybind11 that referenced this issue Aug 17, 2017
This allows you to use:

    cls.def(py::init(&factory_function));

where `factory_function` returns a pointer, holder, or value of the
class type (or a derived type).  Various compile-time checks
(static_asserts) are performed to ensure the function is valid, and
various run-time type checks where necessary.

The feature is optional, and requires including the <pybind11/factory.h>
header.

Some other details of this feature:
- The `py::init` name doesn't conflict with the templated no-argument
  `py::init<...>()`, but keeps the naming consistent (especially if we
  decide to roll this into the core, documented approach replacing raw
  placement-new `__init__`s).
- If returning a CppClass (whether by value or pointer) when an CppAlias
  is required (i.e. python-side inheritance and a declared alias), a
  dynamic_cast to the alias is attempted (for the pointer version); if
  it fails, or if returned by value, an Alias(Class &&) constructor
  is invoked.  If this constructor doesn't exist, a runtime error occurs.
- for holder returns when an alias is required, we try a dynamic_cast of
  the wrapped pointer to the alias to see if it is already an alias
  instance; if it isn't, we raise an error.
- `py::init(class_factory, alias_factory)` is also available that takes
  two factories: the first is called when an alias is not needed, the
  second when it is.
- It also allows using a factory init to workaround binding a type
  without placement new support (issue pybind#948), which is basically
  impossible with this approach.
@jagerman
Copy link
Member

Fixed now (via #805). py::init<...> now just does a simple new Class, no longer requiring placement new support.

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

3 participants