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

Problem when method returns Regexp capture group using special variable $1 #5503

Closed
jbaiza opened this Issue Dec 10, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@jbaiza
Copy link

commented Dec 10, 2018

Environment

Initially we found the problem using Jruby 9.2.1.0, but it's also actual in 9.1.7.0 and in the latest version.

  • JRuby version (jruby -v) and command line (flags, JRUBY_OPTS, etc)
jruby 9.2.5.0 (2.5.0) 2018-12-06 6d5a228 Java HotSpot(TM) 64-Bit Server VM 25.121-b13 on 1.8.0_121-b13 +jit [darwin-x86_64]
  • Operating system and platform (e.g. uname -a)
Darwin JBA-MacBook-Pro.local 18.2.0 Darwin Kernel Version 18.2.0: Fri Oct  5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64 x86_64

Other relevant info you may wish to add:

  • Installed or activated gems
    N/A
  • Application/framework version (e.g. Rails, Sinatra)
    N/A
  • Environment variables
    N/A

Test code:

def test_match(string)
  if string =~ /(.)/
    $1
  end
end

1000000.times { test_match("a") } 

test_match("b")

Expected Behavior

  • function test_match call always returns value (as in Ruby MRI or if match is used instead of =~):
jruby-9.2.5.0 :026 > test_match("b")
 => "b"

Actual Behavior

After some iterations returned result always is nil:

jruby-9.2.5.0 :009 > test_match("b")
 => nil
jruby-9.2.5.0 :010 > test_match("b")
 => nil

Full console output:
test_script.log

@kares kares added the jit label Dec 10, 2018

@enebo enebo added this to the JRuby 9.2.6.0 milestone Dec 10, 2018

@enebo enebo added regression core and removed jit labels Dec 10, 2018

@enebo

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I can get this to happen with --dev so this has to involve more than just JIT. Weirdly with -X+C I can get it to happen with no repetition in the front whereas --dev passes. So maybe 2 things wrong? This is pretty serious regression :|

@headius

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

This is a problem in IR when adding frame push/pop instructions. The pop is happening before the value has been captured, so it ends up nil (or actually, it gets whatever the next-highest heap frame has for $1.

From the IR along the "then" path for the if:

  1:                pop_backref_frame
  2:                return(nth<$1>)

headius added a commit to headius/jruby that referenced this issue Dec 10, 2018

Also fix up return values when frame is present, for backref.
This logic was used to ensure that any heap-scoped variable loads
happened before the "pop" logic for those scopes. However another
type of variable, frame-locals like $~ and $_, did not get the
same treatment. This patch ensures that any frame or binding
requirement triggers the return value copy fixup.

Fixes jruby#5503.
@headius

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Pushed a PR that should fix this. I have not written a test yet.

I do not know when this change was introduced, but it might never have worked right in the optimized "call protocol" execution modes. The problem was that frame-local implicit variables like $~ and $_ were not captured (in some circumstances) before the frame got popped, resulting in reading those values from a previous frame when they were returned. Only "binding" variables (those stored on the heap in DynamicScope instances) were safely captured before pop.

@headius

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

This was introduced in 8a04861 back in 2015. We may have been moving the frame variables $~ and $_ around a bit at the time (i.e. they might have been living on the scope/binding instead), or this was just an oversight.

@headius

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

@subbuss Can you review my patch? I already merged it since I'm pretty sure it's correct, but you originally removed the requireFrame and I want to make sure I didn't miss some detail.

This will add an additional Copy instruction to all returns of a non-temp or literal from a method with a heap frame. I'm not sure how much of an impact this really is, since most non-trivial data loads go immediately into a temp variable.

@subbuss

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

Left a comment on the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.