Re: [PATCH] Support B::Generate and B::C

Front page | perl.perl5.porters | Postings from August 2010

Re: [PATCH] Support B::Generate and B::C

Thread Previous | Thread Next

From:

Nicholas Clark

Date:

August 30, 2010 06:48

Subject:

Re: [PATCH] Support B::Generate and B::C

Message ID:

20100830134750.GH48531@plum.flirble.org

On Mon, Aug 30, 2010 at 11:49:54AM +0100, Ben Morrow wrote:
> Quoth nick@ccl4.org (Nicholas Clark):
> > 
> > What would be needed for that is something like:
> > 
> > 0: Apply Reini's patch locally
> > 1: In (say) XS::APITest create 3 ops, say
> > 
> >                 ADD
> >                /   \
> >           CONST     CONST
> > 
> >    [linked correctly as the core's parser would at this point. I think that
> >     this means op_first and op_last, not op_next]
> > 
> > 2: Call fold_constants
> 
> Note that you can't call fold_constants at runtime, since it causes an
> assertion failure. This means that (e.g.) B::BINOP->new will assert,

I didn't realise this. However, I could see that OP_mutate and OP_convert
both want to link directly to fold_constants. So I tried to test OP_convert.

Given that OP_convert isn't documented or tested, but seems to be a pretty
much direct copy of Perl_convert in op.c [strange, why not a wrapper?], I
assumed that I needed to call it in the same way as the core does.
From some inspection of core, it seems that it's used for generating listops,
so this:

    my $a = sprintf 42;

seemed to be the simplest way to invoke it there.

Hence I wrote this code to exercise it:

$ cat ../generate.pl
#!perl -w
use strict;

use B::Generate;
use Opcode;

my $i = 0;
my %ops;
$ops{$_} = $i++ for Opcode::opset_to_ops(Opcode::full_opset);

my $constant = B::SVOP->new("const", 0, 42);

# By inspection with gdb, this is what the op looks like when it reaches
# Perl_convert
$constant->next($constant);

printf "Sibling is %s\n",  $constant->sibling;

$constant->convert($ops{'sprintf'}, 0);

__END__

[Note that it takes type as a number, not a name. Unlike most every other
routine]

This didn't work.

It turns out that new_SVOP doesn't return anything. So currently B::Generate
can't generate scalar ops, such as constants. This patch seems to fix it:

--- lib/B/Generate.xs~  2010-08-24 13:53:52.000000000 +0100
+++ lib/B/Generate.xs   2010-08-30 11:56:13.000000000 +0100
@@ -1284,7 +1284,7 @@
         ST(0) = __svop_new(aTHX_ class, type, flags, SvRV(sv));

-void
+SV *
 SVOP_new(class, type, flags, sv)
     SV * class
     SV * type

This gets me to the assertion that you mention, my output now being:

Sibling is B::NULL=SCALAR(0x9fb6b0)
perl5.13.4: op.c:2584: Perl_fold_constants: Assertion `PL_curcop == &PL_compiling' failed.
Aborted

So I commented out the assertion, and rebuilt perl. I've added runloop tracing:

Breakpoint 1, XS_B__OP_convert (cv=0xb69260) at lib/B/Generate.c:1267
1267        dVAR; dXSARGS;
(gdb) c
Continuing.
(../generate.pl:0)      pushmark
(../generate.pl:0)      const(IV(42))
(../generate.pl:0)      sprintf

Program received signal SIGSEGV, Segmentation fault.
0x00000000005644cc in Perl_pp_entersub () at pp_hot.c:2939
2939            return NORMAL;
(gdb) p PL_op
$9 = (OP *) 0x0

*If* fold_constants needs to fold constants, it runs an inferior runloop.
At the end of that, PL_op is NULL.
B::Generate makes no attempt to save PL_op.
Hence I can't see how it *ever* worked.

Even with the above solved, from looking at the code, and thinking about how
it works, there are three more problems I can see with it.

1: For your example of a binop, if I ask B::Generate to generate
   my $six = B::SVOP->new("const", 0, 6);
   my $nine = B::SVOP->new("const", 0, 9);

   my $product = B::BINOP->new("multiply", 0, $six, $nine);

   then what it returns will actually be an OP_CONST, but the XS code will
   bless it into the class B::BINOP. That's not right. BINOPs are different
   structures, so some XS wrappers for BINOPs are going to return garbage or
   SEGV when used on an SVOP.

   This one seems to be fairly easily fixable, but it will cause some surprises
   if people always expect the class of the object returned to match the
   class of constructor called

2: For the example above, *what* do $six and $nine point to after the last
   line?

   If I read the code correctly, if constants have been folded, this line is
   reached in fold_constants()

   op_free(o);

   and the entire original optree is freed. At which point, $six and $nine are
   corrupt. Internally, they still point to the same addresses as ever. But
   that's now freed memory, and they have no way of knowing.

   I can't see an easy solution to this.

3: There is no DESTROY method. Any ops you create leak, unless you
   successfully attach them to somewhere appropriate (which used to have a
   NULL pointer, else you've just shifted the leak around)

So right now, I'm really not sure how useful B::Generate actually is.
It's adding a layer of indirection to the C internals, rather than providing
a layer of abstraction. You end up having to write C in, well more, *through*
Perl. With no control over memory management.

I'm half thinking that if B::Generate wants to fold constants, it would be
better providing the constant folding algorithm in Perl. At least that way
it would keep control over the B:: objects that it is manipulating.

But the other half of me isn't sure if it's worth it. As-is, you have to
understand the C structures to write ops. I wrote a new peephole optimisation
on Friday in a few hours. In C, with some help from *printf, gdb and valgrind.
I think that it would actually have been *harder* to try to do it via an
OO interface in Perl, even without bugs.

Is this a module with no users, and no use case?

Nicholas Clark

Thread Previous | Thread Next