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

Errors with _id2ref when using frozen_string_literal magic comment? #2358

Closed
hainesr opened this issue May 25, 2021 · 9 comments
Closed

Errors with _id2ref when using frozen_string_literal magic comment? #2358

hainesr opened this issue May 25, 2021 · 9 comments
Assignees
Labels
Milestone

Comments

@hainesr
Copy link

hainesr commented May 25, 2021

I have no idea what or where this bug really is, but as the behaviour I'm seeing is only in TruffleRuby I thought I'd submit here to see if you folks have any thoughts.

I am doing some work on rubyzip and I finally got around to adding the frozen_string_literal: true magic comment throughout the library. Now this is in place my CI is failing on TruffleRuby in GitHub Actions. This is the only change that has been made between a passing CI and failing CI:

Actions is using TruffleRuby version 21.1.0 and TruffleRuby-head, which are both failing. I have version 20.3.0 on my local machine and all works fine!

The error is as follows:

ZipFsFileNonmutatingTest#test_dirname:
RangeError: 0x0000000000000100 is not id value
    /home/runner/work/rubyzip/rubyzip/test/test_helper.rb:192:in `_id2ref'
    /home/runner/work/rubyzip/rubyzip/test/test_helper.rb:192:in `dirname'
    /home/runner/work/rubyzip/rubyzip/lib/zip/filesystem.rb:303:in `dirname'
    /home/runner/work/rubyzip/rubyzip/test/filesystem/file_nonmutating_test.rb:132:in `block in test_dirname'
    /home/runner/work/rubyzip/rubyzip/test/test_helper.rb:196:in `assert_forwarded'
    /home/runner/work/rubyzip/rubyzip/test/filesystem/file_nonmutating_test.rb:131:in `test_dirname'

Which is caused by a test that makes heavy use of meta-programming, unfortunately:

def test_dirname
  assert_forwarded(File, :dirname, 'ret_val', 'a/b/c/d') do
    @zip_file.file.dirname('a/b/c/d')
  end
end

assert_forwarded is where _id2ref is called and is defined here:

def assert_forwarded(object, method, ret_val, *expected_args)
  call_args = nil
  call_args_proc = proc { |args| call_args = args }
  object.instance_eval <<-END_EVAL, __FILE__, __LINE__ + 1
    alias #{method}_org #{method}
    def #{method}(*args)
      ObjectSpace._id2ref(#{call_args_proc.object_id}).call(args)
      ObjectSpace._id2ref(#{ret_val.object_id})
      end
  END_EVAL

  assert_equal(ret_val, yield) # Invoke test
  assert_equal(expected_args, call_args)
ensure
  object.instance_eval <<-END_EVAL, __FILE__, __LINE__ + 1
    undef #{method}
    alias #{method} #{method}_org
  END_EVAL
end

Any thoughts gratefully received! Many thanks.

@eregon eregon self-assigned this May 25, 2021
@eregon
Copy link
Member

eregon commented May 25, 2021

Thank you for the detailed report!

The RangeError: 0x0000000000000100 is not id value means the object was not found.
I guess what's happening here is a side effect of using frozen_string_literal: true, which means some Strings now are the interned frozen literal strings (instead of regular mutable Strings), and those don't seem to be handled in ObjectSpace._id2ref yet (a bug).

@eregon eregon added the bug label May 25, 2021
@eregon
Copy link
Member

eregon commented May 25, 2021

A fix for existing TruffleRuby versions would be to avoid using ObjectSpace._id2ref (which is not recommended API anyway).
I think one here would be to use define_method with a block, and then there would be no need for object_id + _id2ref, the code could simply refer to variables captured by the block.

@hainesr
Copy link
Author

hainesr commented May 25, 2021

Many thanks for your quick response!

I'm certainly not wedded to that code as it is - I'd be more than happy to refactor it once I am certain that I fully understand it (it's not mine originally, you see) 😄

Thanks!

@chrisseaton
Copy link
Collaborator

I think you'll find your _id2ref code possibly won't work on JRuby either.

irb(main):001:0> RUBY_DESCRIPTION
=> "jruby 9.2.17.0 (2.5.8) 2021-03-29 84d363da97 OpenJDK 64-Bit Server VM 16+36-2231 on 16+36-2231 +jit [darwin-x86_64]"
irb(main):002:0> string = 'foo'
=> "foo"
irb(main):003:0> ObjectSpace._id2ref(string.object_id)
(irb):3: warning: ObjectSpace is disabled; _id2ref only supports immediates, pass -X+O to enable
Traceback (most recent call last):
        7: from /Users/chrisseaton/.rubies/jruby-9.2.17.0/bin/irb:13:in `<main>'
        6: from org/jruby/RubyKernel.java:1189:in `catch'
        5: from org/jruby/RubyKernel.java:1189:in `catch'
        4: from org/jruby/RubyKernel.java:1442:in `loop'
        3: from org/jruby/RubyKernel.java:1048:in `eval'
        2: from (irb):3:in `evaluate'
        1: from org/jruby/RubyObjectSpace.java:113:in `_id2ref'
RangeError (0x00000000000007d2 is not id value)

@hainesr
Copy link
Author

hainesr commented May 25, 2021

It does work with JRuby but we have to turn ObjectSpace on for the tests, which I know is not ideal. I'd be very happy to rid myself of that bit of code! I'll stare at it a bit tonight and see if I can figure out what it's doing...

@chrisseaton
Copy link
Collaborator

I can see what it's doing and why - it's hard to pass in a reference to an object to an eval string. If I have object foo, and I want to eval('foo.bar'), how do I pass foo in? That's what they're doing - eval('_id2ref(#{foo.object_id})'. You can fix this by passing the binding to eval, which then lets it directly reference foo, and using define_method rather than def so again it can capture foo from that binding.

@eregon
Copy link
Member

eregon commented May 25, 2021

Actually neither eval nor _id2ref are needed: rubyzip/rubyzip#483

@eregon eregon added this to the 21.2.0 milestone May 25, 2021
@hainesr
Copy link
Author

hainesr commented May 25, 2021

Thank you both @eregon, @chrisseaton for your attention and help with this!

graalvmbot pushed a commit that referenced this issue May 25, 2021
@eregon
Copy link
Member

eregon commented May 26, 2021

Just to clarify, the _id2ref bug is fixed in beb45b8 (the issue was automatically closed by the commit fixing it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants