2016-08-01 | 2216 words | An example of fixing a bug in Rakudo

While testing a fix for one of the Less Than Awesome behaviours in standalone Signature objects, I came across a bugglet. Smartmatching two Signatures throws, while spilling a bit of the guts:

<Zoffix> m: my $m = method ($a: $b) { }; say $m.signature ~~ :($a, $b);

<camelia> rakudo-moar 46838d: OUTPUT«Method 'type' not found for invocant of class 'Any'␤ in block at line 1␤␤»

So I figured I'll write about fixing it, 'cause hacking on internals is lots of fun. Let's roll!

Golf It Down

The less code there is to reproduces the bug, the fewer places there are for that bug to hide. We have a detached method and then we smartmatch its signature against something else. Let's try to golf it down a bit and smartmatch two Signatures, without involving a method:

<Zoffix> m: :($a, $b) ~~ :($a, $b);

<camelia> rakudo-moar 46838d: ( no output )

The bug disappeared, so perhaps out Signature on the left doesn't contain the stuff that triggers the bug. Let's dump the signature of the method to see what we should match against:

<Zoffix> m: my $m = method ($a: $b) { }; say $m.signature <camelia> rakudo-moar 46838d: OUTPUT«($a: $b, *%_)␤»

Aha! It has a slurpy hash: *%_ . Let's try matching a Signature with a slurpy in it:

<Zoffix> m: :(*%) ~~ :();

<camelia> rakudo-moar 46838d: OUTPUT«Method 'type' not found for invocant of class 'Any'␤ in block at line 1␤␤»

And there we go: hole in three. Let's proceed.

Roast It

There's an official Perl 6 test suite that Rakudo must pass to be called a Perl 6 compiler. Since we got a bug on our hands, we should add a test for it to the test suite to ensure it doesn't rear its ugly head again.

The copy of the repo gets automatically cloned into t/spec when you run make spectest in Rakudo's checkout. If you don't have a commit bit, you can just change the remote/branch of that checkout to your fork:

cd t/spec git remote rm origin git remote add origin https://github.com/YOURUSERNAME/roast git checkout your-branch cd ../..

It may be tricky to figure out which file to put the test in, if you're new. You can always ask the good folks on irc.freenode.net/#perl6 for advice. In this case, I'll place the test into S06-signature/outside-subroutine.t

While not required, I find it helpful to open a ticket for the bug. This way I can reference it in my fix in the compiler repo, I can reference it in the commit to the test repo, and people get a place where to tell me why I'm being stupid when I am. I opened this bug as RT#128795.

Now, for the code of the test itself. I'll adjust the plan at the top of the file to include however many tests I'm writing—in this case one. I'll use the lives-ok test sub and stick our buggy golfed code into it. Here's the diff of the changes to the file; note the reference to the ticket number in the comment before the test:

@@ -1,7 +1,7 @@ use v6; use Test; -plan 3; +plan 4; # RT #82946 subtest 'signature binding outside of routine calls' => { @@ -25,4 +25,7 @@ subtest 'smartmatch on signatures with literal strings' => { # RT #128783 lives-ok { EVAL ’:($:)‘ }, ’signature marker is allowed in bare signature‘; +# RT #128795 +lives-ok { :(*%)~~ :() }, 'smartmatch with no slurpy on right side'; + # vim: ft=perl6

Run the file now to ensure the test fails. Hint: some files have fudging; explaining it is out of the scope of this article, but if you notice failures you're not expecting, look it up.

$ make t/spec/S06-signature/outside-subroutine.t ... Test Summary Report ------------------- t/spec/S06-signature/outside-subroutine.t (Wstat: 256 Tests: 4 Failed: 1) Failed test: 4 Non-zero exit status: 1

With the test in place, it's time to look at some source code. Let the bug hunt begin!

Make it Saucy

Our bug involves a Smartmatch operator, which aliases the left side to the topic variable $_ and calls .ACCEPTS method on the right side with it. Both of our sides are Signature objects, so let's pop open Rakudo's sauce code for that class.

In the Rakudo's repo, directory src/core/ contains most of the built in types in separate files named after those types, so we'll just pop open src/core/Signature.pm in the editor and locate the definition of method ACCEPTS .

