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

Raising a RuntimeError when calling Kernel#binding from a C/non-ruby frame. #3449

Closed
wants to merge 1 commit into from
Closed

Raising a RuntimeError when calling Kernel#binding from a C/non-ruby frame. #3449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion spec/tags/optional/capi/binding_tags.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
fails:CApiBindingSpecs Kernel#binding gives the top-most Ruby binding when called from C
fails:CApiBindingSpecs Kernel#binding raises when called from C
15 changes: 15 additions & 0 deletions src/main/java/org/truffleruby/core/kernel/KernelNodes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
import java.util.Objects;
import java.util.concurrent.TimeUnit;

import com.oracle.truffle.api.Truffle;
import com.oracle.truffle.api.dsl.Bind;
import com.oracle.truffle.api.dsl.Cached.Shared;
import com.oracle.truffle.api.dsl.Fallback;
import com.oracle.truffle.api.dsl.GenerateCached;
import com.oracle.truffle.api.dsl.GenerateInline;
import com.oracle.truffle.api.dsl.NeverDefault;
import com.oracle.truffle.api.frame.Frame;
import com.oracle.truffle.api.frame.FrameInstance;
import com.oracle.truffle.api.interop.InteropLibrary;
import com.oracle.truffle.api.interop.UnsupportedMessageException;
import com.oracle.truffle.api.object.PropertyGetter;
Expand Down Expand Up @@ -92,6 +94,7 @@
import org.truffleruby.interop.ToJavaStringNode;
import org.truffleruby.core.string.ImmutableRubyString;
import org.truffleruby.interop.TranslateInteropExceptionNode;
import org.truffleruby.language.CallStackManager;
import org.truffleruby.language.ImmutableRubyObject;
import org.truffleruby.language.LexicalScope;
import org.truffleruby.language.Nil;
Expand Down Expand Up @@ -334,6 +337,18 @@ RubyBinding binding(Frame callerFrame, Object self, Object[] rubyArgs, RootCallT
@Cached(
value = "getAdoptedNode(this).getEncapsulatingSourceSection()",
allowUncached = true, neverDefault = false) SourceSection sourceSection) {

final MaterializedFrame materializedCallerFrame = Truffle.getRuntime()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what the materializedCallerFrame looks like in the debugger:
image

Compared to when called from Ruby:
image

.iterateFrames(f -> f.getFrame(FrameInstance.FrameAccess.MATERIALIZE).materialize(), 1);
Copy link
Member

@eregon eregon Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely can't do iterateFrames in Kernel#binding.
Kernel#binding is very well optimized currently and iterateFrames would completely ruin its performance.

One way to detect C extension methods quickly is #3436 (comment).
But also I don't think this matters. We do have currently a Ruby frame for C extension methods, so it's only fair to return a Binding for that case.

The aim of this new exception is to not give an empty/unconnected binding for situations where there isn't a real binding.
For instance method(:binding).call is such a case, because Method#call is implemented in C/Java and so there is no way to get a proper Binding for Method#call, hence there it's clearer to raise an exception.
As mentioned in #3436 (comment), we should raise the new exception for method(:binding).call and add a spec for that.
And let's leave C extension methods (& rb_funcall) alone, i.e. let them return a Binding as they do, because there is a real Ruby frame in there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to try #3436 (comment) in this PR.
I tried it locally and test fast passed so maybe it's OK, it would need a full CI run to know better.
It's too risky for 24.0 but we can always merge it for 24.1.


if (!CallStackManager.isRubyFrame(materializedCallerFrame)) {
throw new RaiseException(
getContext(),
coreExceptions().runtimeError(
"Cannot create Binding object for non-Ruby caller",
this));
}

needCallerFrame(callerFrame, target);
return BindingNodes.createBinding(getContext(), getLanguage(), callerFrame.materialize(), sourceSection);
}
Expand Down
Loading