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.

Sign up for GitHub

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

Jump to bottom

[Truffle] Kernel#send should be more transparent #3013

Closed
eregon opened this issue Jun 2, 2015 · 7 comments
Closed

[Truffle] Kernel#send should be more transparent #3013

eregon opened this issue Jun 2, 2015 · 7 comments
Milestone
Non-Release

Comments

@eregon
Copy link
Member

eregon commented Jun 2, 2015

Quite a few shared specs use send and expect it to be the same as if it was an actual literal call.
For instance, spec/ruby/core/kernel/__callee___spec.rb, spec/ruby/core/module/private_spec.rb and many others.

In MRI, send seems to not even appear in the backtrace:

$ ruby -e 'send :foo'
-e:1:in `<main>': undefined method `foo' for main:Object (NoMethodError)

compared to:
ruby -e 'spawn :foo'
-e:1:in `spawn': no implicit conversion of Symbol into String (TypeError)
    from -e:1:in `<main>'

I am not sure sure how we can handle this, but ideally we would not create any additional frame between a send(:bla,1,2) and bla(1,2).

If we do not want to simulate that behavior, how could we reliably get the caller frame? Ignore frames which have Kernel#send for METHOD?
@eregon eregon added the truffle label Jun 2, 2015
@eregon eregon added this to the truffle-dev milestone Jun 2, 2015
@chrisseaton
Copy link
Contributor

Maybe we just need to hide the frame when we construct backtraces, and when we find the caller frame for other purposes?

eregon added a commit that referenced this issue Jun 3, 2015
@eregon
[Truffle] Ignore send for __method__, __callee__ and Module.nesting.
3b96ed5 
See #3013.
@eregon
Copy link
Member Author

eregon commented Jun 3, 2015

I modified RubyCallStack.getCallerMethod to ignore send since all its callers would expect it.
Let's see if there are other cases where we should ignore send.

eregon added a commit that referenced this issue Jun 5, 2015
@eregon
[Truffle] Get the appropriate caller frame for visibility methods.
50a6f77 
* See #3013. In this case ignoring all builtins might make sense.
@chrisseaton
Copy link
Contributor

Is this resolved for the issues you have? I'd rather not leave issues speculatively open in case something might still be a problem for people in the future. If you don't have a test case then it's not actionable and I'll close until we have one.

@eregon
Copy link
Member Author

eregon commented Jun 13, 2015

Yeah it seems OK for now as getCallerMethod does what we need and I did not meet new related problems.

@eregon eregon closed this as completed Jun 13, 2015
@eregon
Copy link
Member Author

eregon commented Jun 17, 2015

I replaced most uses of TruffleRuntime.getCallerFrame() by the RubyCallStack version ignoring send. This should match better the semantics but of course is not exactly fast, like for Kernel#binding.

A nice way to fix this would be to pass the caller frame if the target method needs it
and just pass it along in send, effectively ignoring send's frame.

eregon added a commit that referenced this issue Jun 17, 2015
@eregon
[Truffle] Use the caller ignoring send in module_eval.
1e4f7f1 
* See #3013.
eregon added a commit that referenced this issue Jun 17, 2015
@eregon
[Truffle] Replace most uses of TruffleRuntime.getCallerFrame() by the…
2b2d2a1 
… RubyCallStack version.

* Ignores #send as it should for these cases.
* See #3013.
@eregon
Copy link
Member Author

eregon commented Jun 17, 2015

The two remaining usages would work just fine with the RubyCallStack version as well, but in these cases ignoring send does not change anything.

@chrisseaton
Copy link
Contributor

A nice way to fix this would be to pass the caller frame if the target method needs it

Yes, we need to do this for another reason as well - iterateFrames is boundary, so we can't constant fold through it at the moment. This in turns stops us constant folding through things like binding.local_variable_get.

@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Non-Release
Development

No branches or pull requests
3 participants
@enebo @eregon @chrisseaton