8043235: Type-based optimizations interfere with continuation methods

Reviewed-by: jlaskey, lagergren

nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java

@@ -2383,6 +2383,14 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
             return false;
         }
 
+        if(isDeoptimizedExpression(lhs)) {
+            // This is actually related to "lhs.getType().isPrimitive()" above: any expression being deoptimized in
+            // the current chain of rest-of compilations used to have a type narrower than Object (so it was primitive).
+            // We must not perform undefined check specialization for them, as then we'd violate the basic rule of
+            // "Thou shalt not alter the stack shape between a deoptimized method and any of its (transitive) rest-ofs."
+            return false;
+        }
+
         final CompilationEnvironment  env = compiler.getCompilationEnvironment();
         // TODO: why?
         if (env.isCompileRestOf()) {
@@ -2450,6 +2458,19 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
             return false;
         }
 
+        if(isDeoptimizedExpression(lhs)) {
+            // This is actually related to "!lhs.getType().isObject()" above: any expression being deoptimized in
+            // the current chain of rest-of compilations used to have a type narrower than Object. We must not
+            // perform null check specialization for them, as then we'd no longer be loading aconst_null on stack
+            // and thus violate the basic rule of "Thou shalt not alter the stack shape between a deoptimized
+            // method and any of its (transitive) rest-ofs."
+            // NOTE also that if we had a representation for well-known constants (e.g. null, 0, 1, -1, etc.) in
+            // Label$Stack.localLoads then this wouldn't be an issue, as we would never (somewhat ridiculously)
+            // allocate a temporary local to hold the result of aconst_null before attempting an optimistic
+            // operation.
+            return false;
+        }
+
         // this is a null literal check, so if there is implicit coercion
         // involved like {D}x=null, we will fail - this is very rare
         final Label trueLabel  = new Label("trueLabel");
@@ -2505,6 +2526,39 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
         return true;
     }
 
+    /**
+     * Was this expression or any of its subexpressions deoptimized in the current recompilation chain of rest-of methods?
+     * @param rootExpr the expression being tested
+     * @return true if the expression or any of its subexpressions was deoptimized in the current recompilation chain.
+     */
+    private boolean isDeoptimizedExpression(final Expression rootExpr) {
+        final CompilationEnvironment env = compiler.getCompilationEnvironment();
+        if(!env.isCompileRestOf()) {
+            return false;
+        }
+        return new Supplier<Boolean>() {
+            boolean contains;
+            @Override
+            public Boolean get() {
+                rootExpr.accept(new NodeVisitor<LexicalContext>(new LexicalContext()) {
+                    @Override
+                    public boolean enterFunctionNode(FunctionNode functionNode) {
+                        return false;
+                    }
+                    @Override
+                    public boolean enterDefault(final Node node) {
+                        if(!contains && node instanceof Optimistic) {
+                            final int pp = ((Optimistic)node).getProgramPoint();
+                            contains = isValid(pp) && env.isContinuationEntryPoint(pp);
+                        }
+                        return !contains;
+                    }
+                });
+                return contains;
+            }
+        }.get();
+    }
+
     private void loadRuntimeNode(final RuntimeNode runtimeNode) {
         final List<Expression> args = new ArrayList<>(runtimeNode.getArgs());
         if (nullCheck(runtimeNode, args)) {

nashorn/test/script/basic/JDK-8043235.js

@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8043235: Type-based optimizations interfere with continuation methods
+ *
+ * @test
+ * @run
+ */
+
+function g() { 
+  return "Hello World!"
+}
+ 
+function f1() {
+    var c;
+    var paused = false;
+    // If we didn't disable nullCheck specialization for expressions
+    // containing a deoptimization site, we'd get an AssertionError
+    while (!paused && (null !== (c = g()))) {
+      print(c);
+      paused = true;
+    }
+}
+
+function f2() {
+    var c;
+    var paused = false;
+    // If we didn't disable undefinedCheck specialization for expressions
+    // containing a deoptimization site, we'd get an AssertionError
+    while (!paused && (undefined !== (c = g()))) {
+      print(c);
+      paused = true;
+    }
+}
+
+f1();
+f2();

nashorn/test/script/basic/JDK-8043235.js.EXPECTED

@@ -0,0 +1,2 @@
+Hello World!
+Hello World!