8159611: C2: ArrayCopy elimination skips required parameter checks
authorsimonis
Thu, 06 Oct 2016 18:51:24 +0200
changeset 42086 feac795f345d
parent 42085 b23a10ed6a60
child 42087 afd6ae4fec81
8159611: C2: ArrayCopy elimination skips required parameter checks Reviewed-by: kvn, zmajo, thartmann
hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
hotspot/src/share/vm/opto/arraycopynode.cpp
hotspot/src/share/vm/opto/arraycopynode.hpp
hotspot/src/share/vm/opto/library_call.cpp
hotspot/src/share/vm/opto/macroArrayCopy.cpp
hotspot/test/compiler/escapeAnalysis/TestArrayCopy.java
--- a/hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp	Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp	Thu Oct 06 18:51:24 2016 +0200
@@ -3187,7 +3187,6 @@
   if (flags & LIR_OpArrayCopy::length_positive_check) {
     __ testl(length, length);
     __ jcc(Assembler::less, *stub->entry());
-    __ jcc(Assembler::zero, *stub->continuation());
   }
 
 #ifdef _LP64
--- a/hotspot/src/share/vm/opto/arraycopynode.cpp	Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/arraycopynode.cpp	Thu Oct 06 18:51:24 2016 +0200
@@ -26,9 +26,10 @@
 #include "opto/arraycopynode.hpp"
 #include "opto/graphKit.hpp"
 
-ArrayCopyNode::ArrayCopyNode(Compile* C, bool alloc_tightly_coupled)
+ArrayCopyNode::ArrayCopyNode(Compile* C, bool alloc_tightly_coupled, bool has_negative_length_guard)
   : CallNode(arraycopy_type(), NULL, TypeRawPtr::BOTTOM),
     _alloc_tightly_coupled(alloc_tightly_coupled),
+    _has_negative_length_guard(has_negative_length_guard),
     _kind(None),
     _arguments_validated(false),
     _src_type(TypeOopPtr::BOTTOM),
@@ -45,10 +46,11 @@
                                    Node* dest, Node* dest_offset,
                                    Node* length,
                                    bool alloc_tightly_coupled,
+                                   bool has_negative_length_guard,
                                    Node* src_klass, Node* dest_klass,
                                    Node* src_length, Node* dest_length) {
 
-  ArrayCopyNode* ac = new ArrayCopyNode(kit->C, alloc_tightly_coupled);
+  ArrayCopyNode* ac = new ArrayCopyNode(kit->C, alloc_tightly_coupled, has_negative_length_guard);
   Node* prev_mem = kit->set_predefined_input_for_runtime_call(ac);
 
   ac->init_req(ArrayCopyNode::Src, src);
--- a/hotspot/src/share/vm/opto/arraycopynode.hpp	Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/arraycopynode.hpp	Thu Oct 06 18:51:24 2016 +0200
@@ -58,6 +58,7 @@
   // the arraycopy is not parsed yet so doesn't exist when
   // LibraryCallKit::tightly_coupled_allocation() is called.
   bool _alloc_tightly_coupled;
+  bool _has_negative_length_guard;
 
   bool _arguments_validated;
 
@@ -82,7 +83,7 @@
     return TypeFunc::make(domain, range);
   }
 
-  ArrayCopyNode(Compile* C, bool alloc_tightly_coupled);
+  ArrayCopyNode(Compile* C, bool alloc_tightly_coupled, bool has_negative_length_guard);
 
   intptr_t get_length_if_constant(PhaseGVN *phase) const;
   int get_count(PhaseGVN *phase) const;
@@ -133,6 +134,7 @@
                              Node* dest,  Node* dest_offset,
                              Node* length,
                              bool alloc_tightly_coupled,
+                             bool has_negative_length_guard,
                              Node* src_klass = NULL, Node* dest_klass = NULL,
                              Node* src_length = NULL, Node* dest_length = NULL);
 
@@ -162,6 +164,8 @@
 
   bool is_alloc_tightly_coupled() const { return _alloc_tightly_coupled; }
 
+  bool has_negative_length_guard() const { return _has_negative_length_guard; }
+
   static bool may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase, ArrayCopyNode*& ac);
   bool modifies(intptr_t offset_lo, intptr_t offset_hi, PhaseTransform* phase, bool must_modify);
 
