MMD calls the wrong sub

[rgrjr]

The test case is pretty large, so it is attached as a tgz file; smaller test cases (e.g. test-aset.pir) perform normally. That smells like a GC bug to me.

[rgrjr]

Test case showing MMD failure

[rgrjr]

This test code works in r33805, but fails (i.e. reproduces the bug) in r33806. Interestingly, compiling the *.pbc files in r33805 and running them in the r33806 parrot binary also works.

-- Bob

2008-12-11 15:01:13:

revision: 33806; author: tewk [subid] passes test 25 now => /trunk/compilers/imcc/imcc.y: action: M => /trunk/compilers/imcc/imcparser.c: action: M => /trunk/compilers/imcc/imcparser.h: action: M => /trunk/compilers/imcc/pbc.c: action: M => /trunk/compilers/imcc/symreg.h: action: M => /trunk/t/compilers/imcc/syn/subflags.t: action: M => /trunk/t/pmc/key.t: action: M

[rgrjr]

This still fails in r39563. I updated the test case, but Trac won't let me upload it because it is almost 400KB, so I have put it on my Web site:

 http://www.rgrjr.com/perl/bug-39-090614.tgz

As before, you need to (a) unpack the tarball, (b) edit the makefile macros, and (c) type "make" to run the test case. There are several possible outcomes:

1. With the vanilla tarball on my configuration, it gets an unbounded recursion because the MMD sub that specializes on String is never called:

	rogers@rgr> make
	/usr/src/parrot/parrot -o structures.pbc structures.pir
	/usr/src/parrot/parrot -o fdefinition.pbc fdefinition.pir
	/usr/src/parrot/parrot -o stream.pbc stream.pir
	. . .
	/usr/src/parrot/parrot -G kea.pbc
	'foo' is a String.
	length of 'foo' is 3.
	[list-length of a COMMON-LISP::CONS]
	[list-length of a COMMON-LISP::CONS]
	length of 'foo' is 3.
	[list-length of a String]
	[list-length of a String]
	[list-length of a String]
	[list-length of a String]
	[list-length of a String]
	[list-length of a String]
	maximum recursion depth exceeded
	current instr.: 'lisp;COMMON-LISP;LIST-LENGTH' pc 2696 (list.pir:891)
	called from Sub 'lisp;COMMON-LISP;_length' pc 2654 (kea.pir:1139)
	called from Sub 'lisp;COMMON-LISP;WRITE-STRING' pc 1428 (stream.pir:529)
	. . .
	called from Sub 'lisp;COMMON-LISP;STRING=' pc 2720 (string.pir:840)
	called from Sub 'lisp;PARROT;%DEFPACKAGE' pc 1691 (package.pir:616)
	called from Sub 'lisp;COMMON-LISP;file-onload' pc 254 (exports.pir:73)
	called from Sub 'lisp;COMMON-LISP;parrot_load' pc 3976 (kea.pir:1680)
	called from Sub 'lisp;COMMON-LISP;main' pc 4598 (kea.pir:1825)
	make: *** [test] Error 1
	rogers@rgr> 

The two "length of 'foo' is 3." lines show that this dispatch works initially, and then again before loading exports.pbc, but fails during loading of exports.pbc.

2. If you remove the "-G" from makefile line 45, it fails much earlier:

	rogers@rgr> make
	/usr/src/parrot/parrot kea.pbc
	'foo' is a String.
	length of 'foo' is 3.
	Null PMC access in get_pmc_keyed_str()
	current instr.: 'lisp;COMMON-LISP;parrot_load' pc 3976 (kea.pir:1680)
	called from Sub 'lisp;COMMON-LISP;main' pc 4379 (kea.pir:1794)
	make: *** [test] Error 1
	rogers@rgr> 

But this does *not* happen if you take out the ops that generate both "length of 'foo' is 3." messages.

3. If load_bytecode can't find a file, that means it got past the bug on your configuration. Let me know, and we can compare configuration details.

[rgrjr]

I am still seeing the equivalent failure in r49616, but it turns out this is *not* an MMD failure; it fails due to bypassing MMD altogether. The real problem is that r33806 causes ".const 'Sub' ..." to behave differently: When given the name of a multisub in r33805, it loads the register with the multisub, but in r33806 it loads the first component sub, which (if that is not the appropriate method) makes it look like an incorrect dispatch. I can't imagine this change was intended.

The following test case is based on t/pmc/multidispatch.t case 9, with the addition of two other ways (that should be equivalent) of finding the "foo" multi. Please ignore the enormous tarballs. ;-}

#! perl

use strict;
use warnings;
use lib qw( . lib ../lib ../../lib );

use Test::More;
use Parrot::Test::Util 'create_tempfile';

use Parrot::Test tests => 1;

pir_output_is( <<'CODE', <<'OUTPUT', '.const of a multisub gets the multi');
.sub foo :multi(_, Integer)
    .param pmc first
    .param pmc second
    print "(_, Int) method:  "
    print first
    print ', '
    print second
    print "\n"
.end
.sub foo :multi(_, Float)
    .param pmc first
    .param pmc second
    print "(_, Float) method:  "
    print first
    print ', '
    print second
    print "\n"
.end
.sub main :main
    say "Direct calls use the multi."
    $P0 = new ['Float']
    $P0 = 9.5
    foo(1, $P0)
    $P1 = new ['Integer']
    $P1 = 3
    foo(1, $P1)
    say ".const 'Sub' should also use the multi."
    .const 'Sub' foo_const = 'foo'
    print "got "
    print foo_const
    print " type "
    $S0 = typeof foo_const
    say $S0
    foo_const(1, $P0)
    say "And so should get_global."
    .local pmc foo_sub
    foo_sub = get_global 'foo'
    print "got "
    print foo_sub
    print " type "
    $S0 = typeof foo_sub
    say $S0
    foo_sub(1, $P0)
.end
CODE
Direct calls use the multi.
(_, Float) method:  1, 9.5
(_, Int) method:  1, 3
.const 'Sub' should also use the multi.
got foo type MultiSub
(_, Float) method:  1, 9.5
And so should get_global.
got foo type MultiSub
(_, Float) method:  1, 9.5
OUTPUT

[pmichaud]

Replying to rgrjr:

I am still seeing the equivalent failure in r49616, but it turns out this is *not* an MMD failure; it fails due to bypassing MMD altogether. The real problem is that r33806 causes ".const 'Sub' ..." to behave differently: When given the name of a multisub in r33805, it loads the register with the multisub, but in r33806 it loads the first component sub, which (if that is not the appropriate method) makes it look like an incorrect dispatch. I can't imagine this change was intended.

Short answer: The change is intended and is working as designed -- the test file is missing :subid markers that would enable it to work.

Longer answer: r33806 introduces a ":subid" flag for .sub declarations, enabling each sub in a compilation unit to be uniquely identified by something other than its "name", which is often not unique (as in the case of :multi subs). The ".const 'Sub'" declaration now refers to :subid instead of subroutine names. Prior to this change, it was often not feasible to uniquely locate a given Sub PMC -- e.g. to handle lexical scoping or attach other properties to a Sub.

If a .sub declaration doesn't provide a :subid flag, then the name of the sub is used as its :subid. However, a given :subid is also required to be unique within a compilation unit in order to work; the behavior of multiple identical :subid's is undefined. (In the example given above, it appears to have used the first component of the MultiSub.)

The solution is to make sure that each component of a :multi-declared Sub also provides a unique :subid. If this is done, then each component can be uniquely identified, and the MultiSub PMC will have the :subid corresponding to the name. Then ".const 'Sub'" can be used to locate any component of the MultiSub, leave the name to uniquely identify the MultiSub itself:

pmichaud@orange:~/parrot/trunk$ cat x.pir
.sub foo :multi(_, Integer)  :subid('foo_Int')
    .param pmc first
    .param pmc second
    print "(_, Int) method:  "
    print first
    print ', '
    print second
    print "\n"
.end
.sub foo :multi(_, Float)    :subid('foo_Float')
    .param pmc first
    .param pmc second
    print "(_, Float) method:  "
    print first
    print ', '
    print second
    print "\n"
.end
.sub main :main
    say "Direct calls use the multi."
    $P0 = new ['Float']
    $P0 = 9.5
    foo(1, $P0)
    $P1 = new ['Integer']
    $P1 = 3
    foo(1, $P1)
    say ".const 'Sub' should also use the multi."
    .const 'Sub' foo_const = 'foo'
    print "got "
    print foo_const
    print " type "
    $S0 = typeof foo_const
    say $S0
    foo_const(1, $P0)
    say "And so should get_global."
    .local pmc foo_sub
    foo_sub = get_global 'foo'
    print "got "
    print foo_sub
    print " type "
    $S0 = typeof foo_sub
    say $S0
    foo_sub(1, $P0)
.end

pmichaud@orange:~/parrot/trunk$ ./parrot x.pir
Direct calls use the multi.
(_, Float) method:  1, 9.5
(_, Int) method:  1, 3
.const 'Sub' should also use the multi.
got foo type MultiSub
(_, Float) method:  1, 9.5
And so should get_global.
got foo type MultiSub
(_, Float) method:  1, 9.5
pmichaud@orange:~/parrot/trunk$ 

Note that using the name of the MultiSub PMC as the :subid does run into problems if there are multiple independent MultiSubs of the same name within a compilation unit (as the :subid is again no longer unique) -- in that case one has to look up the MultiSubs via their namespaces. However, this is not different from the situation that existed prior to the introduction of :subid, so it likely isn't a significant issue.

One change we could consider is to have .sub's declared :multi but lacking a :subid flag automatically get a unique :subid (or otherwise don't fallback to using the name of the sub as :subid). That would have avoided the confusion that led to this ticket.

Based on the above, I'm marking this ticket as "invalid". If we want to modify the handling of :subid for sub declarations marked :multi, let's open a new ticket.

Thanks!

Pm

[rgrjr]

Thank you for that very thorough explanation. (I only wish I had isolated the problem sooner.) But there are two things that strike me as suboptimal:

1. It is counterintuitive to me that the only way to give a subid to a MultiSub is by giving different subids to all of the component methods. Possibly this could be addressed by not assigning a default subid to component subs of a multi. Users who need to refer to the components will most likely need to assign their own subids anyway.

1. Why doesn't IMCC complain if you use an ambiguous subid? At least a warning would have clued me in sooner.

But I'm not going to submit a new ticket for either of these, as I don't greatly care.