Re: Memory fault when inserting QTableItem into QTable

On Monday 12 July 2004 03:03, Ashley Winters wrote:
> --- Richard Dale <[email protected]> wrote:
> [snip]
>
> > PerlQt overrides the takeItem() virtual method as part of its garbage
> >
> > collection tracking, and so when the perl version of takeItem() is
> > called the
> > recently deleted instance is found to not have a corresponding perl
> > instance
> > and another is created. Inside the perl takeItem() method the call
> > 'setAllocated( $_[0], 1 );' means that PerlQt owns this newly created
> >
> > instance owns the (already deleted) QTableItem and is responsible for
> >
> > deleting it.
> >
> > sub Qt::Table::takeItem
> > {
> >     package Qt::_internal;
> >     delete ${ this()->{"hidden children"} } { sv_to_ptr($_[0]) };
> >     delete $_[0]->{"has been hidden"};
> >     setAllocated( $_[0], 1 );
> >     no strict 'refs';
> >     $Qt::AutoLoad::AUTOLOAD = 'Qt::Table::takeItem';
> >     my $autoload = " Qt::Table::_UTOLOAD";
> >     dontRecurse();
> >     $autoload->( $_[0] );
> > }
>
> Hmm...
>
> > On garbage collection of the '$_[0]' passed to takeItem(), PerlQt
> > attempts to
> > delete an already deleted instance and there is a crash. Now I
> > understand
> > what's going on I can start to think of a fix..
>
> The takeItem() family of functions need to know who their caller is.
> When being called from C++, they should bypass all garbage-collection
> voodoo. It's either being called from C++ or from Perl, and whichever
> one is calling it beeds to take responsibility for deleting the object
> thereafter.
>
> When called from Perl, there is already a perfectly valid $item
> (otherwise, how did it get passed to takeItem()?), and we are obligated
> to call setAllocated(1) on the object, as well as remove any
> pre-existing 'hidden children' object remaining in the parent object.
> That way, Perl's garbage collection will properly destroy the object
> (just like the programmer was asking in the first place by calling
> takeItem()).
>
> When called from C++, the item being taken must be setAllocated(0), to
> allow the calling function to move or delete the item as it wishes.
> It's explicitly taking responsibility for the destruction of the object
> AWAY from Perl by calling takeItem(), and we need to honor it.
>
> So, the patch is a simple one-line fix:
>
> -     setAllocated( $_[0], 1 );
> +     setAllocated( $_[0], was_called_from_perl() );
>
> Of course, someone has to write was_called_from_perl(), which needs to
> pass the following test:
>
> package test1;
> use Qt::isa 'Qt::Table';
>
> # This does NOT qualify as being called from Perl if it is called from
> # C++
> sub takeItem { SUPER->takeItem(@_) }
>
> package test2;
> use Qt::isa 'Qt::Table';
>
> # How do you propose to pass this one when called from C++?
> sub takeItem { this->Qt::Table::takeItem(@_) }
>
> Okay, test2 counts as being called from Perl even when
> test2::takeItem() is called from C++. Use SUPER for proper behavior.
>
> Of course, all the takeItem() functions need the above one-line patch.
I think that rather than needing to determine how takeItem() has been been 
called, we need to be able to mark instances as being already deleted in 
QtSmokeBinding::deleted(). The attached patch adds a 'zombie' flag to the 
smokeperl_object struct like this:

struct smokeperl_object {
    bool allocated;
    bool zombie;
    Smoke *smoke;
    int classId;
    void *ptr;
};

So QtSmokeBinding::deleted() no longer unmaps the instance being deleted, it 
just marks it as a zombie. Later in handlers.cpp, smokeperl_free() always 
unmaps the instance, but only deletes it if it is both allocated and not a 
zombie:

