Search code examples
perlxs

Passing an array reference to a perl callback using call_sv(), should I use newRV() or newRV_noinc()?

I have this XS code (XsTest.xs):

#define PERL_NO_GET_CONTEXT
#include "EXTERN.h"
#include "perl.h"
#include "XSUB.h"

MODULE = My::XsTest  PACKAGE = My::XsTest
PROTOTYPES: DISABLE

void
foo( callback )
       SV *callback
    PREINIT:
        AV *array;
        SSize_t array_len;
        SV **sv_ptr;
        SV *sv;
        double value;
    CODE:
        if ( !SvROK(callback) ) {
            croak("Not a reference!");
        }
        if ( SvTYPE(SvRV(callback)) != SVt_PVCV ) {
            croak("Not a code reference!");
        }
        /* This array will go out of scope (and be freed) at the end of this XSUB 
         * due to the sv_2mortal()
         */
        array = (AV *)sv_2mortal((SV *)newAV()); /* Line #28 */
        /* NOTE: calling dSP is not necessary for an XSUB, since it has 
         * already been arranged for by xsubpp by calling dXSARGS 
         */
        printf( "Line #28: SvREFCNT(array) = %d\n", SvREFCNT(array));
        ENTER;
        SAVETMPS;
        PUSHMARK(SP);
        EXTEND(SP, 1);
        /* Should I use newRV_inc() or newRV_noinc() here? Or does it not 
         * matter?
         * NOTE: XPUSHs mortalizes the RV (so we do not need to call sv_2mortal()
         */
        XPUSHs((SV *)newRV_inc((SV *) array)); /* Line #41 */
        printf( "Line #41: SvREFCNT(array) = %d\n", SvREFCNT(array));
        PUTBACK;
        call_sv(callback, G_VOID);
        printf( "Line #45: SvREFCNT(array) = %d\n", SvREFCNT(array)); /* Line #45: */
        array_len = av_top_index(array) + 1;
        printf( "Array length: %ld\n", array_len );
        if ( array_len != 1 ) {
            croak( "Unexpected array size: %ld", array_len );
        }
        sv_ptr = av_fetch( array, 0, 0 );
        sv = *sv_ptr;  
        if (SvTYPE(sv) >= SVt_PVAV) {
            croak("Not a scalar value!");
        } 
        value = SvNV(sv);
        printf( "Returned value: %g\n", value);
        FREETMPS;  /* Line # 58 */
        LEAVE;
        printf( "Line #60: SvREFCNT(array) = %d\n", SvREFCNT(array));

I trying to figure out whether to use newRV_inc() or newRV_noinc() at line #41.

The Perl callback is defined in the test script p.pl:

use strict;
use warnings;
use ExtUtils::testlib;
use My::XsTest;

sub callback {
    my ( $ar ) = @_;
    $ar->[0] = 3.12;
}
My::XsTest::foo( \&callback );

The output from running p.pl is:

Line #28: SvREFCNT(array) = 1
Line #41: SvREFCNT(array) = 2
Line #45: SvREFCNT(array) = 2
Array length: 1
Returned value: 3.12
Line #60: SvREFCNT(array) = 2

As far as I can see, if I use newRV_inc():

  • The reference count of array is set to 1 at line #28 when calling newAV(),
  • then it is decreased to zero also at line #28 when calling sv_2mortal() on the same array,
  • at line #41 I create a reference using newRV_inc() and the reference count of array is increased back again from 0 to 1 (due to the _inc in newRV_inc()),
  • at line #58, the FREETMPS macro is called, but this does not affect (?) the refcount of array since it was created outside the SAVETMPS boundary we set up for the callback temporaries. On the other hand, the reference we pushed at line #41 is freed here (since it was made mortal), which causes it to relinquish its ownership of array and hence the reference count of array will be decreased to zero again.
  • at line #60, the XSUB exits and array will be freed (on a following call by the perl runop loop to FREETMPS) since it has reference count of zero. All the scalars in the array will also be freed at that point (?).

The problem with the above reasoning is that it does not agree with the output from SvREFCNT() as shown above. According to the output, the reference count of array is 2 (and not 1) at exit.

What is going on here?

Additional files to reproduce:

lib/My/XsTest.pm:

package My::XsTest;
use strict;
use warnings;
use Exporter qw(import);
our %EXPORT_TAGS = ( 'all' => [ qw(    ) ] );
our @EXPORT_OK = ( @{ $EXPORT_TAGS{'all'} } );
our @EXPORT = qw(    );
our $VERSION = 0.01;
require XSLoader;

XSLoader::load();
1;

Makefile.PL:

use 5.028001;
use strict;
use warnings;
use utf8;
use ExtUtils::MakeMaker 7.12; # for XSMULTI option

WriteMakefile(
  NAME          => 'My::XsTest',
  VERSION_FROM  => 'lib/My/XsTest.pm',
  PREREQ_PM     => { 'ExtUtils::MakeMaker' => '7.12' },
  ABSTRACT_FROM => 'lib/My/XsTest.pm',
  AUTHOR        => 'Håkon Hægland <[email protected]>',
  OPTIMIZE      => '',  # e.g., -O3 (for optimize), -g (for debugging)
  XSMULTI       => 0,
  LICENSE       => 'perl',
  LIBS          => [''], # e.g., '-lm'
  DEFINE        => '', # e.g., '-DHAVE_SOMETHING'
  INC           => '-I.', # e.g., '-I. -I/usr/include/other'
)

Compilation

To compile the module, run:

perl Makefile.PL
make
Solution

  • You should use newRV_inc().

    Your actual problem is that you are creating a new RV which leaks. The fact that the RV is never freed means that the reference count on array is never decremented. You need to mortalise the return value of newRV_inc().

    One other comment: the reference count of array is not reduced to zero when you mortalise it; it remains as 1. I'm not sure where you got that idea from. What actually happens is that when you call newAV(), you are given an AV with a reference count of one, which is 1 too high. Left as-is, it will leak. sv_2mortal() doesn't change array's ref count, but it does take ownership of one reference, which "corrects" the overall reference count and array will no longer leak.