Skip to content

Commit 27b9496

Browse files
eregoneregon
committed
[GR-28912] Refactor frame and special variable reading to avoid races.
PullRequest: truffleruby/2371
2 parents ec62fae + 66d3f13 commit 27b9496

11 files changed

+206
-255
lines changed

‎spec/truffle/caller_data_spec.rb

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
# truffleruby_primitives: true
2+
3+
# Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved. This
4+
# code is released under a tri EPL/GPL/LGPL license. You can use it,
5+
# redistribute it and/or modify it under the terms of the:
6+
#
7+
# Eclipse Public License version 2.0, or
8+
# GNU General Public License version 2, or
9+
# GNU Lesser General Public License version 2.1.
10+
11+
require_relative '../ruby/spec_helper'
12+
13+
module TruffleCallerSpecFixtures
14+
15+
def self.last_line_set(last_line)
16+
Primitive.io_last_line_set(Primitive.caller_special_variables, last_line)
17+
last_line
18+
end
19+
20+
def self.last_match_set(match)
21+
Primitive.regexp_last_match_set(Primitive.caller_special_variables, match)
22+
match
23+
end
24+
25+
def self.caller_binding
26+
Primitive.caller_binding
27+
end
28+
29+
def self.caller_binding_and_variables(last_line, last_match)
30+
b = Primitive.caller_binding
31+
Primitive.io_last_line_set(Primitive.caller_special_variables, last_line)
32+
Primitive.regexp_last_match_set(Primitive.caller_special_variables, last_match)
33+
b
34+
end
35+
36+
end
37+
38+
describe "A caller" do
39+
40+
it "can have its special variables read and modified" do
41+
last_line = "Hello!"
42+
md = Primitive.matchdata_create(/o/, "Hello", [4], [5])
43+
TruffleCallerSpecFixtures.last_line_set(last_line)
44+
TruffleCallerSpecFixtures.last_match_set(md)
45+
$_.should == last_line
46+
$~.should == md
47+
end
48+
49+
it "can have its special variables read and modified through an intermediate #send" do
50+
last_line = "Hello!"
51+
md = Primitive.matchdata_create(/o/, "Hello", [4], [5])
52+
TruffleCallerSpecFixtures.send(:last_line_set, last_line)
53+
TruffleCallerSpecFixtures.send(:last_match_set, md)
54+
$_.should == last_line
55+
$~.should == md
56+
end
57+
58+
it "can have its special variables and frame read by the same method" do
59+
last_line = "Hello!"
60+
md = Primitive.matchdata_create(/o/, "Hello", [4], [5])
61+
b = TruffleCallerSpecFixtures.caller_binding_and_variables(last_line, md)
62+
$_.should == last_line
63+
$~.should == md
64+
b.local_variable_get(:md).should == md
65+
end
66+
67+
it "can have its special variables and frame read by the same method through an intermediate #send" do
68+
last_line = "Hello!"
69+
md = Primitive.matchdata_create(/o/, "Hello", [4], [5])
70+
b = TruffleCallerSpecFixtures.send(:caller_binding_and_variables, last_line, md)
71+
$_.should == last_line
72+
$~.should == md
73+
b.local_variable_get(:md).should == md
74+
end
75+
76+
end

‎src/main/java/org/truffleruby/core/kernel/TruffleKernelNodes.java

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@
2727
import org.truffleruby.core.module.RubyModule;
2828
import org.truffleruby.core.proc.RubyProc;
2929
import org.truffleruby.core.symbol.RubySymbol;
30+
import org.truffleruby.language.FrameOrVariablesReadingNode;
3031
import org.truffleruby.language.Nil;
32+
import org.truffleruby.language.ReadOwnFrameAndVariablesNode;
3133
import org.truffleruby.language.RubyContextNode;
3234
import org.truffleruby.language.RubyNode;
3335
import org.truffleruby.language.RubyRootNode;
@@ -227,7 +229,8 @@ private static FrameSlot getVariableSlot(MaterializedFrame frame) {
227229
}
228230

