Skip to content

Commit ed93fe9

Browse files
eregoneregon
committed
Use a RubyLambdaRootNode for define_method(name, &block) methods to preserve lambda semantics
* Fixes #2418 * Remove RubyRootNode#getBody() as it feels like an anti-pattern (it discards semantics in the RootNode and instrumentation nodes might be in between). * Make Symbol#to_proc use a RubyLambdaRootNode for consistency and simplicity.
1 parent 75b1e8e commit ed93fe9

14 files changed

+79
-70
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ Bug fixes:
1313
* `BasicSocket#*_nonblock(exception: false)` now only return `:wait_readable/:wait_writable` for `EAGAIN`/`EWOULDBLOCK` like MRI (#2400).
1414
* Fix issue with `strspn` used in the `date` C extension compiled as a macro on older glibc and then missing the `__strspn_c1` symbol on newer glibc (#2406).
1515
* Fix constant lookup when loading the same file multiple times (#2408).
16+
* Fix handling of `break`, `next` and `redo` in `define_method(name, &block)` methods (#2418).
1617

1718
Compatibility:
1819

‎src/main/java/org/truffleruby/builtins/CoreMethodNodeManager.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,8 +267,7 @@ private static void addMethod(
267267
if (alwaysInlined) {
268268
callTarget = callTargetFactory.apply(sharedMethodInfo);
269269
callTargetSupplier = null;
270-
final RubyRootNode rootNode = RubyRootNode.of(callTarget);
271-
alwaysInlinedNodeFactory = ((ReRaiseInlinedExceptionNode) rootNode.getBody()).nodeFactory;
270+
alwaysInlinedNodeFactory = RubyRootNode.of(callTarget).getAlwaysInlinedNodeFactory();
272271
} else {
273272
if (context.getLanguageSlow().options.LAZY_CALLTARGETS) {
274273
callTarget = null;

‎src/main/java/org/truffleruby/core/method/MethodNodes.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ private RubyProc createProc(RootCallTarget callTarget, InternalMethod method, Ob
310310
getLanguage().procShape,
311311
ProcType.LAMBDA,
312312
method.getSharedMethodInfo(),
313-
new ProcCallTargets(callTarget, callTarget),
313+
new ProcCallTargets(callTarget),
314314
declarationFrame,
315315
variables,
316316
method,

‎src/main/java/org/truffleruby/core/module/ModuleNodes.java

Lines changed: 11 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,6 @@
8383
import org.truffleruby.language.RubyDynamicObject;
8484
import org.truffleruby.language.RubyGuards;
8585
import org.truffleruby.language.RubyLambdaRootNode;
86-
import org.truffleruby.language.RubyMethodRootNode;
8786
import org.truffleruby.language.RubyNode;
8887
import org.truffleruby.language.RubyRootNode;
8988
import org.truffleruby.language.Visibility;
@@ -138,7 +137,6 @@
138137
import com.oracle.truffle.api.frame.VirtualFrame;
139138
import com.oracle.truffle.api.nodes.IndirectCallNode;
140139
import com.oracle.truffle.api.nodes.Node;
141-
import com.oracle.truffle.api.nodes.NodeUtil;
142140
import com.oracle.truffle.api.profiles.BranchProfile;
143141
import com.oracle.truffle.api.source.SourceSection;
144142

@@ -1323,23 +1321,13 @@ private RubySymbol defineMethodInternal(RubyModule module, String name, RubyUnbo
13231321
@TruffleBoundary
13241322
private RubySymbol defineMethod(RubyModule module, String name, RubyProc proc,
13251323
MaterializedFrame callerFrame) {
1326-
final RubyRootNode rootNode = RubyRootNode.of(proc.callTargets.getCallTargetForLambda());
1324+
final RootCallTarget callTargetForLambda = proc.callTargets.getCallTargetForLambda();
1325+
final RubyLambdaRootNode rootNode = RubyLambdaRootNode.of(callTargetForLambda);
13271326
final SharedMethodInfo info = proc.sharedMethodInfo.forDefineMethod(module, name);
1328-
final Arity arityForCheck = rootNode instanceof RubyLambdaRootNode
1329-
? ((RubyLambdaRootNode) rootNode).arityForCheck
1330-
: info.getArity();
1331-
1332-
final RubyNode body = NodeUtil.cloneNode(rootNode.getBody());
1333-
final RubyNode newBody = new CallMethodWithProcBody(proc.declarationFrame, body);
1334-
final RubyMethodRootNode newRootNode = new RubyMethodRootNode(
1335-
getLanguage(),
1336-
info.getSourceSection(),
1337-
rootNode.getFrameDescriptor(),
1338-
info,
1339-
newBody,
1340-
Split.HEURISTIC,
1341-
rootNode.returnID,
1342-
arityForCheck);
1327+
final RubyNode body = rootNode.copyBody();
1328+
final RubyNode newBody = new CallMethodWithLambdaBody(proc.declarationFrame, body);
1329+
1330+
final RubyLambdaRootNode newRootNode = rootNode.copyRootNode(info, newBody);
13431331
final RootCallTarget newCallTarget = Truffle.getRuntime().createCallTarget(newRootNode);
13441332

13451333
final InternalMethod method = InternalMethod.fromProc(
@@ -1354,20 +1342,20 @@ private RubySymbol defineMethod(RubyModule module, String name, RubyProc proc,
13541342
return addMethod(module, name, method, callerFrame);
13551343
}
13561344

1357-
private static class CallMethodWithProcBody extends RubyContextSourceNode {
1345+
private static class CallMethodWithLambdaBody extends RubyContextSourceNode {
13581346

13591347
private final MaterializedFrame declarationFrame;
1360-
@Child private RubyNode procBody;
1348+
@Child private RubyNode lambdaBody;
13611349

1362-
public CallMethodWithProcBody(MaterializedFrame declarationFrame, RubyNode procBody) {
1350+
public CallMethodWithLambdaBody(MaterializedFrame declarationFrame, RubyNode lambdaBody) {
13631351
this.declarationFrame = declarationFrame;
1364-
this.procBody = procBody;
1352+
this.lambdaBody = lambdaBody;
13651353
}
13661354

13671355
@Override
13681356
public Object execute(VirtualFrame frame) {
13691357
RubyArguments.setDeclarationFrame(frame, declarationFrame);
1370-
return procBody.execute(frame);
1358+
return lambdaBody.execute(frame);
13711359
}
13721360

13731361
}

‎src/main/java/org/truffleruby/core/proc/ProcCallTargets.java

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
import com.oracle.truffle.api.CompilerDirectives;
1313
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
1414
import com.oracle.truffle.api.RootCallTarget;
15+
import com.oracle.truffle.api.nodes.RootNode;
16+
import org.truffleruby.language.RubyLambdaRootNode;
17+
import org.truffleruby.language.RubyProcRootNode;
1518
import org.truffleruby.language.RubyRootNode;
1619
import org.truffleruby.language.methods.BlockDefinitionNode;
1720
import org.truffleruby.parser.MethodTranslator;
@@ -37,21 +40,24 @@ public ProcCallTargets(
3740
RootCallTarget callTargetForLambda,
3841
Supplier<RootCallTarget> altCallTargetCompiler) {
3942
assert callTargetForProc != null || callTargetForLambda != null;
43+
assert callTargetForProc == null || validProcRootNode(callTargetForProc);
44+
assert callTargetForLambda == null || validLambdaRootNode(callTargetForLambda);
4045

4146
this.callTargetForProc = callTargetForProc;
4247
this.callTargetForLambda = callTargetForLambda;
4348
this.altCallTargetCompiler = altCallTargetCompiler;
4449
}
4550

46-
public ProcCallTargets(RootCallTarget callTargetForProc, RootCallTarget callTargetForLambda) {
47-
this(callTargetForProc, callTargetForLambda, null);
51+
public ProcCallTargets(RootCallTarget callTargetForLambda) {
52+
this(callTargetForLambda, callTargetForLambda, null);
4853
}
4954

5055
public RootCallTarget getCallTargetForProc() {
5156
if (callTargetForProc == null) {
5257
CompilerDirectives.transferToInterpreterAndInvalidate();
5358
callTargetForProc = altCallTargetCompiler.get();
5459
copySplit(callTargetForLambda, callTargetForProc);
60+
assert validProcRootNode(callTargetForProc);
5561
altCallTargetCompiler = null;
5662
}
5763
return callTargetForProc;
@@ -62,6 +68,7 @@ public RootCallTarget getCallTargetForLambda() {
6268
CompilerDirectives.transferToInterpreterAndInvalidate();
6369
callTargetForLambda = altCallTargetCompiler.get();
6470
copySplit(callTargetForProc, callTargetForLambda);
71+
assert validLambdaRootNode(callTargetForLambda);
6572
altCallTargetCompiler = null;
6673
}
6774
return callTargetForLambda;
@@ -70,4 +77,19 @@ public RootCallTarget getCallTargetForLambda() {
7077
private void copySplit(RootCallTarget src, RootCallTarget dst) {
7178
RubyRootNode.of(dst).setSplit(RubyRootNode.of(src).getSplit());
7279
}
80+
81+
private boolean validProcRootNode(RootCallTarget callTarget) {
82+
final RootNode rootNode = callTarget.getRootNode();
83+
assert rootNode instanceof RubyProcRootNode ||
84+
// RubyLambdaRootNode is used for the ProcCallTargets(RootCallTarget callTargetForLambda) constructor
85+
rootNode instanceof RubyLambdaRootNode : rootNode + " " + rootNode.getClass();
86+
return true;
87+
}
88+
89+
/** {@link ModuleNodes.DefineMethodNode} relies on this always being a RubyLambdaRootNode */
90+
private boolean validLambdaRootNode(RootCallTarget callTarget) {
91+
final RootNode rootNode = callTarget.getRootNode();
92+
assert rootNode instanceof RubyLambdaRootNode : rootNode + " " + rootNode.getClass();
93+
return true;
94+
}
7395
}

‎src/main/java/org/truffleruby/core/symbol/SymbolNodes.java

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,12 @@
3232
import org.truffleruby.core.string.StringNodes;
3333
import org.truffleruby.language.LexicalScope;
3434
import org.truffleruby.language.RubyBaseNode;
35+
import org.truffleruby.language.RubyLambdaRootNode;
3536
import org.truffleruby.language.RubyRootNode;
3637
import org.truffleruby.language.Visibility;
3738
import org.truffleruby.language.arguments.ReadCallerFrameNode;
3839
import org.truffleruby.language.arguments.RubyArguments;
40+
import org.truffleruby.language.control.BreakID;
3941
import org.truffleruby.language.control.RaiseException;
4042
import org.truffleruby.language.control.ReturnID;
4143
import org.truffleruby.language.methods.Arity;
@@ -215,7 +217,7 @@ public static RubyProc createProc(RubyContext context, RubyLanguage language,
215217
language.procShape,
216218
ProcType.PROC,
217219
RubyRootNode.of(callTarget).getSharedMethodInfo(),
218-
new ProcCallTargets(callTarget, callTarget),
220+
new ProcCallTargets(callTarget),
219221
declarationFrame,
220222
variables,
221223
method,
@@ -239,14 +241,18 @@ public static RootCallTarget createCallTarget(RubyLanguage language, RubySymbol
239241
"Symbol#to_proc",
240242
ArgumentDescriptor.ANON_REST);
241243

242-
final RubyRootNode rootNode = new RubyRootNode(
244+
// ModuleNodes.DefineMethodNode relies on the lambda CallTarget to always use a RubyLambdaRootNode,
245+
// and we want to use a single CallTarget for both proc and lambda.
246+
final RubyLambdaRootNode rootNode = new RubyLambdaRootNode(
243247
language,
244248
sourceSection,
245249
new FrameDescriptor(nil),
246250
sharedMethodInfo,
247251
new SymbolProcNode(symbol.getString()),
248252
Split.HEURISTIC,
249-
ReturnID.INVALID);
253+
ReturnID.INVALID,
254+
BreakID.INVALID,
255+
ARITY);
250256

251257
return Truffle.getRuntime().createCallTarget(rootNode);
252258
}

‎src/main/java/org/truffleruby/extra/TruffleGraalNodes.java

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.truffleruby.builtins.PrimitiveNode;
1919
import org.truffleruby.core.cast.ToCallTargetNode;
2020
import org.truffleruby.core.proc.ProcCallTargets;
21+
import org.truffleruby.core.proc.ProcType;
2122
import org.truffleruby.core.proc.RubyProc;
2223
import org.truffleruby.language.RubyLambdaRootNode;
2324
import org.truffleruby.language.RubyNode;
@@ -85,31 +86,22 @@ public abstract static class CopyCapturedLocalsNode extends CoreMethodArrayArgum
8586
@Specialization(guards = "proc.isLambda()")
8687
protected RubyProc copyCapturedLocals(RubyProc proc) {
8788
final RubyLambdaRootNode rootNode = RubyLambdaRootNode.of(proc.callTarget);
88-
final RubyNode newBody = NodeUtil.cloneNode(rootNode.getBody());
89+
final RubyNode newBody = rootNode.copyBody();
8990

9091
assert NodeUtil.findAllNodeInstances(newBody, WriteDeclarationVariableNode.class).isEmpty();
9192

9293
for (ReadDeclarationVariableNode readNode : NodeUtil
9394
.findAllNodeInstances(newBody, ReadDeclarationVariableNode.class)) {
94-
MaterializedFrame frame = RubyArguments
95+
final MaterializedFrame frame = RubyArguments
9596
.getDeclarationFrame(proc.declarationFrame, readNode.getFrameDepth() - 1);
96-
Object value = frame.getValue(readNode.getFrameSlot());
97+
final Object value = frame.getValue(readNode.getFrameSlot());
9798
readNode.replace(new ObjectLiteralNode(value));
9899
}
99-
final RubyLambdaRootNode newRootNode = new RubyLambdaRootNode(
100-
getLanguage(),
101-
rootNode.getSourceSection(),
102-
rootNode.getFrameDescriptor(),
103-
rootNode.getSharedMethodInfo(),
104-
newBody,
105-
Split.HEURISTIC,
106-
rootNode.returnID,
107-
rootNode.breakID,
108-
rootNode.arityForCheck);
109100

101+
final RubyLambdaRootNode newRootNode = rootNode.copyRootNode(rootNode.getSharedMethodInfo(), newBody);
110102
final RootCallTarget newCallTarget = Truffle.getRuntime().createCallTarget(newRootNode);
111103

112-
SpecialVariableStorage variables = proc.declarationVariables;
104+
final SpecialVariableStorage variables = proc.declarationVariables;
113105

114106
final Object[] args = RubyArguments
115107
.pack(
@@ -132,9 +124,9 @@ protected RubyProc copyCapturedLocals(RubyProc proc) {
132124
return new RubyProc(
133125
coreLibrary().procClass,
134126
getLanguage().procShape,
135-
proc.type,
127+
ProcType.LAMBDA,
136128
proc.sharedMethodInfo,
137-
new ProcCallTargets(newCallTarget, newCallTarget),
129+
new ProcCallTargets(newCallTarget),
138130
newCallTarget,
139131
newDeclarationFrame,
140132
variables,

‎src/main/java/org/truffleruby/language/RubyCheckArityRootNode.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,13 @@
2222
import org.truffleruby.language.methods.SharedMethodInfo;
2323
import org.truffleruby.language.methods.Split;
2424

25-
import com.oracle.truffle.api.RootCallTarget;
2625
import com.oracle.truffle.api.frame.FrameDescriptor;
2726
import com.oracle.truffle.api.frame.VirtualFrame;
2827
import com.oracle.truffle.api.profiles.BranchProfile;
2928
import com.oracle.truffle.api.source.SourceSection;
3029

3130
public abstract class RubyCheckArityRootNode extends RubyRootNode {
3231

33-
public static RubyCheckArityRootNode of(RootCallTarget callTarget) {
34-
return (RubyCheckArityRootNode) callTarget.getRootNode();
35-
}
36-
3732
@Child private CheckKeywordArityNode checkKeywordArityNode;
3833

3934
public final Arity arityForCheck;

‎src/main/java/org/truffleruby/language/RubyCoreMethodRootNode.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,12 @@
1818
import org.truffleruby.language.methods.TranslateExceptionNode;
1919

2020
import com.oracle.truffle.api.CompilerDirectives;
21-
import com.oracle.truffle.api.RootCallTarget;
2221
import com.oracle.truffle.api.frame.FrameDescriptor;
2322
import com.oracle.truffle.api.frame.VirtualFrame;
2423
import com.oracle.truffle.api.source.SourceSection;
2524

2625
public class RubyCoreMethodRootNode extends RubyCheckArityRootNode {
2726

28-
public static RubyCoreMethodRootNode of(RootCallTarget callTarget) {
29-
return (RubyCoreMethodRootNode) callTarget.getRootNode();
30-
}
31-
3227
@Child private TranslateExceptionNode translateExceptionNode;
3328

3429
public RubyCoreMethodRootNode(

‎src/main/java/org/truffleruby/language/RubyLambdaRootNode.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,19 @@ public RubyLambdaRootNode(
6464
this.breakID = breakID;
6565
}
6666

67+
public RubyLambdaRootNode copyRootNode(SharedMethodInfo newSharedMethodInfo, RubyNode newBody) {
68+
return new RubyLambdaRootNode(
69+
language,
70+
getSourceSection(),
71+
getFrameDescriptor(),
72+
newSharedMethodInfo,
73+
newBody,
74+
getSplit(),
75+
returnID,
76+
breakID,
77+
arityForCheck);
78+
}
79+
6780
@Override
6881
public Object execute(VirtualFrame frame) {
6982
TruffleSafepoint.poll(this);
@@ -96,7 +109,7 @@ public Object execute(VirtualFrame frame) {
96109
nextProfile.enter();
97110
return e.getResult();
98111
} catch (BreakException e) {
99-
if (matchingBreakProfile.profile(e.getBreakID() == breakID)) {
112+
if (matchingBreakProfile.profile(breakID != BreakID.INVALID && e.getBreakID() == breakID)) {
100113
return e.getResult();
101114
} else {
102115
throw e;

0 commit comments

Comments
 (0)