--- a/hotspot/src/share/vm/opto/library_call.cpp	Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/library_call.cpp	Thu Oct 06 18:51:24 2016 +0200
@@ -4046,7 +4046,7 @@
       if (!stopped()) {
         newcopy = new_array(klass_node, length, 0);  // no arguments to push
 
-        ArrayCopyNode* ac = ArrayCopyNode::make(this, true, original, start, newcopy, intcon(0), moved, true,
+        ArrayCopyNode* ac = ArrayCopyNode::make(this, true, original, start, newcopy, intcon(0), moved, true, false,
                                                 load_object_klass(original), klass_node);
         if (!is_copyOfRange) {
           ac->set_copyof(validated);
@@ -4566,7 +4566,7 @@
 
   const TypePtr* raw_adr_type = TypeRawPtr::BOTTOM;
 
-  ArrayCopyNode* ac = ArrayCopyNode::make(this, false, src, NULL, dest, NULL, countx, false);
+  ArrayCopyNode* ac = ArrayCopyNode::make(this, false, src, NULL, dest, NULL, countx, false, false);
   ac->set_clonebasic();
   Node* n = _gvn.transform(ac);
   if (n == ac) {
@@ -4703,7 +4703,7 @@
           set_control(is_obja);
           // Generate a direct call to the right arraycopy function(s).
           Node* alloc = tightly_coupled_allocation(alloc_obj, NULL);
-          ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, alloc != NULL);
+          ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, alloc != NULL, false);
           ac->set_cloneoop();
           Node* n = _gvn.transform(ac);
           assert(n == ac, "cannot disappear");
@@ -5100,6 +5100,8 @@
     trap_bci = alloc->jvms()->bci();
   }
 
+  bool negative_length_guard_generated = false;
+
   if (!C->too_many_traps(trap_method, trap_bci, Deoptimization::Reason_intrinsic) &&
       can_emit_guards &&
       !src->is_top() && !dest->is_top()) {
@@ -5132,6 +5134,15 @@
                          load_array_length(dest),
                          slow_region);
 
+    // (6) length must not be negative.
+    // This is also checked in generate_arraycopy() during macro expansion, but
+    // we also have to check it here for the case where the ArrayCopyNode will
+    // be eliminated by Escape Analysis.
+    if (EliminateAllocations) {
+      generate_negative_guard(length, slow_region);
+      negative_length_guard_generated = true;
+    }
+
     // (9) each element of an oop array must be assignable
     Node* src_klass  = load_object_klass(src);
     Node* dest_klass = load_object_klass(dest);
@@ -5159,7 +5170,7 @@
     return true;
   }
 
-  ArrayCopyNode* ac = ArrayCopyNode::make(this, true, src, src_offset, dest, dest_offset, length, alloc != NULL,
+  ArrayCopyNode* ac = ArrayCopyNode::make(this, true, src, src_offset, dest, dest_offset, length, alloc != NULL, negative_length_guard_generated,
                                           // Create LoadRange and LoadKlass nodes for use during macro expansion here
                                           // so the compiler has a chance to eliminate them: during macro expansion,
                                           // we have to set their control (CastPP nodes are eliminated).
--- a/hotspot/src/share/vm/opto/macroArrayCopy.cpp	Tue Nov 01 09:19:14 2016 +0100
+++ b/hotspot/src/share/vm/opto/macroArrayCopy.cpp	Thu Oct 06 18:51:24 2016 +0200
@@ -1154,7 +1154,10 @@
     // Call StubRoutines::generic_arraycopy stub.
     Node* mem = generate_arraycopy(ac, NULL, &ctrl, merge_mem, &io,
                                    TypeRawPtr::BOTTOM, T_CONFLICT,
-                                   src, src_offset, dest, dest_offset, length);
+                                   src, src_offset, dest, dest_offset, length,
+                                   // If a  negative length guard was generated for the ArrayCopyNode,
+                                   // the length of the array can never be negative.
+                                   false, ac->has_negative_length_guard());
 
     // Do not let reads from the destination float above the arraycopy.
     // Since we cannot type the arrays, we don't know which slices
@@ -1258,5 +1261,7 @@
   generate_arraycopy(ac, alloc, &ctrl, merge_mem, &io,
                      adr_type, dest_elem,
                      src, src_offset, dest, dest_offset, length,
-                     false, false, slow_region);
+                     // If a  negative length guard was generated for the ArrayCopyNode,
+                     // the length of the array can never be negative.
+                     false, ac->has_negative_length_guard(), slow_region);
 }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/escapeAnalysis/TestArrayCopy.java	Thu Oct 06 18:51:24 2016 +0200
