Call plus ensure does not properly initialize all temps #3969
Comments
|
Can you paste a ruby snippet to help me debug this? This might actually be as simple as the CFG missing some exception edges ... Tom and I had talked about them long back and I haven't kept track whether those got added or not. |
|
As for your patch, we don't want to force all vars that are defined by an exception-throwing instruction to be undefined. That is unnecessarily conservative .. Ex: "a = 1; .... a = some_call(...) ... " No need to treat 'a' as undefined. |
|
Ahh good point about variables that were already initialized...that's probably why this broke, it assigned nil to something already holding a program value. Here's the Ruby code: def foo
saved = bar()
ensure
bar(saved)
end
def bar(saved = nil); 1; end |
|
I've taken a couple stabs at it and haven't come up with a patch. I thought guarding both of the |
|
Here's what I've got...it still doesn't work even though I stepped through the DefinedVariablesProblem and saw it add the correct variable to the undefined list... diff --git a/core/src/main/java/org/jruby/ir/dataflow/analyses/DefinedVariableNode.java b/core/src/main/java/org/jruby/ir/dataflow/analyses/DefinedVariableNode.java
index 2098b34..819545c2 100644
--- a/core/src/main/java/org/jruby/ir/dataflow/analyses/DefinedVariableNode.java
+++ b/core/src/main/java/org/jruby/ir/dataflow/analyses/DefinedVariableNode.java
@@ -66,7 +66,7 @@ public class DefinedVariableNode extends FlowGraphNode<DefinedVariablesProblem,
@Override
public void applyTransferFunction(Instr i) {
// v is defined
- if (i instanceof ResultInstr) {
+ if (i instanceof ResultInstr && !i.getOperation().canRaiseException()) {
tmp.set(problem.getDFVar(((ResultInstr) i).getResult()));
}
@@ -105,7 +105,7 @@ public class DefinedVariableNode extends FlowGraphNode<DefinedVariablesProblem,
}
// v is defined
- if (i instanceof ResultInstr) {
+ if (i instanceof ResultInstr && !i.getOperation().canRaiseException()) {
tmp.set(problem.getDFVar(((ResultInstr) i).getResult()));
}
} |
|
I am going to take a look at it now ... the fix is mostly to fix how state is initialized for blocks that are rescue handlers. They need to receive state from the start of all BBs (rather than end of all BBs) that can fall into the rescue block. There are multiple ways of doing that. Either fixing the DataflowProblem generic solution to fix the MEET operation, do it on a case-by-case basis for each problem, or by adding appropriate dummy CFG edges to get the same effect. |
|
I can explain how this all set up in person if you want to meet up sometime. /cc @enebo |
|
It would be good to meet up this week. I'm going out of town for a week and will have time to hack around while on vacation. |
Environment
JRuby master as of today.
Expected Behavior
@subbuss added a pass to only initialize variables (to nil) that would not otherwise be initialized before they need to be read.
Actual Behavior
The pass in question does not appear to be handling some cases correctly. The following fails to compile in JIT because there are uninitialized variables:
I believe this fails to verify because the JVM knows that calls can raise exceptions, and with an ensure block it might branch to code that reads the
savedvalue before it is written. Unfortunately, the following patch seems to break something (though it does seem to help):When I apply this patch and try to run with -X+C (force compile on load) I get the following output:
@subbuss Is my patch wrong?
The text was updated successfully, but these errors were encountered: