8144212: JDK 9 b93 breaks Apache Lucene due to compact strings
Summary: String compress/inflate intrinsics need to capture char and byte memory.
Reviewed-by: aph, roland, kvn
--- a/hotspot/src/share/vm/opto/graphKit.cpp Mon Jan 18 08:25:26 2016 +0100
+++ b/hotspot/src/share/vm/opto/graphKit.cpp Mon Jan 18 08:34:14 2016 +0100
@@ -4340,20 +4340,51 @@
value, T_BYTE, coder_field_idx, MemNode::unordered);
}
-Node* GraphKit::compress_string(Node* src, Node* dst, Node* count) {
+// Capture src and dst memory state with a MergeMemNode
+Node* GraphKit::capture_memory(const TypePtr* src_type, const TypePtr* dst_type) {
+ if (src_type == dst_type) {
+ // Types are equal, we don't need a MergeMemNode
+ return memory(src_type);
+ }
+ MergeMemNode* merge = MergeMemNode::make(map()->memory());
+ record_for_igvn(merge); // fold it up later, if possible
+ int src_idx = C->get_alias_index(src_type);
+ int dst_idx = C->get_alias_index(dst_type);
+ merge->set_memory_at(src_idx, memory(src_idx));
+ merge->set_memory_at(dst_idx, memory(dst_idx));
+ return merge;
+}
+
+Node* GraphKit::compress_string(Node* src, const TypeAryPtr* src_type, Node* dst, Node* count) {
assert(Matcher::match_rule_supported(Op_StrCompressedCopy), "Intrinsic not supported");
- uint idx = C->get_alias_index(TypeAryPtr::BYTES);
- StrCompressedCopyNode* str = new StrCompressedCopyNode(control(), memory(idx), src, dst, count);
+ assert(src_type == TypeAryPtr::BYTES || src_type == TypeAryPtr::CHARS, "invalid source type");
+ // If input and output memory types differ, capture both states to preserve
+ // the dependency between preceding and subsequent loads/stores.
+ // For example, the following program:
+ // StoreB
+ // compress_string
+ // LoadB
+ // has this memory graph (use->def):
+ // LoadB -> compress_string -> CharMem
+ // ... -> StoreB -> ByteMem
+ // The intrinsic hides the dependency between LoadB and StoreB, causing
+ // the load to read from memory not containing the result of the StoreB.
+ // The correct memory graph should look like this:
+ // LoadB -> compress_string -> MergeMem(CharMem, StoreB(ByteMem))
+ Node* mem = capture_memory(src_type, TypeAryPtr::BYTES);
+ StrCompressedCopyNode* str = new StrCompressedCopyNode(control(), mem, src, dst, count);
Node* res_mem = _gvn.transform(new SCMemProjNode(str));
- set_memory(res_mem, idx);
+ set_memory(res_mem, TypeAryPtr::BYTES);
return str;
}
-void GraphKit::inflate_string(Node* src, Node* dst, Node* count) {
+void GraphKit::inflate_string(Node* src, Node* dst, const TypeAryPtr* dst_type, Node* count) {
assert(Matcher::match_rule_supported(Op_StrInflatedCopy), "Intrinsic not supported");
- uint idx = C->get_alias_index(TypeAryPtr::BYTES);
- StrInflatedCopyNode* str = new StrInflatedCopyNode(control(), memory(idx), src, dst, count);
- set_memory(_gvn.transform(str), idx);
+ assert(dst_type == TypeAryPtr::BYTES || dst_type == TypeAryPtr::CHARS, "invalid dest type");
+ // Capture src and dst memory (see comment in 'compress_string').
+ Node* mem = capture_memory(TypeAryPtr::BYTES, dst_type);
+ StrInflatedCopyNode* str = new StrInflatedCopyNode(control(), mem, src, dst, count);
+ set_memory(_gvn.transform(str), dst_type);
}
void GraphKit::inflate_string_slow(Node* src, Node* dst, Node* start, Node* count) {
--- a/hotspot/src/share/vm/opto/graphKit.hpp Mon Jan 18 08:25:26 2016 +0100
+++ b/hotspot/src/share/vm/opto/graphKit.hpp Mon Jan 18 08:34:14 2016 +0100
@@ -881,8 +881,9 @@
Node* load_String_coder(Node* ctrl, Node* str);
void store_String_value(Node* ctrl, Node* str, Node* value);
void store_String_coder(Node* ctrl, Node* str, Node* value);
- Node* compress_string(Node* src, Node* dst, Node* count);
- void inflate_string(Node* src, Node* dst, Node* count);
+ Node* capture_memory(const TypePtr* src_type, const TypePtr* dst_type);
+ Node* compress_string(Node* src, const TypeAryPtr* src_type, Node* dst, Node* count);
+ void inflate_string(Node* src, Node* dst, const TypeAryPtr* dst_type, Node* count);
void inflate_string_slow(Node* src, Node* dst, Node* start, Node* count);
// Handy for making control flow
--- a/hotspot/src/share/vm/opto/library_call.cpp Mon Jan 18 08:25:26 2016 +0100
+++ b/hotspot/src/share/vm/opto/library_call.cpp Mon Jan 18 08:34:14 2016 +0100
@@ -1359,9 +1359,9 @@
// 'dst_start' points to dst array + scaled offset
Node* count = NULL;
if (compress) {
- count = compress_string(src_start, dst_start, length);
+ count = compress_string(src_start, TypeAryPtr::get_array_body_type(src_elem), dst_start, length);
} else {
- inflate_string(src_start, dst_start, length);
+ inflate_string(src_start, dst_start, TypeAryPtr::get_array_body_type(dst_elem), length);
}
if (alloc != NULL) {
@@ -1587,7 +1587,7 @@
(void) store_to_memory(control(), adr, ch, T_CHAR, TypeAryPtr::BYTES, MemNode::unordered,
false, false, true /* mismatched */);
} else {
- ch = make_load(control(), adr, TypeInt::CHAR, T_CHAR, MemNode::unordered,
+ ch = make_load(control(), adr, TypeInt::CHAR, T_CHAR, TypeAryPtr::BYTES, MemNode::unordered,
LoadNode::DependsOnlyOnTest, false, false, true /* mismatched */);
set_result(ch);
}
--- a/hotspot/src/share/vm/opto/stringopts.cpp Mon Jan 18 08:25:26 2016 +0100
+++ b/hotspot/src/share/vm/opto/stringopts.cpp Mon Jan 18 08:34:14 2016 +0100
@@ -1466,7 +1466,7 @@
// Use fast intrinsic
Node* src = kit.array_element_address(src_array, kit.intcon(0), T_BYTE);
Node* dst = kit.array_element_address(dst_array, start, T_BYTE);
- kit.inflate_string(src, dst, __ value(count));
+ kit.inflate_string(src, dst, TypeAryPtr::BYTES, __ value(count));
} else {
// No intrinsic available, use slow method
kit.inflate_string_slow(src_array, dst_array, start, __ value(count));
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/intrinsics/string/TestStringIntrinsicMemoryFlow.java Mon Jan 18 08:34:14 2016 +0100
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+import jdk.test.lib.Asserts;
+
+/*
+ * @test
+ * @bug 8144212
+ * @summary Check for correct memory flow with the String compress/inflate intrinsics.
+ * @library /testlibrary
+ * @run main TestStringIntrinsicMemoryFlow
+ */
+public class TestStringIntrinsicMemoryFlow {
+
+ public static void main(String[] args) {
+ for (int i = 0; i < 100_000; ++i) {
+ String s = "MyString";
+ char[] c = {'M'};
+ char res = testInflate1(s);
+ Asserts.assertEquals(res, 'M', "testInflate1 failed");
+ res = testInflate2(s);
+ Asserts.assertEquals(res, (char)42, "testInflate2 failed");
+ res = testCompress1(c);
+ Asserts.assertEquals(res, 'M', "testCompress1 failed");
+ byte resB = testCompress2(c);
+ Asserts.assertEquals(resB, (byte)42, "testCompress2 failed");
+ }
+ }
+
+ private static char testInflate1(String s) {
+ char c[] = new char[1];
+ // Inflate String from byte[] to char[]
+ s.getChars(0, 1, c, 0);
+ // Read char[] memory written by inflate intrinsic
+ return c[0];
+ }
+
+ private static char testInflate2(String s) {
+ char c1[] = new char[1];
+ char c2[] = new char[1];
+ c2[0] = 42;
+ // Inflate String from byte[] to char[]
+ s.getChars(0, 1, c1, 0);
+ // Read char[] memory written before inflation
+ return c2[0];
+ }
+
+ private static char testCompress1(char[] c) {
+ // Compress String from char[] to byte[]
+ String s = new String(c);
+ // Read the memory written by compress intrinsic
+ return s.charAt(0);
+ }
+
+ private static byte testCompress2(char[] c) {
+ byte b1[] = new byte[1];
+ b1[0] = 42;
+ // Compress String from char[] to byte[]
+ new String(c);
+ // Read byte[] memory written before compression
+ return b1[0];
+ }
+}