@@ -0,0 +1,115 @@
+/*
+ * Copyright (c) 2016, SAP SE 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.
+ */
+
+/*
+ * @test
+ * @bug 8159611
+ * @summary The elimination of System.arraycopy by EscapeAnalysis prevents
+ *          an IndexOutOfBoundsException from being thrown if the arraycopy
+ *          is called with a negative length argument.
+ * @modules java.base/jdk.internal.misc
+ * @library /testlibrary /test/lib
+ * @build sun.hotspot.WhiteBox
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ *                                sun.hotspot.WhiteBox$WhiteBoxPermission
+ *
+ * @run main/othervm
+ *        -Xbootclasspath/a:.
+ *        -XX:+UnlockDiagnosticVMOptions
+ *        -XX:+WhiteBoxAPI
+ *        -XX:-UseOnStackReplacement
+ *        compiler.escapeAnalysis.TestArrayCopy
+ *
+ * @author Volker Simonis
+ */
+
+package compiler.escapeAnalysis;
+
+import sun.hotspot.WhiteBox;
+import java.lang.reflect.Method;
+
+public class TestArrayCopy {
+
+    private static final WhiteBox WB = WhiteBox.getWhiteBox();
+    // DST_LEN Must be const, otherwise EliminateAllocations won't work
+    static final int DST_LEN = 4;
+    static final int SRC_LEN = 8;
+
+    public static boolean do_test1(Object src, int src_pos, int dst_pos, int cpy_len) {
+        try {
+            System.arraycopy(src, src_pos, new Object[DST_LEN], dst_pos, cpy_len);
+            return false;
+        } catch (IndexOutOfBoundsException e) {
+            return true;
+        }
+    }
+
+    public static int do_test2(Object src, int src_pos, int dst_pos, int cpy_len) {
+        try {
+            System.arraycopy(src, src_pos, new Object[DST_LEN], dst_pos, cpy_len);
+            return 0;
+        } catch (IndexOutOfBoundsException e) {
+            return 1;
+        } catch (ArrayStoreException e) {
+            return 2;
+        }
+    }
+
+    static final int COUNT = 100_000;
+    static final int[] src_pos = { 0, -1, -1,  0,  0,  0,  1,  1,  1,  1, 1 };
+    static final int[] dst_pos = { 0, -1,  0, -1,  0,  1,  0,  1,  1,  1, 1 };
+    static final int[] cpy_len = { 0,  0,  0,  0, -1, -1, -1, -1,  8,  4, 2 };
+
+    public static void main(String args[]) throws Exception {
+        int length = args.length > 0 ? Integer.parseInt(args[0]) : -1;
+        int[] int_arr = new int[SRC_LEN];
+        Object[] obj_arr = new Object[SRC_LEN];
+
+        Method test1 = TestArrayCopy.class.getMethod("do_test1", Object.class, int.class, int.class, int.class);
+        Method test2 = TestArrayCopy.class.getMethod("do_test2", Object.class, int.class, int.class, int.class);
+
+        for (int i = 0; i < src_pos.length; i++) {
+            int sp = src_pos[i];
+            int dp = dst_pos[i];
+            int cl = cpy_len[i];
+            String version1 = String.format("System.arraycopy(Object[8], %d, new Object[%d], %d, %d)", sp, DST_LEN, dp, cl);
+            String version2 = String.format("System.arraycopy(int[8], %d, new Object[%d], %d, %d)", sp, DST_LEN, dp, cl);
+            System.out.format("Testing " + version1 + "\nand " + version2).flush();
+            for (int x = 0; x < COUNT; x++) {
+                if (!do_test1(obj_arr, sp, dp, cl) &&
+                    (sp < 0 || dp < 0 || cl < 0 || (sp + cl >= SRC_LEN) || (dp + cl >= DST_LEN))) {
+                    throw new RuntimeException("Expected IndexOutOfBoundsException for " + version1);
+                }
+                int res = do_test2(int_arr, sp, dp, cl);
+                if (res == 0 || res == 1) {
+                    throw new RuntimeException("Expected ArrayStoreException for " + version2);
+                }
+            }
+            WB.deoptimizeMethod(test1);
+            WB.clearMethodState(test1);
+            WB.deoptimizeMethod(test2);
+            WB.clearMethodState(test2);
+        }
+
+    }
+}