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

Merged
merged 2 commits into from May 20, 2016

Conversation

Projects
None yet
3 participants
@headius
Member

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)) {

This comment has been minimized.

@subbuss

subbuss May 20, 2016

Contributor

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

@subbuss

subbuss May 20, 2016

Contributor

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

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss May 20, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 20, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 20, 2016

Member

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

Member

headius commented May 20, 2016

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

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 20, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 20, 2016

Member

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

Member

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

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@headius headius added this to the JRuby 9.1.2.0 milestone May 20, 2016

@headius headius deleted the headius:fix_kwargs_scoping branch May 20, 2016

@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