There are actually four multis for ACCEPTS . Here's the full code. Don't try to understand all of it, just note its size.

multi method ACCEPTS(Signature:D: Capture $topic) { nqp::p6bool(nqp::p6isbindable(self, nqp::decont($topic))); } multi method ACCEPTS(Signature:D: @topic) { self.ACCEPTS(@topic.Capture) } multi method ACCEPTS(Signature:D: %topic) { self.ACCEPTS(%topic.Capture) } multi method ACCEPTS(Signature:D: Signature:D $topic) { my $sclass = self.params.classify({.named}); my $tclass = $topic.params.classify({.named}); my @spos := $sclass{False} // (); my @tpos := $tclass{False} // (); while @spos { my $s; my $t; last unless @tpos && ($t = @tpos.shift); $s=@spos.shift; if $s.slurpy or $s.capture { @spos=(); @tpos=(); last; } if $t.slurpy or $t.capture { return False unless any(@spos) ~~ {.slurpy or .capture}; @spos=(); @tpos=(); last; } if not $s.optional { return False if $t.optional } return False unless $t ~~ $s; } return False if @tpos; if @spos { return False unless @spos[0].optional or @spos[0].slurpy or @spos[0].capture; } for flat ($sclass{True} // ()).grep({!.optional and !.slurpy}) -> $this { my $other; return False unless $other=($tclass{True} // ()).grep( {!.optional and $_ ~~ $this }); return False unless +$other == 1; } my $here=($sclass{True}:v).SetHash; my $hasslurpy=($sclass{True} // ()).grep({.slurpy}); $here{@$hasslurpy} :delete; $hasslurpy .= Bool; for flat @($tclass{True} // ()) -> $other { my $this; if $other.slurpy { return False if any($here.keys) ~~ -> Any $_ { !(.type =:= Mu) }; return $hasslurpy; } if $this=$here.keys.grep( -> $t { $other ~~ $t }) { $here{$this[0]} :delete; } else { return False unless $hasslurpy; } } return False unless self.returns =:= $topic.returns; True; }

The error we get from the bug mentions .type method call and there is one such method call in the code above (close to the end of it). In this case, there's quite a bit of code to sort through. It would be nice to be able to play around with it, stick a couple of dd or say calls to dump out variables, right?

That approach, however, is somewhat annoying because after each change we have to recompile the entire Rakudo. On the meatiest box I got, it takes about 60 seconds. Not the end of the world, but there's a way to make things lightning fast!

Mix Your Fix

We need to fix a bug in a method of a class. Another way to think of it is: we need to replace a broken method with a working one. Signature class is just like any other class, so if we want to replace one of its methods, we can just mix in a role!

The broken ACCEPTS will continue to live in the compiler, and we'll pop open a separate playground file and define a role—let's calls it FixedSignature —in it. To get our new-and-improved ACCEPTS method in standalone signature objects, we'll use the but operator to mix the FixedSignature in.

Here's the role, the mixing in, and the code that triggers the bug. I'll leave out method bodies for brieviety, but there's they are the same as in the code above.

role FixedSignature { multi method ACCEPTS(Signature:D: Capture $topic) { #`(redacted for brevity) } multi method ACCEPTS(Signature:D: @topic) { #`(redacted for brevity) } multi method ACCEPTS(Signature:D: %topic) { #`(redacted for brevity) } multi method ACCEPTS(Signature:D: Signature:D $topic) { #`(redacted for brevity) } } my $a = :(*%) but FixedSignature; my $b = :() but FixedSignature; say $a ~~ $b;

There are two more things we need to do for our role to work properly. First, we're dealing with multis and right now the multis in our role are creating ambiguities with the multis in the original Signature class. To avoid that, we'll define a proto:

proto method ACCEPTS (|) { * }

Since the code is using some NQP, we also need to bring in those features into our playground file with the role. Just add the appropriate pragma at the top of the file:

use MONKEY-GUTS;

With these modifications, our final test file becomes the following:

use MONKEY-GUTS; role FixedSignature { proto method ACCEPTS (|) { * } multi method ACCEPTS(Signature:D: Capture $topic) { #`(redacted for brevity) } multi method ACCEPTS(Signature:D: @topic) { #`(redacted for brevity) } multi method ACCEPTS(Signature:D: %topic) { #`(redacted for brevity) } multi method ACCEPTS(Signature:D: Signature:D $topic) { #`(redacted for brevity) } } my $a = :(*%) but FixedSignature; my $b = :() but FixedSignature; say $a ~~ $b;

And with this trick in place, we now have a rapid-fire weapon to hunt down the bug with—the changes we make compile instantly.

Pull The Trigger

Now, we can debug the code just like any other. I prefer applying liberal amounts of dd (or say ) calls and dumping out the variables to ensure their contents match expectations.

The .type method call our error message mentions is in this line:

return False if any($here.keys) ~~ -> Any $_ { !(.type =:= Mu) };

It calls it on the keys of $here , so let's dump the $here before that statement:

... dd $here return False if any($here.keys) ~~ -> Any $_ { !(.type =:= Mu) }; ... # OUTPUT: # SetHash $here = SetHash.new(Any)

Here's our offending Any , let's go up a bit and dump the $here right where it's defined:

... my $here=$sclass{True}.SetHash; dd $here; ... # OUTPUT: # SetHash $here = SetHash.new(Any)

It's still there, and for a good reason. If we trace the creation of $sclass , we'll see it's this:

my $sclass = self.params.classify({.named});

The params of the Signature on the right of the smartmatch get classified based on whether they are named or not. The named parameters will be inside a list under the True key of $sclass . Since we do not have any named params, there won't be such a key, and we can verify that with this bit of code:

:().params.classify(*.named).say # OUTPUT: # {}

When we go to define $here , we get an Any from $sclass{True} , since that key doesn't exist, and when we call .SetHash on it, we get our problematic Sethash object with an Any in it. And so, we have our fix for the bug: ensure the True key in $sclass is actually there before creating a SetHash out of its value:

my $here=($sclass{True}:v).SetHash;

Add that to our playground file with the FixedSignature role in it, run it, and verify the fix works. Now, simply transplant the fix back into src/core/Signature.pm and then compile the compiler.

perl Configure.pl --gen-moar --gen-nqp --backends=moar make make test make install

Verify our fix worked before we proceed onto the final stages:

$ make t/spec/S06-signature/outside-subroutine.t ... All tests successful. Files=1, Tests=4, 1 wallclock secs ( 0.03 usr 0.00 sys + 0.32 cusr 0.02 csys = 0.37 CPU) Result: PASS

A Clean Kill

So far, all we know is the bug we found was fixed and the tests we wrote for it pass. However, before we ship our fix, we must ensure we didn't break anything else. There are other devs working from the same repo and you'll be interfering with their work if you break stuff.

Run the full Roast test suite with make spectest command. You can use the TEST_JOBS environmental variable to specify the number of simultaneous tests. Generally a value slightly higher than the available cores works the fastest... and cores make all the difference. On my 24-core VM I cut releases on, the spectest completes in about 1 minute and 15 seconds. On my 2-core web server, it takes about 25 minutes. You get the idea.

TEST_JOBS=28 make spectest ... All tests successful. Files=1111, Tests=52510, 82 wallclock secs (13.09 usr 2.44 sys + 1517.34 cusr 97.67 csys = 1630.54 CPU) Result: PASS

Once the spectest completes and we have the clean bill of health, we're ready to ship our fix. Commit the Rakudo fix, then go into t/spec and commit the Roast fix:

git commit -m 'Fix Smartmatch with two signatures, only one of which has slurpy hash' \ -m 'Fixes RT#128795' src/core/Signature.pm git push cd t/spec git commit -m 'smartmatch on signature with no slurpy on right side does not crash' \ -m 'RT#128795' S06-signature/outside-subroutine.t git push

If you're pushing to your fork of these projects, you have to go the extra step and submit a Pull Request (just go to your fork and GitHub should display a button just for that).

And we're done! Celebrate with the appropriate amount of fun.

Conclusion

Rakudo bugs can be easy to fix, requiring not much more than knowledge of Perl 6. To fix them, you don't need to re-compile the entire compiler, but can instead define a small role with a method you're trying to fix and modify and recompile just that.

It's important to add tests for the bug into the official test suite and it's also important to run the full spectest after you fix the bug. But most important of all, is to have fun fixing it.

-Ofun