Papierkorb / bindgen

Binding and wrapper generator for C/C++ libraries
GNU General Public License v3.0
179 stars 18 forks source link

Destructors not called on GC-enabled instances #86

Closed HertzDevil closed 2 years ago

HertzDevil commented 4 years ago

At no point during execution does Bindgen actually invoke the destructors of wrapped C++ instances; UseGC only ensures the instances use the GC heap, deletion is still performed by a raw GC_free. Consider:

#include <iostream>
#include <gc/gc_cpp.h>

struct T {
  virtual ~T() { std::cout << "T::~T()\n"; }
};

extern "C" T * bg_T__CONSTRUCT_() {
  return new (UseGC) T();
}
lib Binding
  alias T = Void
  fun bg_T__CONSTRUCT_() : T*
end

x = Binding.bg_T__CONSTRUCT_()
x = nil
5.times {GC.collect} # a plain collection won't work, perhaps because the pointer would then be on the stack

Nothing happens, but if a clean-up function is supplied to the constructor:

extern "C" void bg_T__DESTRUCT_(void * _self_, void * client_data) {
  reinterpret_cast<T *>(_self_)->~T();
}

extern "C" T *bg_T__CONSTRUCT_() {
  return new (UseGC, bg_T__DESTRUCT_) T();
}

Then T::~T() will be printed. Is this behaviour intentional? (To be fair, since Qt has its own ownership semantics, calling the destructors on returned wrappers would probably break it immediately.)

f-fr commented 2 years ago

I just hit exactly the same problem with the qt bindings. The memory of allocated classes is freed but their destructors are not called. And because many Qt classes use the pimpl-idiom, this means they have an internal d-pointer to some data and this data will be leaked. This is true even for simple classes like QBrush (this is where the problem hit me because I used Qt::Brush in some paint_event causing the program to eat up all memory).

This is really a serious issue. However, it can probably be solved wrapping new instances of wrapped classes in some small template wrapper inheriting from gc_cleanup or so.

f-fr commented 2 years ago

I created a simple implementation that seems to work, at least with the qt bindings.

docelic commented 2 years ago

Closing due to code added in #119