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

Kwargs should not need dynamic scope, since we have static handy. #3906

Merged
merged 2 commits into from
May 20, 2016

Conversation

headius
Copy link
Member

@headius headius commented May 20, 2016

...unfortunately...

In the process of getting the JIT to work properly without the AddLocalVarLoadStore pass, I ran into an issue with Block getting set into DynamicScope and blowing up (caused by OptimizeDelegation, fixed in 88a8896). In fixing that I realized that this particular method was always requiring a dynamic scope, for no obvious reason:

  def popen3(*cmd, **opts, &block)
    in_r, in_w = IO.pipe
    opts[:in] = in_r
    in_w.sync = true

    out_r, out_w = IO.pipe
    opts[:out] = out_w

    err_r, err_w = IO.pipe
    opts[:err] = err_w

    popen_run(cmd, opts, [in_r, out_w, err_w], [in_w, out_r, err_r], &block)
  end

It turned out that arity-checking of keyword arguments forced a DynamicScope so it could get to StaticScope. However, all Ruby bodies have direct access to their StaticScope now, so the DynamicScope was unnecessary.

Unfortunately, after I came up with this patch to remove that flag, I ran into another crash due to CheckLJE not reporting that it required DynamicScope.

I fixed that (on master) in 434e54e, but it just left me stranded on another mysterious failure in Rake's helpers.rb:

Error:

XXXXX

Method:

def permute_task(task_desc, task_type, base_name, options, *prereqs, &block)
  default_task = nil
  all_tasks = nil

  # iterate over all flag sets, noting default mapping
  tasks = {}
  options.each do |name, flags|
    if name == :default
      default_task = flags
      next
    end

    if name == :all
      all_tasks = flags
      next
    end

    test_task = task_type.new("#{base_name}:#{name}", &block).tap do |t|
      t.ruby_opts ||= []
      # THIS IS THE LINE THAT BLOWS UP
      flags.each do |flag|
        t.ruby_opts.unshift flag
      end
    end
    tasks[name] = test_task.name
    Rake::Task[test_task.name].tap do |t|
      t.add_description "#{flags.inspect}"
      t.prerequisites.concat prereqs
    end
  end

  # set up default, if specified
  if default_task
    desc "Run #{task_desc}s for #{default_task}"
    task base_name => tasks[default_task]
  end

  # set up "all", if specified, or make it run everything
  all_tasks ||= tasks.keys
  desc "Run #{task_desc}s for #{all_tasks.inspect}"
  task "#{base_name}:all" => all_tasks.map {|key| tasks[key]}
end

For some reason, variables are now getting crossed. The block param flags is getting mixed up with test_task later on, at least from the perspective of the inner block.

I have not provided the IR for brevity, but -Xir.print should be working properly now...build this branch and try to run anything with rake.

So, this is a work in progress. I expect this latest issue is another problem with scope optimizations killing a scope that should be there or vice versa...something that was masked before by the arity check's dynscope requirement. Any thoughts?

cc @subbuss @enebo

if (flags.contains(CAN_RECEIVE_BREAKS) || flags.contains(HAS_NONLOCAL_RETURNS) ||
flags.contains(CAN_RECEIVE_NONLOCAL_RETURNS) || flags.contains(BINDING_HAS_ESCAPED) ||
flags.contains(USES_ZSUPER) || flags.contains(RECEIVES_KEYWORD_ARGS)) {
if (flags.containsAll(NEEDS_DYNAMIC_SCOPE_FLAGS)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct ... shouldn't be containsAll ... containsAny if such a method exists.

@subbuss
Copy link
Contributor

subbuss commented May 20, 2016

See inline comment I left on the patch which should fix bugs with needs-dynscope flag. I cannot test now since I am off on a day trip to the north shore.

@headius
Copy link
Member Author

headius commented May 20, 2016

Oh good! I was hoping someone would see an obvious error in my commit. I'll fix that and see how it looks!

@headius
Copy link
Member Author

headius commented May 20, 2016

@subbuss You're a lifesaver even out of the office...looks like that may have been it!

@headius
Copy link
Member Author

headius commented May 20, 2016

Ok, it does look good. So the question is if we want to go with this approach, or if we want to try to improve AddLocalVarLoadStore like @subbuss has suggested.

@headius
Copy link
Member Author

headius commented May 20, 2016

Sorry, "this approach" is referring to the other PR where we removed ALVLS. This one can be merged to master either way.

@headius headius merged commit b2321a8 into jruby:master May 20, 2016
@headius headius added this to the JRuby 9.1.2.0 milestone May 20, 2016
@headius headius deleted the fix_kwargs_scoping branch May 20, 2016 15:01
@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants