Skip to content
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

assertion failure in object_rep::add_dependency #22

Open
nitrocaster opened this issue Dec 5, 2015 · 6 comments
Open

assertion failure in object_rep::add_dependency #22

nitrocaster opened this issue Dec 5, 2015 · 6 comments

Comments

@nitrocaster
Copy link

Although this is not specifically related to deboostifying, I considered it would be reasonable to open an issue here since it is the most active fork at the moment.

The problem was described here:

The minimal working example is provided by the author of the above topic:

I basically copy-pasted it here:

extern "C" {
#include <lualib.h>
};
#include <luabind/luabind.hpp>
#include <stdio.h>

using namespace luabind;

class Widget
{
public:
    Widget(const char* msg = "no msg", Widget* parent = nullptr) :
        msg(msg), parent(parent)
    {}
    virtual void foo() { puts("Widget::foo()"); }

    std::string msg;
    Widget* parent;
};

class WidgetWrap : public Widget, public luabind::wrap_base
{
public:
    WidgetWrap(const char* msg, Widget* parent) :
        Widget(msg, parent)
    {}
    void foo() override { call<void>("foo"); }
    static void default_foo(Widget* base) { base->Widget::foo(); }
};

int main()
{
    lua_State* L = luaL_newstate();
    luaL_openlibs(L);
    luabind::open(L);
    module(L)
    [
        class_<Widget, no_bases, default_holder, WidgetWrap>("Widget")
            .def(constructor<const char*, Widget*>())
            .def_readonly("parent", &Widget::parent)
            .def("foo", &Widget::foo, &WidgetWrap::default_foo)
    ];
    const char* code =
        "class 'MyWidget' (Widget)\n"
        "function MyWidget:__init(msg, parent)\n"
        "  Widget.__init(self, msg, parent)\n"
        "  if parent ~= nil then\n"
        "    self.parent.num = 0\n"
        "  end\n"
        "end\n"
        "function MyWidget:foo()\n"
        "  self.parent.num = self.parent.num + 1\n"
        "end\n"
        "parent = MyWidget('parent')\n"
        "child = MyWidget('child', parent)\n";
    luaL_dostring(L, code);
    auto child = object_cast<Widget*>(globals(L)["child"]);
    for (int i = 0; i < 48; i++)
        child->foo();
    return 0;
}
@nitrocaster
Copy link
Author

It turns out previously there was a saparate table for reference storage: f1fe251

Here

self.parent.num = self.parent.num + 1

auto-generated getter for parent returns a reference, which is equivalent (for non-primitive types) to calling a function with return_reference_to policy. In this particular case, each time foo() is called, object_rep gets 2 additional references. These references are not collected by GC. It seems for me that the new referencing scheme (not sure about old variant) is not supposed to delete references ever before corresponding c++ object gets destroyed.

@decimad, any thoughts?

@decimad
Copy link
Owner

decimad commented Dec 6, 2015

Well I had a quick look and it seems to me, that the automatic reference counting is wrong. The member is looked up (pointer), then luabind finds there's already an object_rep for it (global "parent" variable), so a dependency is added but no holder for it is created. Because no holder gets created, there's no holder which could be destroyed, thus the reference counting scheme doesn't work anymore. The dependency policy should really not add a ref, if we're accessing a property which is a pointer to a different object which can be found by get_back_reference... But I don't know how to achieve this currently...

@decimad
Copy link
Owner

decimad commented Dec 6, 2015

Addind a custom overload for a pointer-to-pointer-member solves the problem at hand, however this pointer might point to a true member, so...

template <class T, class D>
object make_get(lua_State* L, D* T::* mem_ptr, std::true_type /*member_ptr*/) const
{
    using result_type = typename reference_result<D*>::type;
    using get_signature = meta::type_list<result_type, Class const&>;

    return make_function(L, access_member_ptr<T, D*, result_type>(mem_ptr), get_signature(), GetPolicies());
}

@decimad
Copy link
Owner

decimad commented Dec 6, 2015

One might be able to replace the pointer converter with a custom one in this case. If it converts to a pointer that is outside of (&object, &object + sizeof(object)) then it would not add a dependency...

@decimad
Copy link
Owner

decimad commented Dec 6, 2015

Oh, i think the right way to go would be to have the pointer-converter of a member-access create an object_rep (through make_pointer_instance) even if there is already a reference to the pointed-to-object.

@nitrocaster nitrocaster mentioned this issue Dec 13, 2015
@decimad
Copy link
Owner

decimad commented Jul 4, 2017

I developed a patch on branch "experimental" which trades off performance for ultimate correctness / memory saving. It would be great if brave people could try that one out, I currently don't have much code triggering these cases at hand.

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