int smokeperl_free(pTHX_ SV *sv, MAGIC *mg) {
    smokeperl_object *o = (smokeperl_object*)mg->mg_ptr;

    const char *className = o->smoke->classes[o->classId].className;
    unmapPointer(o, o->classId, 0);
    if(o->allocated && !o->zombie && o->ptr) {
        if(do_debug & qtdb_gc) fprintf(stderr, "Deleting (%s*)%p\n", 
className, o->ptr);
...

This way the perl takeItem()s don't need any change, because is zombie flag 
will always trump an allocated flag.

-- Richard

_______________________________________________
Kde-perl mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-perl

diff -Naur -X /home/duke/bin/patcher.exclude PerlQt/handlers.cpp temp/handlers.cpp
--- PerlQt/handlers.cpp	2003-09-09 09:40:28.000000000 +0100
+++ temp/handlers.cpp	2004-07-12 06:11:09.000000000 +0100
@@ -48,11 +48,10 @@
     smokeperl_object *o = (smokeperl_object*)mg->mg_ptr;
 
     const char *className = o->smoke->classes[o->classId].className;
-    if(o->allocated && o->ptr) {
+    unmapPointer(o, o->classId, 0);
+    if(o->allocated && !o->zombie && o->ptr) {
         if(do_debug & qtdb_gc) fprintf(stderr, "Deleting (%s*)%p\n", className, o->ptr);
         SmokeClass sc(o->smoke, o->classId);
-	if(sc.hasVirtual()) 
-            unmapPointer(o, o->classId, 0);
         object_count --;
         char *methodName = new char[strlen(className) + 2];
         methodName[0] = '~';
@@ -335,6 +334,7 @@
 		o.classId = m->type().classId();
 		o.ptr = p;
 		o.allocated = false;
+		o.zombie = false;
 
 		if(m->type().isStack())
 		    o.allocated = true;
@@ -1051,6 +1051,7 @@
           o.classId = ix;                                          \
           o.ptr = (void*)t;                                        \
           o.allocated = IS_STACK;                                  \
+          o.zombie = false;                                  \
                                                                    \
           sv_bless(obj, gv_stashpv( PCLASS, TRUE));                \
                                                                    \
diff -Naur -X /home/duke/bin/patcher.exclude PerlQt/perlqt.h temp/perlqt.h
--- PerlQt/perlqt.h	2003-06-14 04:47:37.000000000 +0100
+++ temp/perlqt.h	2004-07-12 06:11:09.000000000 +0100
@@ -5,6 +5,7 @@
 
 struct smokeperl_object {
     bool allocated;
+    bool zombie;
     Smoke *smoke;
     int classId;
     void *ptr;
diff -Naur -X /home/duke/bin/patcher.exclude PerlQt/Qt.pm temp/Qt.pm
--- PerlQt/Qt.pm	2003-09-09 03:57:11.000000000 +0100
+++ temp/Qt.pm	2004-07-12 06:11:09.000000000 +0100
@@ -1108,4 +1108,17 @@
     return 1
 }
 
+sub Qt::base::dispose 
+{ 
+    package Qt::_internal;
+	Qt::_internal::dispose(this());
+	return;
+}
+
+sub Qt::base::isDisposed 
+{ 
+    package Qt::_internal;
+	return Qt::_internal::isDisposed(this());
+}
+
 1;
diff -Naur -X /home/duke/bin/patcher.exclude PerlQt/Qt.xs temp/Qt.xs
--- PerlQt/Qt.xs	2003-09-09 09:41:25.000000000 +0100
+++ temp/Qt.xs	2004-07-12 06:11:09.000000000 +0100
@@ -689,8 +689,7 @@
 	if(!o || !o->ptr) {
 	    return;
 	}
-        unmapPointer(o, o->classId, 0);
-	o->ptr = 0;
+        o->zombie = true;
     }
     bool callMethod(Smoke::Index method, void *ptr, Smoke::Stack args, bool isAbstract) {
 	SV *obj = getPointerObject(ptr);
@@ -1677,6 +1676,7 @@
     o.classId = qt_Smoke->idClass("QMetaObject");
     o.ptr = meta;
     o.allocated = true;
+    o.zombie = false;
     sv_magic((SV*)hv, sv_qapp, '~', (char*)&o, sizeof(o));
     MAGIC *mg = mg_find((SV*)hv, '~');
     mg->mg_virtual = &vtbl_smoke;
@@ -1743,6 +1743,44 @@
     delete qobj;
 
 void
+dispose(obj)
+    SV *obj
+    CODE:
+    smokeperl_object *o = sv_obj_info(obj);
+    if(!o || !o->ptr) { 
+		XSRETURN_EMPTY;
+	} else {
+		const char *className = o->smoke->classes[o->classId].className;
+		char *methodName = new char[strlen(className) + 2];
+		methodName[0] = '~';
+		strcpy(methodName + 1, className);
+		Smoke::Index nameId = o->smoke->idMethodName(methodName);
+		Smoke::Index meth = o->smoke->findMethod(o->classId, nameId);
+		if(meth > 0) {
+			Smoke::Method &m = o->smoke->methods[o->smoke->methodMaps[meth].method];
+			Smoke::ClassFn fn = o->smoke->classes[m.classId].classFn;
+			Smoke::StackItem i[1];
+			(*fn)(m.method, o->ptr, i);
+		}
+		delete[] methodName;
+		o->allocated = false;
+		o->zombie = true;
+	}
+
+bool
+isDisposed(obj)
+    SV *obj
+    CODE:
+    smokeperl_object *o = sv_obj_info(obj);
+    if(!o || o->zombie) { 
+		RETVAL = 0; 
+	} else {
+		RETVAL = 1;
+	}
+    OUTPUT:
+    RETVAL
+
+void
 mapObject(obj)
     SV *obj
     CODE: