# HG changeset patch # User simonis # Date 1475772684 -7200 # Node ID feac795f345d62695d075e7ce23fc6d792384cff # Parent b23a10ed6a608a5591b6d21aae2f233491a51f62 8159611: C2: ArrayCopy elimination skips required parameter checks Reviewed-by: kvn, zmajo, thartmann diff -r b23a10ed6a60 -r feac795f345d hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp --- 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 diff -r b23a10ed6a60 -r feac795f345d hotspot/src/share/vm/opto/arraycopynode.cpp --- 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); diff -r b23a10ed6a60 -r feac795f345d hotspot/src/share/vm/opto/arraycopynode.hpp --- 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); diff -r b23a10ed6a60 -r feac795f345d hotspot/src/share/vm/opto/library_call.cpp --- 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). diff -r b23a10ed6a60 -r feac795f345d hotspot/src/share/vm/opto/macroArrayCopy.cpp --- 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); } diff -r b23a10ed6a60 -r feac795f345d hotspot/test/compiler/escapeAnalysis/TestArrayCopy.java --- /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); + } + + } +}