Ticket #1029 (closed bug: fixed)

Opened 5 years ago

Last modified 4 years ago

taillcall into PIR compiler broken

Reported by: coke Owned by: whiteknight
Priority: normal Milestone:
Component: none Version: trunk
Severity: high Keywords:
Cc: Language: perl6
Patch status: Platform: all

Description

See  Original RT

If you apply this patch to parrot:

index 776a062..4f1486f 100644
--- a/compilers/pct/src/PCT/HLLCompiler.pir
+++ b/compilers/pct/src/PCT/HLLCompiler.pir
@@ -512,8 +512,7 @@ Transform PAST C<source> into POST.
     .param pmc adverbs         :slurpy :named
 
     $P0 = compreg 'PIR'
-    $P1 = $P0(source)
-    .return ($P1)
+    .tailcall $P0(source)
 .end

This causes failures in rakudo. For example, "class {}" without the patch returns nothing in interactive mode, but with the patch, it returns a failure regarding the # of arguments expected.

A simple test of the tailcall (below) doesn't show this behavior.

.sub main 
  $P1 = elm()
  $P1()
.end

.sub elm

  $P1 = compreg 'PIR'
  $S0 =<<'END_PIR'
.sub foo
say 'ok'
.end
END_PIR
  .tailcall $P1($S0)
.end

Change History

  Changed 5 years ago by coke

It would be interesting to find out if PIRC avoided this issue.

follow-up: ↓ 4   Changed 4 years ago by coke

Applying this patch to r44285 causes failures in "make nqp_test". No rakudo needed.

  Changed 4 years ago by coke

per moritz++, current rakudo (as of today) fails to build with this patch.

in reply to: ↑ 2   Changed 4 years ago by jkeenan

Replying to coke:

Applying this patch to r44285 causes failures in "make nqp_test". No rakudo needed.

Where is make nqp_test found?

[parrot] 504 $ fns . | xargs grep -Ei 'nqp_?test'

[rakudo-star-2010.07] 505 $ grep -n nqp_test Makefile 
[rakudo-star-2010.07] 506 $ ack nqp_test *

What am I missing here?

  Changed 4 years ago by coke

nqp_test is from the old nqp compiler, which rakudo used to be based on.

A simple 'make test' shows the failure is cropping up elsewhere now anyway:

prove t/profiling/profiling.tt

Passes without the patch, fails with it.

  Changed 4 years ago by coke

That's prove t/profiling/profiling.t

  Changed 4 years ago by whiteknight

  • owner set to whiteknight

  Changed 4 years ago by whiteknight

  • status changed from new to assigned
  • platform set to all

short bit of analysis here. The tailcall happens without a problem. The PIR compreg resets interp->current_cont to NEEDS_CONTINUATION. The NCI VTABLE_invoke returns the old cur_opcode+2 destination instead of the updated destination from the interp->current_cont. The runloop continues execution past the end of the function, which leads to any number of potential problems. I'm testing a fix for this right now.

  Changed 4 years ago by whiteknight

  • status changed from assigned to closed
  • resolution set to fixed

Fixed in 8b188db. All tests pass.

  Changed 4 years ago by coke

  • status changed from closed to reopened
  • resolution fixed deleted

Part of this change was reverted in 9328cf.

See #1935.

  Changed 4 years ago by pmichaud

The tailcall has been reverted in 9328cff, as it was causing failures in both Rakudo and nqp-rx.

I'll add a note to this ticket that we probably should use the tailcall in HLLCompiler only to evoke a known Parrot bug, but not as a permanent "optimization" or change. In this particular case (and in most of HLLCompiler) there's no significant optimization to be had in using tailcalls, and there are definite negatives if the tailcalls are used in situations where they cause crashes and failures for downstream users.

Pm

  Changed 4 years ago by coke

> Comment(by pmichaud):
>
>  The tailcall has been reverted in 9328cff, as it was causing failures in
>  both Rakudo and nqp-rx.
>
>  I'll add a note to this ticket that we probably should use the tailcall in
>  HLLCompiler only to evoke a known Parrot bug, but not as a permanent
>  "optimization" or change.  In this particular case (and in most of
>  HLLCompiler) there's no significant optimization to be had in using
>  tailcalls, and there are definite negatives if the tailcalls are used in
>  situations where they cause crashes and failures for downstream users.

FYI, the original bug report was against the old NQP, and was reduced
to a parrot test when it was noticed that it was failing just fine
with the patch.

It would be great if we could get a test that exhibits the current
failure as a core parrot test (I realize this is difficult.)

I think the goal should be to be fix the bug, not avoid it. (Though
avoiding it until it's fixed is, of course, a great idea.)

-- 
Will "Coke" Coleda

  Changed 4 years ago by whiteknight

I have added a test for the behavior in ef82f5c, along with another more robust fix for the issue. We have removed the .tailcall from HLLCompiler and we do not need to replace it, but I think we are getting to a point where this issue is, or is nearly fixed. I am running some more tests now to verify.

  Changed 4 years ago by whiteknight

  • status changed from reopened to closed
  • resolution set to fixed

Okay, i've verified that my newest fix resolves the issues that NQP was seeing. There is also a test now in place to verify that this same thing doesn't happen again, although I still don't know if .tailcall works 100%. It certainly is underutilized in the test suite and elsewhere.

I'm going to re-close this issue. Thanks to everybody who helped test this one.

Note: See TracTickets for help on using tickets.