Re: perl module documentation patches, segfault

[Ben Pfaff]

John Darrington <address@hidden> writes:

> On Sun, Feb 01, 2009 at 02:50:01PM -0800, Ben Pfaff wrote:
>      I noticed that the Perl module documentation doesn't say how to
>      tell how you've arrived at the end of the dictionary.  It seems
>      that get_var() returns undef in this case, thus:
[...]
> You're right.  Feel free to commit that.     

Thanks, I pushed that.

>      Second, it seems that get_next_case() does not return a plain
>      array but a reference to one.  I would think that a plain array
>      would be more "natural" Perl, but here is a documentation fix-up
>      in case the choice was intentional:
[...]
> From what I remember I decided to make that a reference, because it
> appeared to be the only way that it would be possible to use a sentinel
> value to indicate the end of the file.  Perhaps this was an erroneous
> assumption, due to my limited knowledge of perl.

perlsyn(1) says:

   Truth and Falsehood
       The number 0, the strings '0' and '', the empty list "()", and "undef"
       are all false in a boolean context. All other values are true.

This means that any of those values could be used as a sentinel,
because none of them would be confused with a nonempty array.

>      Finally, I was working on a script to dump out a system file as
>      text.  Here's what I have so far.  It works, in that it dumps the
>      cases, but it segfaults after it reads the last one:
>      
>          #! /usr/bin/perl
>      
>          use PSPP;
>      
>          use strict;
>          use warnings;
>      
>          my $reader = PSPP::Reader->open ($ARGV[0]);
>          while (my @case = @{$reader->get_next_case ()}) {
>              print join (',', @case), "\n";
>          }
>      
>      Is there a good way to debug this?  Running gdb on perl doesn't
>      seem very helpful, since the backtrace is just this:
>      
>          0x080b8fe9 in Perl_pp_rv2av ()
>          (gdb) bt
>          #0  0x080b8fe9 in Perl_pp_rv2av ()
>          #1  0x080b17a9 in Perl_runops_standard ()
>          #2  0x080ac5d0 in perl_run ()
>          #3  0x08063ddd in main ()
>          (gdb) 
>      
>
> Hmm.  The problem seems to be derefencing something whose referand no
> longer exists.  You should be able to set a breakpoint on the
> get_next_case function in PSPP.xs or running perl under valgrind might
> give some clues. 

I figured it out.  The get_next_case() implementation was
returning a null pointer when cases were exhausted.  Apparently
you are not supposed to do that.

How about the following commit to both fix this and return an
array instead of a reference?

commit 22ee9b2b06bdc68170128a3e7085e84c34a26c84
Author: Ben Pfaff <address@hidden>
Date:   Sun Feb 1 19:00:21 2009 -0800

    perl-module: Make PSPP::Reader::get_next_case() return a list.
    
    PSPP::Reader::get_next_case() was documented to return a list,
    but actually it returned a reference to a list.  This commit
    changes the behavior to match the documentation.
    
    This also fixes the behavior on reading past the end of the file.
    Previously this was documented to return undef but caused a
    segfault in practice.  Now it returns an empty list.

diff --git a/perl-module/PSPP.xs b/perl-module/PSPP.xs
index 2d2c8d9..237ae95 100644
--- a/perl-module/PSPP.xs
+++ b/perl-module/PSPP.xs
@@ -671,32 +671,24 @@ CODE:
 RETVAL
 
 
-SV *
+void
 get_next_case (sfr)
  struct sysreader_info *sfr;
-CODE:
+PPCODE:
  struct ccase *c;
 
- if (! (c = casereader_read (sfr->reader)))
- {
-  RETVAL = 0;
- }
- else
+ if (c = casereader_read (sfr->reader))
  {
   int v;
-  AV *av_case = (AV *) sv_2mortal ((SV *) newAV());
 
+  EXTEND (SP, dict_get_var_cnt (sfr->dict));
   for (v = 0; v < dict_get_var_cnt (sfr->dict); ++v )
     {
       const struct variable *var = dict_get_var (sfr->dict, v);
       const union value *val = case_data (c, var);
 
-      av_push (av_case, value_to_scalar (val, var));
+      PUSHs (sv_2mortal (value_to_scalar (val, var)));
     }
 
   case_unref (c);
-  RETVAL = newRV ((SV *) av_case);
  }
-OUTPUT:
- RETVAL
-
diff --git a/perl-module/lib/PSPP.pm b/perl-module/lib/PSPP.pm
index 93d70ef..e9e3215 100644
--- a/perl-module/lib/PSPP.pm
+++ b/perl-module/lib/PSPP.pm
@@ -493,7 +493,8 @@ This method returns an array of scalars, each of which are 
the values of
 the data in the system file.
 The first call to C<get_next_case> after C<open> has been called retrieves
 the first case in the system file.  Each subsequent call retrieves the next
-case.  If there are no more cases to be read, the function returns undef.
+case.  If there are no more cases to be read, the function returns an empty
+list.
 
 If the case contains system missing values, these values are set to the 
 empty string.
diff --git a/perl-module/t/Pspp.t b/perl-module/t/Pspp.t
index fe14151..030a342 100644
--- a/perl-module/t/Pspp.t
+++ b/perl-module/t/Pspp.t
@@ -387,11 +387,11 @@ RESULT
     print MYFILE "$_ => $vl->{$_}\n" for keys %$vl;
  }
 
- while (my $c = $sf->get_next_case () )
+ while (my @c = $sf->get_next_case () )
  {
     for ($v = 0; $v < $dict->get_var_cnt(); $v++)
     {
-       print MYFILE "val$v: \"@$c[$v]\"\n";
+       print MYFILE "val$v: \"$c[$v]\"\n";
     }
     print MYFILE "\n";
  }
@@ -469,9 +469,9 @@ EOF
 
  my $output = PSPP::Sysfile->new ("$tempdir/out.sav", $dict);
 
- while (my $c = $input->get_next_case () )
+ while (my (@c) = $input->get_next_case () )
  {
-   $output->append_case ($c);
+   $output->append_case (address@hidden);
  }
 
  $output->close ();
@@ -519,10 +519,10 @@ SYNTAX
 
  my $dict = $sf->get_dict ();
 
- my $c = $sf->get_next_case ();
+ my (@c) = $sf->get_next_case ();
 
  my $var = $dict->get_var (0);
- my $val = @$c[0];
+ my $val = $c[0];
  my $formatted = PSPP::format_value ($val, $var);
  my $str = gmtime ($val - PSPP::PERL_EPOCH);
  print "Formatted string is \"$formatted\"\n";
@@ -557,30 +557,30 @@ SYNTAX
  my $dict = $sf->get_dict ();
 
 
- my $c = $sf->get_next_case ();
+ my (@c) = $sf->get_next_case ();
 
  my $stringvar = $dict->get_var (0);
  my $numericvar = $dict->get_var (2);
- my $val = @$c[0];
+ my $val = $c[0];
 
  ok ( !PSPP::value_is_missing ($val, $stringvar), "Missing Value Negative 
String");
 
- $val = @$c[2];
+ $val = $c[2];
 
  ok ( !PSPP::value_is_missing ($val, $numericvar), "Missing Value Negative 
Num");
 
- $c = $sf->get_next_case (); 
- $c = $sf->get_next_case (); 
+ @c = $sf->get_next_case (); 
+ @c = $sf->get_next_case (); 
 
- $val = @$c[0];
+ $val = $c[0];
  ok ( PSPP::value_is_missing ($val, $stringvar), "Missing Value Positive");
 
- $c = $sf->get_next_case (); 
- $val = @$c[2];
+ @c = $sf->get_next_case (); 
+ $val = $c[2];
  ok ( PSPP::value_is_missing ($val, $numericvar), "Missing Value Positive 
SYS");
 
- $c = $sf->get_next_case (); 
- $val = @$c[2];
+ @c = $sf->get_next_case (); 
+ $val = $c[2];
  ok ( PSPP::value_is_missing ($val, $numericvar), "Missing Value Positive 
Num");
 }
 

-- 
RMS on DRM: "This ought to be a crime. And, if we had governments of the
people, by the people, for the people, then the executives of those companies
would be in prison.  But they're not in prison, and the reason is that we have
government of the people, by the sell-outs, for the corporations."