Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JRuby 1.2, JRuby 1.3, JRuby 1.3.1
    • Fix Version/s: JRuby 1.7.0.RC1
    • Component/s: Compiler
    • Labels:
      None
    • Number of attachments :
      3

      Description

      I'm not sure if this is the same issue as http://jira.codehaus.org/browse/JRUBY-3699, which seems to have a similar error.

      Here's what I know so far:
      The offending file (attached) needs to be compiled AND jarred

      I will work on getting a spec in a few days to reproduce this.

        Activity

        Hide
        Charles Oliver Nutter added a comment -

        Alex: I didn't realize your reduction actually included a very simple version...I should have read it more carefully.

        Based on your reduction3, I have a trivial reduced case:

        ../jruby-1.4.0/bin/jruby -e "def foo(value, expected); Object; blah = value.send(:=~, expected); end; foo('foo', /foo/)"
        

        The problem here is that the compiler optimization to eliminate local scopes does not do so for "send". Normally, it only detects methods known to set or get $, like the literal = method itself. When you call those methods via send, it does not see them, and therefore does not prepare a scope for them. It does not always fire because often the calling method does have a scope, and the backref ends up getting set there.

        The constant lookup here is what triggers the error. A method that does not appear to set or get $~ but does access a constant gets a DummyDynamicScope just for constant-scoping purposes. This results in the method having a scope, but a scope that refuses to accept $~, so it errors out.

        This is currently hard to fix without also causing a deoptimization for any methods doing .send, which is very heavily used for metaprogrammed code. I do have, in theory, a better way to handle backref logic; but I have not had spare cycles to try to put it in place.

        The temporary fix is not great, because it masks two bugs: the first is the fact that we're not properly preparing for $~ when you use send, and the second is that we're potentially overwriting a previous call's $~ when no constant is present to trigger the error.

        Not sure how to deal with this at the moment. $~ sucks.

        Show
        Charles Oliver Nutter added a comment - Alex: I didn't realize your reduction actually included a very simple version...I should have read it more carefully. Based on your reduction3, I have a trivial reduced case: ../jruby-1.4.0/bin/jruby -e "def foo(value, expected); Object; blah = value.send(:=~, expected); end; foo('foo', /foo/)" The problem here is that the compiler optimization to eliminate local scopes does not do so for "send". Normally, it only detects methods known to set or get $ , like the literal = method itself. When you call those methods via send, it does not see them, and therefore does not prepare a scope for them. It does not always fire because often the calling method does have a scope, and the backref ends up getting set there. The constant lookup here is what triggers the error. A method that does not appear to set or get $~ but does access a constant gets a DummyDynamicScope just for constant-scoping purposes. This results in the method having a scope, but a scope that refuses to accept $~, so it errors out. This is currently hard to fix without also causing a deoptimization for any methods doing .send, which is very heavily used for metaprogrammed code. I do have, in theory, a better way to handle backref logic; but I have not had spare cycles to try to put it in place. The temporary fix is not great, because it masks two bugs: the first is the fact that we're not properly preparing for $~ when you use send, and the second is that we're potentially overwriting a previous call's $~ when no constant is present to trigger the error. Not sure how to deal with this at the moment. $~ sucks.
        Hide
        Charles Oliver Nutter added a comment -

        I am downgrading this to "major" and making the warning only appear in debug mode, since it isn't something the user can fix and won't affect 99% of users. There's a small chance that code which sends a backref or lastline-mutating method could overwrite a previous call's backref or lastline, and that still needs to be remedied. But I'm more and more comfortable saying backref and lastline should not be expected to work across things like evals and sends, since it requires a level of stack manipulation we can't really do.

        The fix for this will likely require a rework of how we handle backref. It needs to be local to the containing method scope (mostly) but mutable by any calls that happen within the body. My current strategy for this would be to have specific named calls (like =~) use one of two ways to set/get backref/lastline:

        • Via a thread-local slot that's used only for the duration of the call. It would be populated with the current backref local variable before the call and used to set that local var after the call completes, avoiding the requirement for a separate heap structure.
        • Via an additional "out" parameter passed on the call stack. In this case, an IRubyObject-implementing carrier object would be prepared for backref-utilizing methods and passed to all calls likely to use backref. Calls that don't use it would simply ignore the extra value.

        At any rate, this is not something easily fixable in 1.5, and the impact to users is extremely low and unlikely to happen.

        Show
        Charles Oliver Nutter added a comment - I am downgrading this to "major" and making the warning only appear in debug mode, since it isn't something the user can fix and won't affect 99% of users. There's a small chance that code which sends a backref or lastline-mutating method could overwrite a previous call's backref or lastline, and that still needs to be remedied. But I'm more and more comfortable saying backref and lastline should not be expected to work across things like evals and sends, since it requires a level of stack manipulation we can't really do. The fix for this will likely require a rework of how we handle backref. It needs to be local to the containing method scope (mostly) but mutable by any calls that happen within the body. My current strategy for this would be to have specific named calls (like =~) use one of two ways to set/get backref/lastline: Via a thread-local slot that's used only for the duration of the call. It would be populated with the current backref local variable before the call and used to set that local var after the call completes, avoiding the requirement for a separate heap structure. Via an additional "out" parameter passed on the call stack. In this case, an IRubyObject-implementing carrier object would be prepared for backref-utilizing methods and passed to all calls likely to use backref. Calls that don't use it would simply ignore the extra value. At any rate, this is not something easily fixable in 1.5, and the impact to users is extremely low and unlikely to happen.
        Hide
        Alex K added a comment -

        I upgraded to 1.9 and now I got this new Warning message
        ":1 warning: DummyDynamicScope should never be used for backref storage"
        a lot. I assume that its somewhere in my code, but how can I now find the reason(s)?

        Show
        Alex K added a comment - I upgraded to 1.9 and now I got this new Warning message ":1 warning: DummyDynamicScope should never be used for backref storage" a lot. I assume that its somewhere in my code, but how can I now find the reason(s)?
        Hide
        Charles Oliver Nutter added a comment -

        Alex: If you are getting this "a lot" then I want to look into it. It indicates that some code is over-optimizing somewhere.

        I'll dig up this warning and see if I can come up with a way to investigate it.

        Show
        Charles Oliver Nutter added a comment - Alex: If you are getting this "a lot" then I want to look into it. It indicates that some code is over-optimizing somewhere. I'll dig up this warning and see if I can come up with a way to investigate it.
        Hide
        Charles Oliver Nutter added a comment -

        I'm going to call this fixed.

        There were recently some key fixes to the compiler (the inspector, really) that ensured scopes would be available for backref methods in more cases. This bug is also very old, and the compiler has been patched numerous times since then for similar issues.

        If this exact issue still exists with a simple reproduction, feel free to reopen.

        Show
        Charles Oliver Nutter added a comment - I'm going to call this fixed. There were recently some key fixes to the compiler (the inspector, really) that ensured scopes would be available for backref methods in more cases. This bug is also very old, and the compiler has been patched numerous times since then for similar issues. If this exact issue still exists with a simple reproduction, feel free to reopen.

          People

          • Assignee:
            Charles Oliver Nutter
            Reporter:
            Logan Barnett
          • Votes:
            5 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: