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

Rails new fails with 9.1.3.0 and "--dev" option #4134

Closed
MartinKoerner opened this Issue Sep 6, 2016 · 14 comments

Comments

Projects
None yet
3 participants
@MartinKoerner

MartinKoerner commented Sep 6, 2016

Environment

  • jruby 9.1.3.0 (2.3.1) 2016-08-29 a2a3b29 Java HotSpot(TM) 64-Bit Server VM 25.20-b23 on 1.8.0_20-b26 [darwin-x86_64]
  • JRUBY_OPTS='--dev'
  • Rails 4.2.7.1

Expected Behavior

$ rails new blankapp
Generates a new Rails app.
It is doing so with JRuby 9.1.2.0 (including '--dev') or with JRuby 9.1.3.0 excluding the '--dev' flag

Actual Behavior

$ rails new blankapp
    create  
    create  README.rdoc
    create  Rakefile
    ...
    create  db
    create  db/seeds.rb
NoMethodError: protected method `destination=' called for #
Did you mean?  destination
           initialize at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/actions/empty_directory.rb:36
      empty_directory at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/actions/empty_directory.rb:14
      empty_directory at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/railties-4.2.7.1/lib/rails/generators/rails/app/app_generator.rb:17
                  lib at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/railties-4.2.7.1/lib/rails/generators/rails/app/app_generator.rb:108
                build at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/railties-4.2.7.1/lib/rails/generators/app_base.rb:133
     create_lib_files at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/railties-4.2.7.1/lib/rails/generators/rails/app/app_generator.rb:220
                  run at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/command.rb:27
       invoke_command at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/invocation.rb:126
  block in invoke_all at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/invocation.rb:133
                 each at org/jruby/RubyHash.java:1344
                  map at org/jruby/RubyEnumerable.java:832
           invoke_all at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/invocation.rb:133
             dispatch at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/group.rb:232
                start at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/thor-0.19.1/lib/thor/base.rb:440
                at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/railties-4.2.7.1/lib/rails/commands/application.rb:17
              require at org/jruby/RubyKernel.java:955
               (root) at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/stdlib/rubygems/core_ext/kernel_require.rb:1
                at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/stdlib/rubygems/core_ext/kernel_require.rb:68
              require at org/jruby/RubyKernel.java:955
               (root) at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/gems/shared/gems/railties-4.2.7.1/lib/rails/cli.rb:14
                at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/stdlib/rubygems/core_ext/kernel_require.rb:1
                 load at org/jruby/RubyKernel.java:973
                at /usr/local/var/rbenv/versions/jruby-9.1.3.0/lib/ruby/stdlib/rubygems/core_ext/kernel_require.rb:68

Follow up ticket from #4127, probably related to #2198

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

The issue in #2198 seems to have been that it was supposed to present an error and the JIT did not, probably due to relaxed visibility-checking. That glossed over the actual problem, which is that attribute accessors are not checking visibility correctly. @subbuss fixed that in c0f90f4.

I believe the JIT still may be skipping the visibility check.

The problem now is that I reverted his change, not realizing why attribute accessors required visibility checking. That commit was 7bed12f.

The proper fix would be to get visibility-checking working properly without requiring the frame self. This needs to be addressed for a quick 9.1.5.0 release.

Member

headius commented Sep 6, 2016

The issue in #2198 seems to have been that it was supposed to present an error and the JIT did not, probably due to relaxed visibility-checking. That glossed over the actual problem, which is that attribute accessors are not checking visibility correctly. @subbuss fixed that in c0f90f4.

I believe the JIT still may be skipping the visibility check.

The problem now is that I reverted his change, not realizing why attribute accessors required visibility checking. That commit was 7bed12f.

The proper fix would be to get visibility-checking working properly without requiring the frame self. This needs to be addressed for a quick 9.1.5.0 release.

@headius headius added this to the JRuby 9.1.5.0 milestone Sep 6, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius
Member

headius commented Sep 6, 2016

cc @enebo

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Sep 6, 2016

Contributor

@headius my comments on #2198 indicates that I opened a new task for the JIT. Not sure if that is resolved or not. In any case, I assume you got this one.

Contributor

subbuss commented Sep 6, 2016

@headius my comments on #2198 indicates that I opened a new task for the JIT. Not sure if that is resolved or not. In any case, I assume you got this one.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

@subbuss Do you remember what issue you opened? :-)

And yes, I'll try to fix it and let you know if I need assistance.

Member

headius commented Sep 6, 2016

@subbuss Do you remember what issue you opened? :-)

And yes, I'll try to fix it and let you know if I need assistance.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Sep 6, 2016

Contributor

I know ... I was kicking myself for not mentioning it in the comment. I'll have to look at github issues I created and find it.

Contributor

subbuss commented Sep 6, 2016

I know ... I was kicking myself for not mentioning it in the comment. I'll have to look at github issues I created and find it.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Sep 6, 2016

Contributor

#2201 maybe?

Contributor

subbuss commented Sep 6, 2016

#2201 maybe?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

@subbuss And it looks like it's fixed...but now we have this issue. Thanks, I'll see what I can do.

Member

headius commented Sep 6, 2016

@subbuss And it looks like it's fixed...but now we have this issue. Thanks, I'll see what I can do.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

I'm working up a patch and have a few observations:

  • Attribute readers, like foo.bar, just compile like normal calls now. Normal calls use standard on-stack mechanisms for checking caller and visibility, so those mechanisms should be ok for attribute writers.
  • I failed to notice that AttrAssignInstr used Helpers.invoke, which does indeed require a frame to do visibility checking. It also means that all attr assignment from IR is doing those calls uncached.

My patch does the following:

  • Modifies the base CallType for AttrAssignInstr to NORMAL so the base class's callSite field gets initialized properly.
  • Adds a second CallSite for functional (i.e. visibility-free) invocation in the cases where object == self. This may not be necessary if our "normal" call site is doing this visibility check correctly.
  • Modifies the interpret method to use the appropriate site to make the call, which eliminates the need for a frame and introduces call site caching.

The JIT may work properly here because it already does use the actual caller to check visibility, without requiring a frame.

Member

headius commented Sep 6, 2016

I'm working up a patch and have a few observations:

  • Attribute readers, like foo.bar, just compile like normal calls now. Normal calls use standard on-stack mechanisms for checking caller and visibility, so those mechanisms should be ok for attribute writers.
  • I failed to notice that AttrAssignInstr used Helpers.invoke, which does indeed require a frame to do visibility checking. It also means that all attr assignment from IR is doing those calls uncached.

My patch does the following:

  • Modifies the base CallType for AttrAssignInstr to NORMAL so the base class's callSite field gets initialized properly.
  • Adds a second CallSite for functional (i.e. visibility-free) invocation in the cases where object == self. This may not be necessary if our "normal" call site is doing this visibility check correctly.
  • Modifies the interpret method to use the appropriate site to make the call, which eliminates the need for a frame and introduces call site caching.

The JIT may work properly here because it already does use the actual caller to check visibility, without requiring a frame.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

I think I might also remove visibility-checking from Helpers.invoke*. Even a pro like me will miss that these methods require a Ruby frame to be set up, and I suspect usually we want an "invoke" from Java code to skip visibility. In any case, having it do the check and expect a frame seems very error-prone.

Member

headius commented Sep 6, 2016

I think I might also remove visibility-checking from Helpers.invoke*. Even a pro like me will miss that these methods require a Ruby frame to be set up, and I suspect usually we want an "invoke" from Java code to skip visibility. In any case, having it do the check and expect a frame seems very error-prone.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

Added PR with my changes: #4138

Member

headius commented Sep 6, 2016

Added PR with my changes: #4138

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Sep 6, 2016

Contributor

I think I might also remove visibility-checking from Helpers.invoke*. Even a pro like me will miss that these methods require a Ruby frame to be set up, and I suspect usually we want an "invoke" from Java code to skip visibility. In any case, having it do the check and expect a frame seems very error-prone.

Good thing! This is also one of the reasons why I wanted the IR to be explicit about all the previously implicit work that was happening. I wanted it for 2 reasons: (a) clarity about operational semantics of ruby code and identifying where some of the complexity is inherent Ruby complexity vs. being accidental JRuby complexity (b) identifying scenarios where some of these operations can be eliminated.

However, we have walked back on this design in a few places since the explicit nature of the operations means the interpreter has additional overhead. But, I think that we should largely follow this approach / principle.

Contributor

subbuss commented Sep 6, 2016

I think I might also remove visibility-checking from Helpers.invoke*. Even a pro like me will miss that these methods require a Ruby frame to be set up, and I suspect usually we want an "invoke" from Java code to skip visibility. In any case, having it do the check and expect a frame seems very error-prone.

Good thing! This is also one of the reasons why I wanted the IR to be explicit about all the previously implicit work that was happening. I wanted it for 2 reasons: (a) clarity about operational semantics of ruby code and identifying where some of the complexity is inherent Ruby complexity vs. being accidental JRuby complexity (b) identifying scenarios where some of these operations can be eliminated.

However, we have walked back on this design in a few places since the explicit nature of the operations means the interpreter has additional overhead. But, I think that we should largely follow this approach / principle.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

Note, it appears the JIT is choosing its call type the old way, at compile time by checking for literal "self". However, I think even in cases where the receiver is not literally "self", the normal site will do the right thing (checking if the caller is an instance of the receiver).

        compileCallCommon(
                jvmMethod(),
                attrAssignInstr.getName(),
                callArgs,
                attrAssignInstr.getReceiver(),
                callArgs.length,
                null,
                false,
                attrAssignInstr.getReceiver() instanceof Self ? CallType.FUNCTIONAL : CallType.NORMAL,
                null,
                attrAssignInstr.isPotentiallyRefined());

This may simplify my patch, but I'll let the PR's CI run its course before trying that.

Member

headius commented Sep 6, 2016

Note, it appears the JIT is choosing its call type the old way, at compile time by checking for literal "self". However, I think even in cases where the receiver is not literally "self", the normal site will do the right thing (checking if the caller is an instance of the receiver).

        compileCallCommon(
                jvmMethod(),
                attrAssignInstr.getName(),
                callArgs,
                attrAssignInstr.getReceiver(),
                callArgs.length,
                null,
                false,
                attrAssignInstr.getReceiver() instanceof Self ? CallType.FUNCTIONAL : CallType.NORMAL,
                null,
                attrAssignInstr.isPotentiallyRefined());

This may simplify my patch, but I'll let the PR's CI run its course before trying that.

headius added a commit to headius/jruby that referenced this issue Sep 6, 2016

headius added a commit to headius/jruby that referenced this issue Sep 6, 2016

Select call site based on receiver operand type at compile time.
This has worked for the JIT and it runs almost all the same
tests as the interpreter.

See #4134.

headius added a commit to headius/jruby that referenced this issue Sep 6, 2016

headius added a commit to headius/jruby that referenced this issue Sep 6, 2016

Deprecate frame-sensitive "invoke" methods and add "invokeFrom".
This is in response to callers not seting up frame and breaking
visibility checks in #4134. This goes along with jruby/jruby#4138.

headius added a commit to headius/jruby that referenced this issue Sep 6, 2016

Deprecate frame-sensitive "invoke" methods and add "invokeFrom".
This is in response to callers not seting up frame and breaking
visibility checks in #4134. This goes along with jruby/jruby#4138.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

See #4139 for the deprecations that go along with #4138.

Member

headius commented Sep 6, 2016

See #4139 for the deprecations that go along with #4138.

@headius headius closed this in 89e02da Sep 6, 2016

headius added a commit that referenced this issue Sep 6, 2016

Merge pull request #4138 from headius/attr_asgn_visibility_fix
Use a CallSite for invocation of attr writers. Fixes #4134.

@headius headius added the needs tests label Sep 7, 2016

@MartinKoerner

This comment has been minimized.

Show comment
Hide comment
@MartinKoerner

MartinKoerner Sep 7, 2016

Wow, thanks for the fast fix

MartinKoerner commented Sep 7, 2016

Wow, thanks for the fast fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment