ingydotnet / inline-c-pm

10 stars 19 forks source link

With perl-5.39.4 and later, XS stack can be 64 bit #104

Open sisyphus opened 1 year ago

sisyphus commented 1 year ago

See https://github.com/Perl/perl5/issues/21523 for some background.

It seems to me that this patch to C.pm accommodates the change correctly and portably:

--- lib/Inline/C.pm_orig    2022-03-03 02:38:00.000000000 +1100
+++ lib/Inline/C.pm 2023-10-01 00:22:46.690094000 +1000
@@ -735,7 +735,7 @@
                 $XS .= <<END;
         PREINIT:
         PerlIO* stream;
-        I32* temp;
+        SSize_t* temp;
         PPCODE:
         temp = PL_markstack_ptr++;
         $function($arg_name_list);
@@ -755,7 +755,7 @@
             else {
                 $XS .= <<END;
         PREINIT:
-        I32* temp;
+        SSize_t* temp;
         PPCODE:
         temp = PL_markstack_ptr++;
         $function($arg_name_list);
@@ -772,7 +772,7 @@
         elsif ($listargs) {
             $XS .= <<END;
         PREINIT:
-        I32* temp;
+        SSize_t* temp;
         CODE:
         temp = PL_markstack_ptr++;
         RETVAL = $function($arg_name_list);

This construct is apparently limited to those XS files that are generated by Inline::C (and Inline::CPP). Is it really needed ? (I guess so - but I'm not knowledgeable on the finer details of the way that XS works.)

Cheers, Rob

sisyphus commented 1 year ago

See https://github.com/Perl/perl5/issues/21523#issuecomment-1742230026 where @tonycoz points out that use of SSize_t will pose a problem with older perls.

Cheers, Rob

sisyphus commented 1 year ago

Is it really needed ?

Well ... the following patch to C.pm allows me to avoid the construct altogether:

--- C.pm_orig   2023-10-02 14:10:25.286594100 +1100
+++ C.pm    2023-10-02 15:18:17.822265000 +1100
@@ -735,6 +735,7 @@
         my $arg_name_list = join(', ', @arg_names);

         if ($return_type eq 'void') {
+
             if ($o->{CONFIG}{_TESTING}) {
                 $XS .= <<END;
         PREINIT:
@@ -756,7 +757,7 @@
         return; /* assume stack size is correct */
 END
             }
-            else {
+            elsif(!defined $Inline::C::TV{$function}) {
                 $XS .= <<END;
         PREINIT:
         I32* temp;
@@ -772,6 +773,23 @@
         return; /* assume stack size is correct */
 END
             }
+            elsif($Inline::C::TV{$function} eq 'void') {
+                $XS .= <<END;
+        CODE:
+        $function($arg_name_list);
+        XSRETURN_EMPTY; /* return empty stack */
+END
+            }
+            elsif($Inline::C::TV{$function} eq 'returning') {
+                $XS .= <<END;
+        CODE:
+        $function($arg_name_list);
+        return; /* assume stack size is correct */
+END
+            }
+            else {
+              die "\$Inline::C::TV{$function} (= $Inline::C::TV{$function}) is defined, but set to neither 'void' nor 'returning'";
+            }
         }
         elsif ($listargs) {
             $XS .= <<END;

But it's a bit of a hack - and requires that %Inline::C::TV is populated in a BEGIN{} block before C.pm is loaded. Let's say I have three Inline::C functions foo, bar and baz - each of which is declared as 'void' Both 'foo and bar are "truly void", but baz needs to return values off the stack. Before Inline::C gets loaded, I can do:

BEGIN {
  my @tv = qw(foo);
  for (@tv) {
    $Inline::C::TV{$_} = 'void';
  }
  my @returning = qw(baz);
  for(@returning) {
    $Inline::C::TV{$_} = 'returning';
  }
};

In the generated XS file, bar still works fine, but uses the current construct (because $Inline::C::TV{bar} is not defined). However foo and baz will successfully avoid the construct.

I actually intend to use this hack. I have XS files (generated by Inline::C) containing numerous void subroutines that use the construct. Early indications are that the hack works fine, and it will be nice to avoid all of that clutter in those XS files of mine.

Cheers, Rob

tonycoz commented 1 year ago

I've pushed a change to blead to make the SSize_t stack offsets optional, so this should pass with a default build of current blead.

If you want to support the larger size markstack you can use the new Stack_off_t type, which is also defined by ppport.h in blead.

If you don't want to use ppport.h you can add code like:

#ifndef PERL_STACK_OFFSET_DEFINED
  typedef I32 Stack_off_t;
  #define PERL_STACK_OFFSET_DEFINED
#endif

and change from using I32 * temp to Stack_off_t *temp;.