Hi Nick,
While I was contentedly reviewing the commits from your last excellent pull
request, I found an issue. :)
A binding for a method which might be overridden in a dynamic host and invoked
as a SUPER method must not use vtable dispatch. This is because the vtable
for the host subclass will contain a callback stub which invokes the host
method... which invokes SUPER... which invokes the host method via the
callback stub... which invokes SUPER... which invokes the host method via the
callback stub... and eventually you get a stack overflow.
Here's some code to reproduce the problem:
package MyCloner;
use base qw( Clownfish::Obj );
sub clone { return shift->SUPER::clone; }
my $cloner = MyCloner->new();
print STDERR "Object: $cloner\n";
my $clone = $cloner->clone; # deep recursion segfault
print STDERR "Clone: $clone\n";
Avoiding this problem is the reason that CFC autogenerates an XS binding for
each fresh method implementation in a subclass, instead of only a single XS
binding in the class where the method is novel. It's also why the invocation
in the XS binding avoids the usual Clownfish method call syntax and instead
extracts a specific function pointer from a specific vtable -- i.e. not
`CFISH_Obj_Destroy(self)` but this:
method = CFISH_METHOD_PTR(CFISH_OBJ, CFISH_Obj_Destroy);
method(arg_self);
The reason that the XS bindings for Clone had all that hackery in them was
that I was trying to preserve CFC's autogeneration of bindings for Clone in
subclasses outside of the Clownfish core runtime, yet have e.g. the Perl sub
Clownfish::String::clone() return an actual Clownfish::String rather than a
Perl UTF-8 string SV. The implementation had some bugs and I'm glad it's
getting an overhaul, but I don't think we've got this licked just yet.
Fortunately I think Clone is a corner case and our overall approach remains
sound and compatible with SUPER. We just need to figure out how to override
the autogenerated bindings for Clone for a handful of classes where the CFC's
type mapping causes a host type to be returned rather than the Clownfish
object.
Marvin Humphrey
On Fri, Nov 20, 2015 at 6:05 AM, <[email protected]> wrote:
> Custom XS wrapper for Obj_Clone
>
> Clone should always return a Clownfish object.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/lucy-clownfish/repo
> Commit: http://git-wip-us.apache.org/repos/asf/lucy-clownfish/commit/51866b61
> Tree: http://git-wip-us.apache.org/repos/asf/lucy-clownfish/tree/51866b61
> Diff: http://git-wip-us.apache.org/repos/asf/lucy-clownfish/diff/51866b61
>
> Branch: refs/heads/master
> Commit: 51866b61531fa92aa1f711d69289ea2b42f2dd48
> Parents: cf29eb7
> Author: Nick Wellnhofer <[email protected]>
> Authored: Sun Nov 15 15:17:22 2015 +0100
> Committer: Nick Wellnhofer <[email protected]>
> Committed: Tue Nov 17 18:27:56 2015 +0100
>
> ----------------------------------------------------------------------
> .../perl/buildlib/Clownfish/Build/Binding.pm | 27 ++++++++++----------
> runtime/perl/lib/Clownfish.pm | 23 -----------------
> runtime/perl/t/binding/016-varray.t | 2 +-
> 3 files changed, 14 insertions(+), 38 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/lucy-clownfish/blob/51866b61/runtime/perl/buildlib/Clownfish/Build/Binding.pm
> ----------------------------------------------------------------------
> diff --git a/runtime/perl/buildlib/Clownfish/Build/Binding.pm
> b/runtime/perl/buildlib/Clownfish/Build/Binding.pm
> index 417e966..c4e7939 100644
> --- a/runtime/perl/buildlib/Clownfish/Build/Binding.pm
> +++ b/runtime/perl/buildlib/Clownfish/Build/Binding.pm
> @@ -219,13 +219,6 @@ CODE:
> RETVAL = CFISH_OBJ_TO_SV_NOINC(self);
> }
> OUTPUT: RETVAL
> -
> -SV*
> -_clone(self)
> - cfish_String *self;
> -CODE:
> - RETVAL = CFISH_OBJ_TO_SV_NOINC(CFISH_Str_Clone_IMP(self));
> -OUTPUT: RETVAL
> END_XS_CODE
>
> my $binding = Clownfish::CFC::Binding::Perl::Class->new(
> @@ -439,11 +432,16 @@ END_POD
> $pod_spec->set_synopsis($synopsis);
> $pod_spec->set_description($description);
> $pod_spec->add_method(
> + method => 'Clone',
> + alias => 'clone',
> + );
> + $pod_spec->add_method(
> method => 'Destroy',
> alias => 'DESTROY',
> pod => $destroy_pod,
> );
>
> + my @hand_rolled = qw( Clone );
> my $xs_code = <<'END_XS_CODE';
> MODULE = Clownfish PACKAGE = Clownfish::Obj
>
> @@ -473,6 +471,13 @@ CODE:
> RETVAL = cfish_Obj_is_a(self, target);
> }
> OUTPUT: RETVAL
> +
> +SV*
> +clone(self)
> + cfish_Obj *self;
> +CODE:
> + RETVAL = CFISH_OBJ_TO_SV_NOINC(CFISH_Obj_Clone(self));
> +OUTPUT: RETVAL
> END_XS_CODE
>
> my $binding = Clownfish::CFC::Binding::Perl::Class->new(
> @@ -483,6 +488,7 @@ END_XS_CODE
> alias => 'DESTROY',
> method => 'Destroy',
> );
> + $binding->exclude_method($_) for @hand_rolled;
> $binding->append_xs($xs_code);
> $binding->set_pod_spec($pod_spec);
>
> @@ -501,13 +507,6 @@ sub bind_varray {
> MODULE = Clownfish PACKAGE = Clownfish::Vector
>
> SV*
> -_clone(self)
> - cfish_Vector *self;
> -CODE:
> - RETVAL = CFISH_OBJ_TO_SV_NOINC(CFISH_Vec_Clone(self));
> -OUTPUT: RETVAL
> -
> -SV*
> pop(self)
> cfish_Vector *self;
> CODE:
>
> http://git-wip-us.apache.org/repos/asf/lucy-clownfish/blob/51866b61/runtime/perl/lib/Clownfish.pm
> ----------------------------------------------------------------------
> diff --git a/runtime/perl/lib/Clownfish.pm b/runtime/perl/lib/Clownfish.pm
> index 8ac8530..546eda6 100644
> --- a/runtime/perl/lib/Clownfish.pm
> +++ b/runtime/perl/lib/Clownfish.pm
> @@ -134,21 +134,6 @@ sub error {$Clownfish::Err::error}
> }
>
> {
> - package Clownfish::String;
> - our $VERSION = '0.004000';
> - $VERSION = eval $VERSION;
> -
> - {
> - # Defeat obscure bugs in the XS auto-generation by redefining
> clone().
> - # (Because of how the typemap works for String*,
> - # the auto-generated methods return UTF-8 Perl scalars rather than
> - # actual String objects.)
> - no warnings 'redefine';
> - sub clone { shift->_clone(@_) }
> - }
> -}
> -
> -{
> package Clownfish::Err;
> our $VERSION = '0.004000';
> $VERSION = eval $VERSION;
> @@ -187,14 +172,6 @@ sub error {$Clownfish::Err::error}
> sub get_error {$error}
> }
>
> -{
> - package Clownfish::Vector;
> - our $VERSION = '0.004000';
> - $VERSION = eval $VERSION;
> - no warnings 'redefine';
> - sub clone { CORE::shift->_clone }
> -}
> -
> 1;
>
> __END__
>
> http://git-wip-us.apache.org/repos/asf/lucy-clownfish/blob/51866b61/runtime/perl/t/binding/016-varray.t
> ----------------------------------------------------------------------
> diff --git a/runtime/perl/t/binding/016-varray.t
> b/runtime/perl/t/binding/016-varray.t
> index 6640310..a48589d 100644
> --- a/runtime/perl/t/binding/016-varray.t
> +++ b/runtime/perl/t/binding/016-varray.t
> @@ -29,6 +29,6 @@ $varray->insert(
> tick => 0,
> element => 'elem',
> );
> -$twin = $varray->_clone;
> +$twin = $varray->clone;
> is_deeply( $twin->to_perl, $varray->to_perl, "clone" );
>
>
