Ticket #285 (new bug)

Opened 5 years ago

Last modified 3 years ago

cmp/bitwise/other mmd vtable functions should not go through proxy pmc

Reported by: ronaldws Owned by:
Priority: normal Milestone:
Component: core Version:
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

In rakudo 1 <=> undef was doing the right thing while undef <=> 1 was failing. The first case works because integer.pmc has a multi cmp_num for undef and the VTABLE_get_integer(INTERP, value) call on the Failure value was correctly calling the Failure get_integer vtable method. When I tried to patch undef.pmcI noticed that I was having trouble getting a cmp_num vtable function in that file to call the Failure get_integer vtable method because the value for SELF was an undef pmc rather than a failure pmc. After further digging I concluded that the cmp_num function was being called through a proxy pmc and cmp_num was behaving differently in that regard from add and subtract which did an mmd call through the default pmc. The module lib/Parrot/Pmc2c/PMC/Object.pm generates the vtable forwarding functions and relies on the vtable_method_does_multi subroutine in lib/Parrot/Pmc2c/PMC.pm to decide which vtable methods go through a proxy and which don't.

After dropping by #parrot on Feb 5 I was advised that having bitwise and cmp functions go through a proxy pmc might be an oversight and I was asked to come up with an appropriate test if possible.

The patch included (not attached) below relies on the largely baseless assumption that none of the default pmc vtable mmd functions should go through a proxy from an object when I only seem to have been told that fewer of them should. I thought at least parts of it might be useful if taken fwiw.

The attached patch to t/oo/vtableoverride.t should provide some testing for the desired behavior.

Cheers, Ron

The fwiw parrot patch: Index: lib/Parrot/Pmc2c/PMC.pm =================================================================== --- lib/Parrot/Pmc2c/PMC.pm (revision 36383) +++ lib/Parrot/Pmc2c/PMC.pm (working copy) @@ -328,14 +328,27 @@

return $self->vtable->attrs($methodname)->{write};

}

+my $multi_cmp = qr/cmp(?:_num|_string|_pmc)?/; +my $multi_logical = qr/logical_(?:or|and|xor)/; +my $multi_bitwise = qr/bitwise_(?:or|and|xor|shl|shr|lsr)/; +my $multi_bitwise_str = qr/bitwise_(?:or|and|xor)/; +my $multi_arithmetic = qr/add|subtract|multiply|divide|floor_divide|modulus|pow/; + +my $multi_bitwise_style = qr/(?:$multi_bitwise|repeat)(?:_int)?/o; +my $multi_bitwise_str_style = qr/(?:$multi_bitwise|concat)(?:_str)?/o; +my $multi_arithmetic_style = qr/$multi_arithmetic(?:_int|_float)?/o; + +my $multi_bit_or_arith_style = qr/ + (?:i_)? + (?:$multi_bitwise_style|$multi_bitwise_str_style|$multi_arithmetic_style) +/xo; +

sub vtable_method_does_multi {

my ( $self, $methodname ) = @_;

return 1 if ($methodname =~ m/

- (?:i_)? - (?:add|subtract|multiply|divide|floor_divide|modulus) - (?:_int|_float)? - $/x); + (?:$multi_cmp|$multi_logical|$multi_bit_or_arith_style) + $/xo);

}

sub super_method {

Attachments

vtableoverride.patch Download (2.3 KB) - added by ronaldws 5 years ago.
Patch to test script relevant to feature.
tt285.vtableoverride.diff Download (2.4 KB) - added by jkeenan 3 years ago.
Attempt at updating patch submitted by ronaldws

Change History

Changed 5 years ago by ronaldws

Patch to test script relevant to feature.

  Changed 5 years ago by jkeenan

  • component changed from none to core

Changed 3 years ago by jkeenan

Attempt at updating patch submitted by ronaldws

follow-up: ↓ 3   Changed 3 years ago by jkeenan

Since vtableoverride.patch no longer applies cleanly, I tried to rework the patch and apply it to t/oo/vtableoverride.t. However, I got a test failure:

$ prove  -v t/oo/vtableoverride.t
t/oo/vtableoverride.t .. 
1..18
ok 1 - get_string VTABLE override
ok 2 - attribute sideeffect of get_string
ok 3 - check first does, ok
ok 4 - check second does, ok
ok 5 - no it doesn't
ok 6 - Morph VTABLE override 1
ok 7 - Morph VTABLE override 1
ok 8 - check first does, ok
ok 9 - check second does, ok
ok 10 - no it doesn't
ok 11 - inherited does
ok 12 - :vtable should imply the self parameter
ok 13 - assign to int register calls vtable get_integer override
ok 14 - subtract calls vtable get_integer override
not ok 15 - cmp_num with calls vtable get_integer override
# Have: 0
# Want: 1
ok 16 - can have :vtable :anon
ok 17 - invalid :vtable throws an exception
ok 18 - Override get_pmc_keyed_int without .return - TT #1593
Failed 1/18 subtests 

Test Summary Report
-------------------
t/oo/vtableoverride.t (Wstat: 0 Tests: 18 Failed: 1)
  Failed test:  15
Files=1, Tests=18,  0 wallclock secs 
  ( 0.02 usr  0.01 sys +  0.01 cusr  0.00 csys =  0.04 CPU)
Result: FAIL

Suggestions?

Thank you very much.

kid51

in reply to: ↑ 2   Changed 3 years ago by ronaldws

It is now two years later and it would take me a week or two to get back up to speed on the status of this ticket. In the meantime undef has essentially become obsolete in Rakudo. The currently active equivalents like Int.new() don't have the same problem. There is probably no solid requirement for Perl 6 filled at this point by these patches.

More theoretically/abstractly the idea of the patch is to have cmp behave more like other math functions with respect to its use of proxy pmcs. There may be some performance or simplicity advantage to doing it that way but off the top of my head I am not at all in a position to try and make or even guide that sort of judgement call. Looking back it was chromatic who thought the issue worth further thought  on IRC.

It looks like there was an attempt to apply the patch to the tests without the change to PMC processing which I would not have expected to be workable. Over the next week or two I can probably attend to the nuts and bolts of making the patches applicable but it would help for someone more familiar with the path of parrot development to assess whether basic idea is still of interest.

Thanks, Ron

Note: See TracTickets for help on using tickets.