229231
@ImportStatic({ Layouts.class, TruffleKernelNodes.class })
230-
public abstract static class GetSpecialVariableStorage extends RubyContextNode {
232+
public abstract static class GetSpecialVariableStorage extends RubyContextNode
233+
implements FrameOrVariablesReadingNode {
231234

232235
public abstract SpecialVariableStorage execute(Frame frame);
233236

@@ -315,6 +318,18 @@ public static SpecialVariableStorage getSlow(MaterializedFrame aFrame) {
315318
public static GetSpecialVariableStorage create() {
316319
return GetSpecialVariableStorageNodeGen.create();
317320
}
321+
322+
@Override
323+
public void startSending(boolean variables, boolean frame) {
324+
if (frame) {
325+
replace(new ReadOwnFrameAndVariablesNode(), "Starting to read own frame and variables");
326+
}
327+
}
328+
329+
@Override
330+
public boolean sendingFrame() {
331+
return false;
332+
}
318333
}
319334

320335
@Primitive(name = "caller_special_variables")

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,6 @@ public InternalMethod getCallingMethod() {
9898
return tryGetMethod(getCallerFrame(FrameAccess.READ_ONLY));
9999
}
100100

101-
public boolean callerIsSend() {
102-
return false; // TODO (eregon, 2 Feb 2021): simplify callers
103-
}
104-
105101
// SourceSection
106102

107103
@TruffleBoundary

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

Lines changed: 26 additions & 189 deletions
Original file line numberDiff line numberDiff line change
@@ -9,240 +9,77 @@
99
*/
1010
package org.truffleruby.language;
1111

12-
import com.oracle.truffle.api.CompilerDirectives;
1312
import com.oracle.truffle.api.frame.Frame;
14-
import com.oracle.truffle.api.nodes.InvalidAssumptionException;
1513
import org.truffleruby.SuppressFBWarnings;
1614
import org.truffleruby.core.binding.RubyBinding;
1715
import org.truffleruby.core.kernel.TruffleKernelNodes.GetSpecialVariableStorage;
18-
import org.truffleruby.language.arguments.ReadCallerDataNode;
19-
import org.truffleruby.language.arguments.ReadCallerFrameAndVariablesNode;
2016
import org.truffleruby.language.arguments.ReadCallerFrameNode;
2117
import org.truffleruby.language.arguments.ReadCallerVariablesNode;
2218

23-
import com.oracle.truffle.api.Assumption;
24-
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
25-
import com.oracle.truffle.api.nodes.Node;
26-
import com.oracle.truffle.api.utilities.AlwaysValidAssumption;
2719
import org.truffleruby.language.methods.DeclarationContext;
2820
import org.truffleruby.language.threadlocal.SpecialVariableStorage;
2921

3022
/** Some Ruby methods need access to the caller frame (the frame active when the method call was made) or to the storage
3123
* of special variables within that frame: see usages of {@link ReadCallerFrameNode} and {@link ReadCallerVariablesNode}
3224
* . This is notably used to get hold of instances of {@link DeclarationContext} and {@link RubyBinding} and methods
33-
* which need to access the last regexp match or the last io line.
25+
* which need to access the last regexp MatchData or the last IO line.
3426
*
3527
* <p>
3628
* This means that when making a method call, we might need to pass down its {@link Frame} or
3729
* {@link SpecialVariableStorage} active when the method call was made.
3830
*
3931
* <p>
40-
* When retrieving the frame or special variable storage in a method called through the Ruby {@code #send} method, we
41-
* must not retrieve the frame of the actual call (made by {@code #send}) but the frame of the {@code #send} call
42-
* itself.
43-
*
44-
* <p>
4532
* Materializing a frame is expensive, and the point of this parent node is to only materialize the frame when we know
4633
* for sure it has been requested by the callee. It is also possible to walk the stack to retrieve the frame to
4734
* materialize - but this is even slower and causes a deoptimization in the callee every time we walk the stack.
4835
*
4936
* <p>
50-
* This class works in tandem with {@link ReadCallerFrameNode} for this purpose. At first, we don't send down the frame.
51-
* If the callee needs it, it will de-optimize and walk the stack to retrieve it (slow). It will also call
37+
* This class works in tandem with {@link FrameOrVariablesReadingNode} for this purpose. At first, we don't send down
38+
* the frame. If the callee needs it, it will de-optimize and walk the stack to retrieve it (slow). It will also call
5239
* {@link #startSendingOwnFrame()}}, so that the next time the method is called, the frame will be passed down and the
53-
* method does not need further de-optimizations. (Note in the case of {@code #send} calls, we need to recursively call
54-
* {@link ReadCallerFrameNode} to get the parent frame!) {@link ReadCallerVariablesNode} is used similarly to access
55-
* special variable storage, but for child nodes that only require access to this storage ensures they receive an object
56-
* that will not require node splitting to be accessed efficiently.
40+
* method does not need further de-optimizations.
5741
*
5842
* <p>
59-
* This class is the sole consumer of {@link RubyRootNode#getNeedsCallerAssumption()}, which is used to optimize
60-
* {@link #getFrameOrStorageIfRequired(Frame)} (called by subclasses in order to pass down the frame or not). Starting
61-
* to send the frame invalidates the assumption. In other words, the assumption guards the fact that {@link #sendsFrame}
62-
* is a compilation constant, and is invalidated whenever it needs to change. */
43+
* {@link ReadCallerVariablesNode} is used similarly to access special variable storage, but for child nodes that only
44+
* require access to this storage ensures they receive an object that will not require node splitting to be accessed
45+
* efficiently. */
6346
@SuppressFBWarnings("IS")
6447
public abstract class FrameAndVariablesSendingNode extends RubyContextNode {
6548

66-
private enum SendsFrame {
67-
NO_FRAME, // callees don't need to read the frame
68-
MY_FRAME, // for most calls
69-
CALLER_FRAME; // for `send` calls
70-
}
71-
72-
@CompilationFinal protected SendsFrame sendsFrame = SendsFrame.NO_FRAME;
73-
@CompilationFinal protected Assumption needsCallerAssumption;
74-
@CompilationFinal protected SendsFrame sendsVariables = SendsFrame.NO_FRAME;
75-
76-
@Child protected ReadCallerDataNode readCaller;
77-
@Child protected GetSpecialVariableStorage readMyVariables;
78-
79-
/** Whether we are sending down the frame (because the called method reads it). */
80-
protected boolean sendingFrames() {
81-
return sendsFrame != SendsFrame.NO_FRAME;
82-
}
83-
84-
protected boolean sendingVariables() {
85-
return sendsVariables != SendsFrame.NO_FRAME;
86-
}
87-
88-
public void startSendingOwnFrame() {
89-
if (getContext().getCallStack().callerIsSend()) {
90-
startSendingFrame(SendsFrame.CALLER_FRAME);
91-
} else {
92-
startSendingFrame(SendsFrame.MY_FRAME);
93-
}
94-
}
95-
96-
public void startSendingOwnVariables() {
97-
if (getContext().getCallStack().callerIsSend()) {
98-
startSendingVariables(SendsFrame.CALLER_FRAME);
99-
} else {
100-
startSendingVariables(SendsFrame.MY_FRAME);
101-
}
102-
}
103-
104-
public void startSendingOwnFrameAndVariables() {
105-
if (getContext().getCallStack().callerIsSend()) {
106-
startSendingFrameAndVariables(SendsFrame.CALLER_FRAME);
107-
} else {
108-
startSendingFrameAndVariables(SendsFrame.MY_FRAME);
109-
}
110-
}
111-
112-
private synchronized void startSendingFrame(SendsFrame frameToSend) {
113-
if (sendingFrames()) {
114-
assert sendsFrame == frameToSend;
115-
return;
116-
}
117-
118-
if (sendingVariables()) {
119-
assert sendsVariables == frameToSend;
120-
}
121-
122-
// We'd only get AlwaysValidAssumption if the root node isn't Ruby (in which case this shouldn't be called),
123-
// or when we already know to send the frame (in which case we'd have exited above).
124-
assert needsCallerAssumption != AlwaysValidAssumption.INSTANCE;
49+
@Child protected FrameOrVariablesReadingNode readingNode;
12550

126-
this.sendsFrame = frameToSend;
127-
if (frameToSend == SendsFrame.CALLER_FRAME) {
128-
if (!sendingVariables()) {
129-
this.readCaller = insert(new ReadCallerFrameNode());
130-
} else {
131-
this.readCaller = readCaller.replace(new ReadCallerFrameAndVariablesNode());
132-
}
133-
}
134-
Node root = getRootNode();
135-
if (root instanceof RubyRootNode) {
136-
((RubyRootNode) root).invalidateNeedsCallerAssumption();
51+
public boolean sendingFrames() {
52+
if (readingNode == null) {
53+
return false;
13754
} else {
138-
throw new Error();
55+
return readingNode.sendingFrame();
13956
}
14057
}
14158

142-
private synchronized void startSendingVariables(SendsFrame variablesToSend) {
143-
if (sendingFrames()) {
144-
assert sendsFrame == variablesToSend;
145-
}
146-
147-
if (sendingVariables()) {
148-
assert sendsVariables == variablesToSend;
149-
return;
150-
}
151-
152-
// We'd only get AlwaysValidAssumption if the root node isn't Ruby (in which case this shouldn't be called),
153-
// or when we already know to send the frame (in which case we'd have exited above).
154-
assert needsCallerAssumption != AlwaysValidAssumption.INSTANCE;
155-
156-
this.sendsVariables = variablesToSend;
157-
if (variablesToSend == SendsFrame.CALLER_FRAME) {
158-
if (!sendingFrames()) {
159-
this.readCaller = insert(new ReadCallerVariablesNode());
160-
} else {
161-
this.readCaller = readCaller.replace(new ReadCallerFrameAndVariablesNode());
162-
}
163-
} else {
164-
this.readMyVariables = insert(GetSpecialVariableStorage.create());
165-
}
166-
Node root = getRootNode();
167-
if (root instanceof RubyRootNode) {
168-
((RubyRootNode) root).invalidateNeedsVariablesAssumption();
169-
} else {
170-
throw new Error();
59+
private synchronized void startSending(boolean variables, boolean frame) {
60+
if (readingNode != null) {
61+
readingNode.startSending(variables, frame);
62+
} else if (variables && !frame) {
63+
readingNode = insert(GetSpecialVariableStorage.create());
64+
} else if (!variables && frame) {
65+
readingNode = insert(new ReadOwnFrameNode());
17166
}
17267
}
17368

174-
private synchronized void startSendingFrameAndVariables(SendsFrame dataToSend) {
175-
if (sendingFrames()) {
176-
assert sendsFrame == dataToSend;
177-
return;
178-
}
179-
180-
if (sendingVariables()) {
181-
assert sendsFrame == dataToSend;
182-
return;
183-
}
184-
185-
// We'd only get AlwaysValidAssumption if the root node isn't Ruby (in which case this shouldn't be called),
186-
// or when we already know to send the frame (in which case we'd have exited above).
187-
assert needsCallerAssumption != AlwaysValidAssumption.INSTANCE;
188-
189-
this.sendsFrame = dataToSend;
190-
this.sendsVariables = dataToSend;
191-
if (dataToSend == SendsFrame.CALLER_FRAME) {
192-
if (sendingFrames() || sendingVariables()) {
193-
this.readCaller = readCaller.replace(new ReadCallerFrameAndVariablesNode());
194-
} else {
195-
this.readCaller = insert(new ReadCallerFrameNode());
196-
}
197-
}
198-
Node root = getRootNode();
199-
if (root instanceof RubyRootNode) {
200-
((RubyRootNode) root).invalidateNeedsCallerAssumption();
201-
} else {
202-
throw new Error();
203-
}
69+
/** Whether we are sending down the frame (because the called method reads it). */
70+
public void startSendingOwnFrame() {
71+
startSending(false, true);
20472
}
20573

206-
private synchronized void resetNeedsCallerAssumption() {
207-
Node root = getRootNode();
208-
if (root instanceof RubyRootNode && (!sendingFrames() || !sendingVariables())) {
209-
needsCallerAssumption = ((RubyRootNode) root).getNeedsCallerAssumption();
210-
} else {
211-
needsCallerAssumption = AlwaysValidAssumption.INSTANCE;
212-
}
74+
public void startSendingOwnVariables() {
75+
startSending(true, false);
21376
}
21477

21578
public Object getFrameOrStorageIfRequired(Frame frame) {
216-
if (frame == null) { // the frame should be proved null or non-null at PE time
217-
return null;
218-
}
219-
220-
if (needsCallerAssumption == null) {
221-
CompilerDirectives.transferToInterpreterAndInvalidate();
222-
resetNeedsCallerAssumption();
223-
}
224-
try {
225-
needsCallerAssumption.check();
226-
} catch (InvalidAssumptionException e) {
227-
CompilerDirectives.transferToInterpreterAndInvalidate();
228-
resetNeedsCallerAssumption();
229-
}
230-
231-
if (sendsVariables == SendsFrame.NO_FRAME && sendsFrame == SendsFrame.NO_FRAME) {
232-
return null;
233-
} else if (sendsVariables == SendsFrame.MY_FRAME && sendsFrame == SendsFrame.NO_FRAME) {
234-
return readMyVariables.execute(frame);
235-
} else if (sendsVariables == SendsFrame.NO_FRAME && sendsFrame == SendsFrame.MY_FRAME) {
236-
return frame.materialize();
237-
} else if (sendsVariables == SendsFrame.MY_FRAME && sendsFrame == SendsFrame.MY_FRAME) {
238-
return new FrameAndVariables(readMyVariables.execute(frame), frame.materialize());
239-
} else if ((sendsVariables == SendsFrame.CALLER_FRAME && sendsFrame == SendsFrame.MY_FRAME) ||
240-
(sendsVariables == SendsFrame.MY_FRAME && sendsFrame == SendsFrame.CALLER_FRAME)) {
241-
CompilerDirectives.transferToInterpreterAndInvalidate();
242-
CompilerDirectives.shouldNotReachHere();
79+
if (readingNode == null) {
24380
return null;
24481
} else {
245-
return readCaller.execute(frame);
82+
return readingNode.execute(frame);
24683
}
24784
}
24885

0 commit comments

Comments
 (0)