custom model rows-reordered marshal

[Kevin Ryde]

With a perl custom treemodel and a perl connected rows-reordered signal,
there seems to be a gremlin in the way $model->rows_reordered reaches
the signal handler.  The slightly nasty reordered.pl below for instance
prints

    $VAR1 = [
              0,
              undef,

where I hoped for the instance object and a treepath in the first two
args.  The diff below to examples examples/customlist.pl gives also

    $VAR1 = [
              1,
              'rows',

I wonder if it might be something like

        * xs/GtkTreeModel.xs (gtk2perl_tree_model_rows_reordered_marshal):
        Call gtk_tree_model_iter_n_children before building the stack, in case
        it's a Perl class and calls out to ITER_N_CHILDREN.  This fixes
        clobbering of instance and treepath args to a Perl rows-reordered
        handler connected on a Perl model.
  * t/GtkTreeModelIface.t: Exercise rows-reordered marshalling.

---

package MyModel;
use strict;
use warnings;
use Gtk2;

use Glib::Object::Subclass
  Glib::Object::,
  interfaces => [ Gtk2::TreeModel:: ];

use constant LEN => 2;

sub INIT_INSTANCE {
  my ($self) = @_;
}
sub GET_FLAGS {
  return [ 'list-only' ];
}
sub GET_N_COLUMNS {
  return 1;
}
sub GET_COLUMN_TYPE {
  return 'Glib::String';
}
sub GET_ITER {
  my ($self, $path) = @_;
  if ($path->get_depth != 1) { return undef; }
  my ($index) = $path->get_indices;
  if ($index >= LEN) { return undef; }
  return [ 123, $index, undef, undef ];
}
sub GET_PATH {
  my ($self, $iter) = @_;
  return Gtk2::TreePath->new_from_indices ($iter->[1]);
}
sub ITER_N_CHILDREN {
  print "ITER_N_CHILDREN\n";
  return 2;
}
sub ITER_NTH_CHILD {
  my ($self, $iter, $n) = @_;
  if (defined $iter) { return undef; }
  if ($n >= 2) { return undef; }
  return [ 123, $n, undef, undef ];
}
sub ITER_HAS_CHILD {
  my ($self, $iter) = @_;
  if (defined $iter) {
    return 0;  # nothing under rows
  } else {
    return 1;
  }
}
sub ITER_PARENT {
  return undef;
}

use strict;
use warnings;
use Gtk2 '-init';
use Data::Dumper;

my $model = MyModel->new;

$model->signal_connect
  (rows_reordered => sub {
     print "reordered signal:\n";
     print Dumper(\@_);
   });
$model->rows_reordered (Gtk2::TreePath->new_from_indices(99,88,77), undef, 1, 0);
exit 0;

---

---

--- customlist.pl 01 Aug 2004 12:44:05 +1000 1.2
+++ customlist.pl 07 Jul 2008 09:52:14 +1000
@@ -442,6 +442,14 @@
   my $customlist = CustomList->new;
   fill_model ($customlist);
 
+  use Data::Dumper;
+  $customlist->signal_connect (rows_reordered => sub {
+                                 print Dumper(\@_);
+                               },
+                               'my-userdata');
+  $customlist->rows_reordered (Gtk2::TreePath->new, undef,
+                               0 .. $customlist->iter_n_children(undef) - 1);
+
   my $view = Gtk2::TreeView->new ($customlist);
 
   my $renderer = Gtk2::CellRendererText->new;

---

---

--- GtkTreeModel.xs 07 Jul 2008 09:53:48 +1000 1.54
+++ GtkTreeModel.xs 07 Jul 2008 09:54:47 +1000
@@ -42,6 +42,13 @@
  int n_children, i;
  dGPERL_CLOSURE_MARSHAL_ARGS;
 
+ /* If model is a Perl object then gtk_tree_model_iter_n_children()
+   will call out to ITER_N_CHILDREN in the class, so do that before
+   trying to build the stack here. */
+ model = g_value_get_object (param_values);
+ iter = g_value_get_boxed (param_values+2);
+ n_children = gtk_tree_model_iter_n_children (model, iter);
+
  GPERL_CLOSURE_MARSHAL_INIT (closure, marshal_data);
 
  PERL_UNUSED_VAR (return_value);
@@ -55,18 +62,15 @@
 
  /* instance */
  GPERL_CLOSURE_MARSHAL_PUSH_INSTANCE (param_values);
- model = SvGtkTreeModel(instance);
 
  /* treepath */
  XPUSHs (sv_2mortal (gperl_sv_from_value (param_values+1)));
 
  /* treeiter */
  XPUSHs (sv_2mortal (gperl_sv_from_value (param_values+2)));
- iter = g_value_get_boxed (param_values+2);
 
  /* gint * new_order */
  new_order = g_value_get_pointer (param_values+3);
- n_children = gtk_tree_model_iter_n_children (model, iter);
  av = newAV ();
  av_extend (av, n_children-1);
  for (i = 0; i < n_children; i++)

---

---

--- GtkTreeModelIface.t 23 May 2008 09:50:57 +1000 1.6
+++ GtkTreeModelIface.t 07 Jul 2008 10:02:57 +1000
@@ -346,7 +346,7 @@
 
 package main;
 
-use Gtk2::TestHelper tests => 166, noinit => 1;
+use Gtk2::TestHelper tests => 174, noinit => 1;
 use strict;
 use warnings;
 
@@ -393,6 +393,24 @@
 $model->ref_node ($iter);
 $model->unref_node ($iter);
 
+{ my $signal_finished = 0;
+  my $len = @{$model->{data}};
+  my @array = (0 .. $len-1);
+  my $id = $model->signal_connect (rows_reordered => sub {
+                                     my ($s_model, $path, $iter, $aref) = @_;
+                                     is ($s_model, $model);
+                                     isa_ok ($path, "Gtk2::TreePath");
+                                     my @indices = $path->get_indices;
+                                     is_deeply (\@indices, []);
+                                     is ($iter, undef);
+                                     is_deeply ($aref, \@array);
+                                     $signal_finished = 1;
+                                   });
+  $model->rows_reordered (Gtk2::TreePath->new, undef, @array);
+  ok ($signal_finished, 'rows-reordered signal ran');
+  $model->signal_handler_disconnect ($id);
+}
+
 my $sorter_two = sub {
  my ($list, $a, $b, $data) = @_;
 

---

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[muppet]

On Jul 6, 2008, at 8:09 PM, Kevin Ryde wrote:

> I wonder if it might be something like
>
> * xs/GtkTreeModel.xs (gtk2perl_tree_model_rows_reordered_marshal):
> Call gtk_tree_model_iter_n_children before building the stack, in  
> case
> it's a Perl class and calls out to ITER_N_CHILDREN.  This fixes
> clobbering of instance and treepath args to a Perl rows-reordered
> handler connected on a Perl model.
> * t/GtkTreeModelIface.t: Exercise rows-reordered marshalling.
>
> <
> reordered
> .pl
> >
> <
> customlist
> .pl
> .reorder
> .diff><GtkTreeModel.xs.reorder.diff><GtkTreeModelIface.t.reorder.diff>

While i'm mildly surprised that this happens (suggesting that POPi  
doesn't clean up like you'd expect), i think your changes avoid the  
issue quite nicely.  Torsten?


--
It's all very complicated and would take a scientist to explain it.
   -- MST3K


_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

muppet <scott@...> writes:
>
> While i'm mildly surprised that this happens (suggesting that POPi
> doesn't clean up like you'd expect),

It wouldn't be to do with PUTBACK would it?  That the sp updated for the
pushes so far hasn't been stored back to its global at the point the
iter_n_children is called ...

(But I'm pretty fuzzy on that.  Usually I can only remember the rules
for about a day at a time, then have to try to read it all again ... :-)
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

There might be another similar bit in gtk_tree_model_get too.  Failing
program below, getting

        Bizarre copy of CODE in aassign at multi-get.pl line 79.

The case where you give explicit column numbers seems ok.  I suppose the
sp global is already past where the local XPUSHs is writing.  But do
both cases need an SPAGAIN in case the stack has been extended deep
within the get_value and/or get_n_columns?  Or for that matter the
iter_n_children in the marshal too?

---

package MyModel;
use strict;
use warnings;
use Gtk2;

use Glib::Object::Subclass
  Glib::Object::,
  interfaces => [ Gtk2::TreeModel:: ];

use constant LEN => 2;

sub INIT_INSTANCE {
  my ($self) = @_;
}
sub GET_FLAGS {
  return [ 'list-only' ];
}
sub GET_N_COLUMNS {
  return 10;
}
sub GET_COLUMN_TYPE {
  return 'Glib::String';
}
sub GET_ITER {
  my ($self, $path) = @_;
  if ($path->get_depth != 1) { return undef; }
  my ($index) = $path->get_indices;
  if ($index >= LEN) { return undef; }
  return [ 123, $index, undef, undef ];
}
sub GET_PATH {
  my ($self, $iter) = @_;
  return Gtk2::TreePath->new_from_indices ($iter->[1]);
}
sub ITER_N_CHILDREN {
  print "ITER_N_CHILDREN\n";
  return 2;
}
sub ITER_NTH_CHILD {
  my ($self, $iter, $n) = @_;
  if (defined $iter) { return undef; }
  if ($n >= 2) { return undef; }
  return [ 123, $n, undef, undef ];
}
sub ITER_HAS_CHILD {
  my ($self, $iter) = @_;
  if (defined $iter) {
    return 0;  # nothing under rows
  } else {
    return 1;
  }
}
sub ITER_PARENT {
  return undef;
}
my @row = (100,200,300,400,500,600,700,800,900,1000);
sub GET_VALUE {
  my ($self, $iter, $col) = @_;
  return $row[$col];
}

use strict;
use warnings;
use Gtk2 '-init';
use Data::Dumper;

my $model = MyModel->new;

# this one ok
if (0) {
  my $iter = $model->get_iter_first;
  my @a = $model->get($iter, 0,2,4,6,8);
  print Dumper(\@a);
}

# but this one gets "Bizarre copy of CODE in aassign" error
{
  my $iter = $model->get_iter_first;
  my @a = $model->get($iter);
  print Dumper(\@a);
}

exit 0;

---

---

--- GtkTreeModel.xs 07 Jul 2008 09:53:48 +1000 1.54
+++ GtkTreeModel.xs 11 Jul 2008 11:26:29 +1000
@@ -1106,12 +1106,17 @@
  else
  {
  /* otherwise return all of the columns */
- for( i = 0; i < gtk_tree_model_get_n_columns(tree_model); i++ )
+ gint columns;
+ PUTBACK;
+ columns = gtk_tree_model_get_n_columns(tree_model);
+ for( i = 0; i < columns; i++ )
  {
  GValue gvalue = {0, };
  gtk_tree_model_get_value (tree_model, iter,
                           i, &gvalue);
+ SPAGAIN;
  XPUSHs (sv_2mortal (gperl_sv_from_value (&gvalue)));
+ PUTBACK;
  g_value_unset (&gvalue);
  }
  }

---

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Kevin Ryde wrote:

> With a perl custom treemodel and a perl connected rows-reordered signal,
> there seems to be a gremlin in the way $model->rows_reordered reaches
> the signal handler.  [...
>
> I wonder if it might be something like
>
> * xs/GtkTreeModel.xs (gtk2perl_tree_model_rows_reordered_marshal):
> Call gtk_tree_model_iter_n_children before building the stack, in case
> it's a Perl class and calls out to ITER_N_CHILDREN.  This fixes
> clobbering of instance and treepath args to a Perl rows-reordered
> handler connected on a Perl model.
>   * t/GtkTreeModelIface.t: Exercise rows-reordered marshalling.

Yeah, I think you're right.  When we call out to
gtk_tree_model_iter_n_children and thus eventually ITER_N_CHILDREN, our local
stack *pointer* is not modified (since it's local), but the actual stack
contents are, and this is what you saw.

Committed to both branches.  Thanks!  (Unfortunately, I didn't see the
ChangeLog entry you wrote up early enough to use it for the commit.  Sorry.)

--
Bye,
-Torsten
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Kevin Ryde wrote:
> There might be another similar bit in gtk_tree_model_get too.

OK, I just sat down and I think I figured it out.  If you are *really*
interested, I attach two files: perl-stack-fiddling lists the expansions of the
relevant macros[1], and perl-stack-fiddling-tree-model-get-all is a "trace" of
what happens to the relevant stacks[2] at each point in the execution of
$model->get($iter).

It all boils down to this: when Perl code is invoked, for example by
gtk2perl_tree_model_get_value, the arguments for the Perl code are put on the
stack *after the element pointed to by the global stack pointer*.  That is,
everything after the global stack pointer is the scratch area used by the
calling code to marshal arguments in and out of Perl code.  So when you put
stuff on the stack in some XSUB, and there's a chance that some code is going to
invoke Perl code before you return from the XSUB, make sure you update the
global stack pointer to point to the element you just put on the stack.

In practical terms: after every XPUSHs or similar, use PUTBACK if it's possible
that some Perl code is invoked before you return from the XSUB.

I hope I can remember all this because I don't want to have to think through it
again.

> The case where you give explicit column numbers seems ok.  I suppose the
> sp global is already past where the local XPUSHs is writing.  But do
> both cases need an SPAGAIN in case the stack has been extended deep
> within the get_value and/or get_n_columns?

In practice, Gtk2::TreeModel::get with explicit column numbers is safe since
there are always more input arguments than return values, and so the stuff we
put on the stack can't end up being overwritten.  But a PUTBACK can't hurt
either, I think.  The get_n_columns() call is harmless since we haven't put
anything on the stack yet that might be overwritten.  So, I think the attached
patch should do it.

> Or for that matter the iter_n_children in the marshal too?

I don't see how iter_n_children is affected by this -- can you elaborate?

Does all of the above make sense?

--
Bye,
-Torsten

[1] Created with Porting/expand-macro.pl from bleadperl.
[2] Actually, I think the markstack stuff isn't relevant after all.  So maybe
just ignore that.

dSP
        SV **sp = (my_perl->Istack_sp)

dXSARGS
        SV **sp = (my_perl->Istack_sp);
        I32 ax = (*(my_perl->Imarkstack_ptr)--);
        SV **mark = (my_perl->Istack_base) + ax++;
        I32 items = (I32) (sp - mark)

PUSHMARK(A0)
        (void) ({
                if (++(my_perl->Imarkstack_ptr) == (my_perl->Imarkstack_max))
                        Perl_markstack_grow (my_perl);
                *(my_perl->Imarkstack_ptr) = (I32) ((A0) - (my_perl->Istack_base));
        })

ST(A0)
        (my_perl->Istack_base)[ax + (A0)]

PUSHs(A0)
        (*++sp = (A0))

XPUSHs(A0)
        (void) ({
                (void) ({
                        if ((my_perl->Istack_max) - sp < (int) (1)) {
                                sp = Perl_stack_grow (my_perl, sp, sp, (int) (1));
                        }
                });
                (*++sp = (A0));
        })

POPs
        (*sp--)

PUTBACK
        (my_perl->Istack_sp) = sp

SPAGAIN
        sp = (my_perl->Istack_sp)

my @a = $m->get($i)

XS(XS_Gtk2__TreeModel_get)

  markstack: ... | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
                   ^sp
                   ^PL_stack_sp

dXSARGS -> PL_markstack_ptr--

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
                   ^sp
                   ^PL_stack_sp

PPCODE -> sp -= items

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
           ^sp
                   ^PL_stack_sp

gtk_tree_model_get_n_columns (tree_model)

gtk2perl_tree_model_get_n_columns

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
                   ^sp
                   ^PL_stack_sp

PUSHMARK (SP)

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
                   ^y
                   ^sp
                   ^PL_stack_sp

PUSHs (m)

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | m | ...
           ^x
                   ^y
                       ^sp
                   ^PL_stack_sp

PUTBACK

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | m | ...
           ^x
                   ^y
                       ^sp
                       ^PL_stack_sp

call_method("GET_N_COLUMNS") -> GET_N_COLUMNS(m) -> returns n

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | n | ...
           ^x
                   ^y
                       ^sp
                       ^PL_stack_sp

SPAGAIN

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | n | ...
           ^x
                   ^y
                       ^sp
                       ^PL_stack_sp

POPi

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i |   | ...
           ^x
                   ^y
                   ^sp
                       ^PL_stack_sp

PUTBACK

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i |   | ...
           ^x
                   ^y
                   ^sp
                   ^PL_stack_sp

back in XS(XS_Gtk2__TreeModel_get)

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
                   ^y
           ^sp
                   ^PL_stack_sp

gtk_tree_model_get_value (tree_model, iter, 0, &gvalue)

gtk2perl_tree_model_get_value (tree_model, iter, 0, &gvalue)

  markstack: ... | y | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
                   ^y
                   ^sp
                   ^PL_stack_sp

PUSHMARK (SP)

  markstack: ... | z | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
                   ^z
                   ^sp
                   ^PL_stack_sp

PUSHs (m), PUSHs (i), PUSHs (0)

  markstack: ... | z | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | m | i | 0 | ...
           ^x
                   ^z
                               ^sp
                   ^PL_stack_sp

PUTBACK

  markstack: ... | z | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | m | i | 0 | ...
           ^x
                   ^z
                               ^sp
                               ^PL_stack_sp

call_method("GET_VALUE") -> GET_VALUE(m, i, 0) -> returns v

  markstack: ... | z | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | v |   |   | ...
           ^x
                   ^z
                               ^sp
                       ^PL_stack_sp

SPAGAIN

  markstack: ... | z | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i | v |   |   | ...
           ^x
                   ^z
                       ^sp
                       ^PL_stack_sp

POPs

  markstack: ... | z | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i |   |   |   | ...
           ^x
                   ^z
                   ^sp
                       ^PL_stack_sp

PUTBACK

  markstack: ... | z | x | ...
                   ^PL_markstack_ptr

  stack: ... | m | i |   |   |   | ...
           ^x
                   ^z
                   ^sp
                   ^PL_stack_sp

back in XS(XS_Gtk2__TreeModel_get)

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | m | i | ...
           ^x
           ^sp
                   ^PL_stack_sp
XPUSHs(v)

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | v | i | ...
           ^x
               ^sp
                   ^PL_stack_sp

in the next iteration, after the call to gtk_tree_model_get_value returned:

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | v | i | ...
           ^x
               ^sp
                   ^PL_stack_sp
XPUSHs(v)

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | v | v | ...
           ^x
                   ^sp
                   ^PL_stack_sp

in the next iteration, after the call to gtk_tree_model_get_value returned:

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | v | v | ...
           ^x
                   ^sp
                   ^PL_stack_sp
XPUSHs(v)

  markstack: ... | x | ...
               ^PL_markstack_ptr

  stack: ... | v | v | v | ...
           ^x
                       ^sp
                   ^PL_stack_sp

When we now call into gtk_tree_model_get_value, the shit hits the fan.  The
marshaling code overwrite everything beyond PL_stack_sp and so nukes everything
after and including the third value we pushed on the stack.

Index: xs/GtkTreeModel.xs
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/xs/GtkTreeModel.xs,v
retrieving revision 1.57
diff -u -d -p -r1.57 GtkTreeModel.xs
--- xs/GtkTreeModel.xs 18 Aug 2008 20:28:44 -0000 1.57
+++ xs/GtkTreeModel.xs 24 Aug 2008 17:10:46 -0000
@@ -1132,26 +1132,29 @@ gtk_tree_model_get (tree_model, iter, ..
  int i;
     PPCODE:
  PERL_UNUSED_VAR (ix);
- /* if column id's were passed, just return those columns */
- if( items > 2 )
- {
+ if (items > 2) {
+ /* if column id's were passed, just return those columns */
  for (i = 2 ; i < items ; i++) {
  GValue gvalue = {0, };
  gtk_tree_model_get_value (tree_model, iter,
                           SvIV (ST (i)), &gvalue);
  XPUSHs (sv_2mortal (gperl_sv_from_value (&gvalue)));
+ // Update the global stack pointer so that other xsubs
+ // don't overwrite what we put on the stack.
+ PUTBACK;
  g_value_unset (&gvalue);
  }
- }
- else
- {
+ } else {
  /* otherwise return all of the columns */
- for( i = 0; i < gtk_tree_model_get_n_columns(tree_model); i++ )
- {
+ int n_columns = gtk_tree_model_get_n_columns (tree_model);
+ for (i = 0; i < n_columns; i++) {
  GValue gvalue = {0, };
  gtk_tree_model_get_value (tree_model, iter,
                           i, &gvalue);
  XPUSHs (sv_2mortal (gperl_sv_from_value (&gvalue)));
+ // Update the global stack pointer so that other xsubs
+ // don't overwrite what we put on the stack.
+ PUTBACK;
  g_value_unset (&gvalue);
  }
  }

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

Torsten Schoenfeld <kaffeetisch@...> writes:
>
> In practical terms: after every XPUSHs or similar, use PUTBACK if it's
> possible that some Perl code is invoked before you return from the
> XSUB.

I believe in addition: and do an SPAGAIN after that perl code, because
it could move the whole stack, completely invalidating your local SP
pointer.

An all-perl failing case for that below.  Remove the grow_the_stack()
call to see it become ok.

> So, I think the attached patch should do it.

Except it doesn't work!  :-)

Looking at the generated .c, the first thing PPCODE does is "SP -=
items", so in the supplied-columns case the PUTBACK sets the global
position to overwrite the "..." args yet unprocessed.

> I don't see how iter_n_children is affected by this -- can you elaborate?

Umm, it's been a while.  I think it's another stack-grow SPAGAIN, like
g_object_get in other message.

> Does all of the above make sense?

I guess it's not meant to be quite so difficult.  If you keep a local
copy of the pointer then you have to be careful to sync with the global
when anything might read, write or move that global.

The bit that gets me confused is which macros use or don't use the local
SP.  All the PUSH/POP use it, but XSRETURN and XST_ don't.  And PPCODE:
needs it valid at the end of your xsub, but CODE: doesn't.  Or something
like that.  :-(


Index: GtkTreeModelIface.t
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/t/GtkTreeModelIface.t,v
retrieving revision 1.8
diff -u -u -r1.8 GtkTreeModelIface.t
--- GtkTreeModelIface.t 18 Aug 2008 20:28:44 -0000 1.8
+++ GtkTreeModelIface.t 25 Aug 2008 00:21:12 -0000
@@ -346,7 +346,7 @@
 
 package main;
 
-use Gtk2::TestHelper tests => 176, noinit => 1;
+use Gtk2::TestHelper tests => 177, noinit => 1;
 use strict;
 use warnings;
 
@@ -475,4 +475,36 @@
              'iter->set() from another iter');
 }
 
+package AnotherCustomList;
+use strict;
+use warnings;
+use Gtk2;
+
+use Glib::Object::Subclass
+  Glib::Object::,
+  interfaces => [ Gtk2::TreeModel:: ];
+
+sub GET_FLAGS       { return [ 'list-only' ]; }
+sub GET_N_COLUMNS   { return 1; }
+sub GET_COLUMN_TYPE { return 'Glib::Int'; }
+sub GET_ITER        { return [ 123, 0, undef, undef ]; }
+
+my @bigarray;
+for (my $i = 0; $i < 500; $i++) { $bigarray[$i] = 9; }
+sub grow_the_stack { myfunc (@bigarray); }
+sub myfunc { }
+
+sub GET_VALUE {
+  my ($self, $iter, $column) = @_;
+  grow_the_stack();
+  return 123;
+}
+
+package main;
+
+$model = AnotherCustomList->new;
+$iter = $model->get_iter_first;
+is ($model->get($iter), 123);
+
+
 # vim: set syntax=perl :


--
Two birds were sitting in a tree when an air force jet flew overhead low
and fast.  "Strewth, did you see that big bird, it was really moving."
The other said, "Yeah, you'd be moving too if your arse was on fire."

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Kevin Ryde wrote:
> Torsten Schoenfeld <kaffeetisch@...> writes:
>> In practical terms: after every XPUSHs or similar, use PUTBACK if it's
>> possible that some Perl code is invoked before you return from the
>> XSUB.
>
> I believe in addition: and do an SPAGAIN after that perl code, because
> it could move the whole stack, completely invalidating your local SP
> pointer.

Ah, I didn't think of the possibility that the whole stack might be moved.  And
strangely enough, it doesn't seem to happen for me.  Your test case passes for
me with only PUTBACK and no SPAGAIN, even when I increase the artificial stack
size from 500 to 500000.  But I see why it might fail: when the requested stack
size exceeds the size currently allocated, perl calls something like realloc()
which might move the whole block.

So I applied your patch.  Thanks!

>> I don't see how iter_n_children is affected by this -- can you elaborate?
>
> Umm, it's been a while.  I think it's another stack-grow SPAGAIN, like
> g_object_get in other message.

I think the iter_n_children case was fixed by moving the code that might end up
overwriting the stack above the code that modifies the local stack pointer.

>> Does all of the above make sense?
>
> I guess it's not meant to be quite so difficult.  If you keep a local
> copy of the pointer then you have to be careful to sync with the global
> when anything might read, write or move that global.

Yeah.  The new rule of thumb then seems to be: if there is a chance that someone
might overwrite the stack before you return from the xsub, call SPAGAIN before
and PUTBACK after any code that modifies the local stack pointer.

--
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Torsten Schoenfeld wrote:

> Ah, I didn't think of the possibility that the whole stack might be moved.  And
> strangely enough, it doesn't seem to happen for me.  Your test case passes for
> me with only PUTBACK and no SPAGAIN, even when I increase the artificial stack
> size from 500 to 500000.  But I see why it might fail: when the requested stack
> size exceeds the size currently allocated, perl calls something like realloc()
> which might move the whole block.

I adjusted the test case slightly and now see the stack movement too.

> Yeah.  The new rule of thumb then seems to be: if there is a chance that someone
> might overwrite the stack before you return from the xsub, call SPAGAIN before
> and PUTBACK after any code that modifies the local stack pointer.

Or, perhaps better: in a PPCODE xsub which modifies the local stack pointer, use
PUTBACK before and SPAGAIN after any code that might modify the global stack
pointer.
--
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

Torsten Schoenfeld <kaffeetisch@...> writes:
>
> So I applied your patch.

I suspect it's still not quite right, I've got doubts about the ST() in
the ->get_value() supplied-columns case, the same as was wrong with what
I first tried for g_object_get.

> I think the iter_n_children case was fixed by moving the code that
> might end up overwriting the stack above the code that modifies the
> local stack pointer.

I can't remember at all now what I was looking at there.

> I adjusted the test case slightly and now see the stack movement too.

A tricky bit it trying to write something that doesn't use up the stack
too soon.  If it's already grown then obviously you don't get a problem.
Might have to stick the subtler test cases in separate files to be
confident of provoking the problem.
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Kevin Ryde wrote:
> Torsten Schoenfeld <kaffeetisch@...> writes:
>> So I applied your patch.
>
> I suspect it's still not quite right, I've got doubts about the ST() in
> the ->get_value() supplied-columns case, the same as was wrong with what
> I first tried for g_object_get.

Oh, jeez.  You're right.  Patch with failing test case and a fix attached.  Does
this look good to you?

I wonder how many stack handling bugs like this still hide in our PPCODE sections.

-Torsten

Index: t/GtkTreeModelIface.t
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/t/GtkTreeModelIface.t,v
retrieving revision 1.10
diff -u -d -p -r1.10 GtkTreeModelIface.t
--- t/GtkTreeModelIface.t 31 Aug 2008 16:46:08 -0000 1.10
+++ t/GtkTreeModelIface.t 6 Sep 2008 12:23:32 -0000
@@ -346,7 +346,7 @@ sub sort {
 
 package main;
 
-use Gtk2::TestHelper tests => 177, noinit => 1;
+use Gtk2::TestHelper tests => 178, noinit => 1;
 use strict;
 use warnings;
 
@@ -508,7 +508,11 @@ use warnings;
 
 $model = StackTestModel->new;
 is_deeply ([ $model->get ($model->get_iter_first) ],
-           \@StackTestModel::ROW,
+           [ @StackTestModel::ROW ],
            '$model->get ($iter) does not result in stack corruption');
 
+is_deeply ([ $model->get ($model->get_iter_first, reverse 0 .. 9) ],
+           [ reverse @StackTestModel::ROW ],
+           '$model->get ($iter, @columns) does not result in stack corruption');
+
 # vim: set syntax=perl :
Index: xs/GtkTreeModel.xs
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/xs/GtkTreeModel.xs,v
retrieving revision 1.59
diff -u -d -p -r1.59 GtkTreeModel.xs
--- xs/GtkTreeModel.xs 31 Aug 2008 16:46:08 -0000 1.59
+++ xs/GtkTreeModel.xs 6 Sep 2008 12:23:32 -0000
@@ -1130,35 +1130,50 @@ gtk_tree_model_get (tree_model, iter, ..
  get_value = 1
     PREINIT:
  int i;
-    PPCODE:
+    CODE:
+ /* we use CODE: instead of PPCODE: so we can handle the stack
+ * ourselves. */
  PERL_UNUSED_VAR (ix);
- /* The code below uses PUTBACK and SPAGAIN to synchronize the local
- * with the global stack pointer.  This is done so that other xsubs
- * (invoked indirectly by gtk_tree_model_get_value) don't overwrite
- * what we put on the stack. */
- if (items > 2) {
+#define OFFSET 2
+ if (items > OFFSET) {
  /* if column id's were passed, just return those columns */
- for (i = 2 ; i < items ; i++) {
+
+ /* the stack is big enough already due to the input arguments,
+ * so we don't need to extend it.  nor do we need to care about
+ * xsubs called by gtk_tree_model_get_value overwriting the
+ * stuff we put on the stack. */
+ for (i = OFFSET ; i < items ; i++) {
  GValue gvalue = {0, };
- PUTBACK;
  gtk_tree_model_get_value (tree_model, iter,
                           SvIV (ST (i)), &gvalue);
- SPAGAIN;
- XPUSHs (sv_2mortal (gperl_sv_from_value (&gvalue)));
+ ST (i - OFFSET) = sv_2mortal (gperl_sv_from_value (&gvalue));
  g_value_unset (&gvalue);
  }
- } else {
+ XSRETURN (items - OFFSET);
+ }
+#undef OFFSET
+
+ else {
  /* otherwise return all of the columns */
+
  int n_columns = gtk_tree_model_get_n_columns (tree_model);
+ /* extend the stack so it can handle 'n_columns' items in
+ * total.  the stack already contains 'items' elements so make
+ * room for 'n_clumns - items' more, move our local stack
+ * pointer forward to the new end, and update the global stack
+ * pointer.  this way, xsubs called by gtk_tree_model_get_value
+ * don't overwrite what we put on the stack. */
+ EXTEND (SP, n_columns - items);
+ SP += n_columns - items;
+ PUTBACK;
  for (i = 0; i < n_columns; i++) {
  GValue gvalue = {0, };
- PUTBACK;
  gtk_tree_model_get_value (tree_model, iter,
                           i, &gvalue);
- SPAGAIN;
- XPUSHs (sv_2mortal (gperl_sv_from_value (&gvalue)));
+ ST (i) = sv_2mortal (gperl_sv_from_value (&gvalue));
  g_value_unset (&gvalue);
  }
+ XSRETURN (n_columns);
  }
 
  ## va_list means nothing to a perl developer, it's a c-specific thing.

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Torsten Schoenfeld wrote:
>> I suspect it's still not quite right, I've got doubts about the ST() in
>> the ->get_value() supplied-columns case, the same as was wrong with what
>> I first tried for g_object_get.
>
> Oh, jeez.  You're right.  Patch with failing test case and a fix attached.

Committed.

-Torsten
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

Torsten Schoenfeld <kaffeetisch@...> writes:
>
> Does this look good to you?

Pretty close.

> I wonder how many stack handling bugs like this still hide in our
> PPCODE sections.

Until I realized the plain CODE: ones should be ok I thought it could be
very scary, what with the gazillion callbacks that can get back to perl
from a gtk func.  But if it's only PPCODE then it might be merely a bit
of a pain :-)

>   int n_columns = gtk_tree_model_get_n_columns (tree_model);
> + /* extend the stack so it can handle 'n_columns' items in
> + * total.  the stack already contains 'items' elements so make
> + * room for 'n_clumns - items' more, move our local stack
> + * pointer forward to the new end, and update the global stack
> + * pointer.  this way, xsubs called by gtk_tree_model_get_value
> + * don't overwrite what we put on the stack. */
> + EXTEND (SP, n_columns - items);

gtk_tree_model_get_n_columns can callback on a custom model, so I think
an SPAGAIN.


--- GtkTreeModel.xs 08 Sep 2008 10:47:39 +1000 1.60
+++ GtkTreeModel.xs 08 Sep 2008 10:49:28 +1000
@@ -1163,6 +1163,7 @@
  * pointer forward to the new end, and update the global stack
  * pointer.  this way, xsubs called by gtk_tree_model_get_value
  * don't overwrite what we put on the stack. */
+ SPAGAIN;
  EXTEND (SP, n_columns - items);
  SP += n_columns - items;
  PUTBACK;


--
The sigfile one-line movie reviews series:
"Spotswood" -- the greatest film Anthony Hopkins ever made in Spotswood.

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list