Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't reference the proc itself in a proc #3173

Closed
yamam opened this issue Jul 24, 2015 · 5 comments
Closed

Can't reference the proc itself in a proc #3173

yamam opened this issue Jul 24, 2015 · 5 comments

Comments

@yamam
Copy link

@yamam yamam commented Jul 24, 2015

In a proc defined in a method, you can't reference the proc itself of local variable.

def mk_proc
    proc1 = proc do
        p proc1
    end
end
mk_proc.call
> jruby-9.0.0.0/bin/jruby -v test.rb
jruby 9.0.0.0 (2.2.2) 2015-07-21 e10ec96 Java HotSpot(TM) Client VM 25.51-b03 on 1.8.0_51-b16 +jit [Windows 7-x86]
io/console not supported; tty will not be manipulated
nil

> jruby-1.7.21/bin/jruby -v test.rb
jruby 1.7.21 (1.9.3p551) 2015-07-07 a741a82 on Java HotSpot(TM) Client VM 1.8.0_51-b16 +jit [Windows 7-x86]
io/console not supported; tty will not be manipulated
#<Proc:0xd992b2@test.rb:2>

> ruby -v test.rb
ruby 2.0.0p576 (2014-09-19) [i386-cygwin]
#<Proc:0x2027282c@test.rb:2>
@enebo enebo added this to the JRuby 9.0.1.0 milestone Jul 24, 2015
dirk added a commit to dirk/jruby that referenced this issue Jul 25, 2015
For jruby#3173 and to check against future regression. Was broken in
9.0.0.0 but seems to be fixed in 9.0.1.0.
@subbuss
Copy link
Contributor

@subbuss subbuss commented Jul 27, 2015

Seems fixed with 8f80816

@subbuss subbuss closed this Jul 27, 2015
@subbuss
Copy link
Contributor

@subbuss subbuss commented Jul 28, 2015

This is not yet quite fixed.

@subbuss subbuss reopened this Jul 28, 2015
subbuss added a commit that referenced this issue Jul 28, 2015
DCE used to do additional tests to decide if an instruction can be
deleted based on the scope's state. This is a no-no. DCE should
not do any additional tests about deletability based on scope
state (which was in reality liveness tests) -- it should simply use
whatever info is produced by LVA. Those tests should have been
part of LVA instead.

This really didn't affect correctness of DCE, but it would be
providing incorrect information about vars still live when the
scope was exited. Specifically, AddLocalVarLoadStoreInstructions pass
uses that LVA information to determine whether to add lvar stores
on exit. Without accurate LVA information, this can inadvertently
prevent lvar updates from being seen outside the scope.

This is part 1 of the fix for #3173.
subbuss added a commit that referenced this issue Jul 28, 2015
The following fixes were made:

1. Same kind of fix as in d59eeec but applied to this pass.
   The actual addition of binding stores shouldn't do additional
   analysis not already present in the dataflow analysis. It should
   simply use information from that analysis and add stores, where
   required.

2. A specific outcome of 1. above is: the addition should not look
   at scope type while adding stores on exit. If the analysis says:
   "add a store", add a store. No need to do it for closures only.

3. There was a subtle bug in the code. For exit BBs, we were
   intersecting lvars that were dirtied with lvars that were live
   at that point. However, we were looking for 'varsLiveOnExit'
   which was erroneous. LVA is a backwards propagation problem
   and we should be looking at 'varsLiveOnEntry' (since the exit
   of the scope corresponds to the dataflow entry for LVA).

Between these fixes and the fix in 8c48c3bc, #3173 should be solved.
@subbuss
Copy link
Contributor

@subbuss subbuss commented Jul 29, 2015

Now fixed.

@subbuss subbuss closed this Jul 29, 2015
@dirk
Copy link
Contributor

@dirk dirk commented Aug 3, 2015

@subbuss: Thanks for including detailed commit messages on those two commits; feel like I learned a lot from reading those and the diff. 😄

@subbuss
Copy link
Contributor

@subbuss subbuss commented Aug 3, 2015

@dirk I tend to write verbose commit messages :-) glad they were helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants