8370405: C2: mismatched store from MergeStores wrongly scalarized in allocation elimination

src/hotspot/share/opto/macro.cpp

@@ -606,6 +606,11 @@ bool PhaseMacroExpand::can_eliminate_allocation(PhaseIterGVN* igvn, AllocateNode
         for (DUIterator_Fast kmax, k = use->fast_outs(kmax);
                                    k < kmax && can_eliminate; k++) {
           Node* n = use->fast_out(k);
+          if (n->is_Mem() && n->as_Mem()->is_mismatched_access()) {
+            DEBUG_ONLY(disq_node = n);
+            NOT_PRODUCT(fail_eliminate = "Mismatched access");
+            can_eliminate = false;
+          }
           if (!n->is_Store() && n->Opcode() != Op_CastP2X && !bs->is_gc_pre_barrier_node(n) && !reduce_merge_precheck) {
             DEBUG_ONLY(disq_node = n;)
             if (n->is_Load() || n->is_LoadStore()) {
@@ -743,6 +748,41 @@ void PhaseMacroExpand::undo_previous_scalarizations(GrowableArray <SafePointNode
   }
 }
 
+#ifdef ASSERT
+  // Verify if a value can be written into a field.
+  void verify_type_compatability(const Type* value_type, const Type* field_type) {
+    BasicType value_bt = value_type->basic_type();
+    BasicType field_bt = field_type->basic_type();
+
+    // Primitive types must match.
+    if (is_java_primitive(value_bt) && value_bt == field_bt) { return; }
+
+    // I have been struggling to make a similar assert for non-primitive
+    // types. I we can add one in the future. For now, I just let them
+    // pass without checks.
+    // In particular, I was struggling with a value that came from a call,
+    // and had only a non-null check CastPP. There was also a checkcast
+    // in the graph to verify the interface, but the corresponding
+    // CheckCastPP result was not updated in the stack slot, and so
+    // we ended up using the CastPP. That means that the field knows
+    // that it should get an oop from an interface, but the value lost
+    // that information, and so it is not a subtype.
+    // There may be other issues, feel free to investigate further!
+    if (!is_java_primitive(value_bt)) { return; }
+
+    tty->print_cr("value not compatible for field: %s vs %s",
+                  type2name(value_bt),
+                  type2name(field_bt));
+    tty->print("value_type: ");
+    value_type->dump();
+    tty->cr();
+    tty->print("field_type: ");
+    field_type->dump();
+    tty->cr();
+    assert(false, "value_type does not fit field_type");
+  }
+#endif
+
 SafePointScalarObjectNode* PhaseMacroExpand::create_scalarized_object_description(AllocateNode *alloc, SafePointNode* sfpt) {
   // Fields of scalar objs are referenced only at the end
   // of regular debuginfo at the last (youngest) JVMS.
@@ -859,6 +899,7 @@ SafePointScalarObjectNode* PhaseMacroExpand::create_scalarized_object_descriptio
         field_val = transform_later(new DecodeNNode(field_val, field_val->get_ptr_type()));
       }
     }
+    DEBUG_ONLY(verify_type_compatability(field_val->bottom_type(), field_type);)
     sfpt->add_req(field_val);
   }
 

src/hotspot/share/runtime/deoptimization.cpp

@@ -1382,6 +1382,9 @@ void Deoptimization::reassign_type_array_elements(frame* fr, RegisterMap* reg_ma
 
     case T_INT: case T_FLOAT: { // 4 bytes.
       assert(value->type() == T_INT, "Agreement.");
+#if INCLUDE_JVMCI
+      // big_value allows encoding double/long value as e.g. [int = 0, long], and storing
+      // the value in two array elements.
       bool big_value = false;
       if (i + 1 < sv->field_size() && type == T_INT) {
         if (sv->field_at(i)->is_location()) {
@@ -1409,6 +1412,9 @@ void Deoptimization::reassign_type_array_elements(frame* fr, RegisterMap* reg_ma
       } else {
         obj->int_at_put(index, value->get_jint());
       }
+#else // not INCLUDE_JVMCI
+      obj->int_at_put(index, value->get_jint());
+#endif // INCLUDE_JVMCI
       break;
     }
 

test/hotspot/jtreg/compiler/c2/TestMergeStoresAndAllocationElimination.java

@@ -0,0 +1,123 @@
+/*
+ * Copyright (c) 2025, 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.
+ */
+
+package compiler.c2;
+
+/*
+ * @test
+ * @bug 8370405
+ * @summary Test case where we had escape analysis tell us that we can possibly eliminate
+ *          the array allocation, then MergeStores introduces a mismatched store, which
+ *          the actual elimination does not verify for. That led to wrong results.
+ * @run main/othervm -XX:CompileCommand=compileonly,compiler.c2.TestMergeStoresAndAllocationElimination::test
+ *                   -XX:CompileCommand=exclude,compiler.c2.TestMergeStoresAndAllocationElimination::dontinline
+ *                   -XX:-TieredCompilation -Xbatch
+ *                   -XX:+IgnoreUnrecognizedVMOptions -XX:-CICompileOSR
+ *                   compiler.c2.TestMergeStoresAndAllocationElimination
+ * @run main compiler.c2.TestMergeStoresAndAllocationElimination
+ */
+
+public class TestMergeStoresAndAllocationElimination {
+    static void dontinline() {}
+
+    static int test(boolean flag) {
+        int[] arr = new int[4];
+        // The values below will be caputured as "raw stores" in the Initialize
+        // of the array allocation above.
+        // These stores are for cosmetics only, we set the "1" bits so that it is
+        // simple to track where values are coming from.
+        arr[0] = 0x0001_0000;
+        arr[1] = 0x0010_0000;
+        arr[2] = 0x0000_0100;
+        arr[3] = 0x0100_0000;
+        // So far, the result should be:
+        // 0x421_0300
+
+        // The call below prevents further assignments from being captured into
+        // the Initialize above.
+        dontinline();
+        // The follwoing stores are eventually optimized by MergeStores, and create
+        // a mismatched StoreL.
+        arr[0] = 0x0000_0001;
+        arr[1] = 0x0000_0010;
+        // Now, the result should be:
+        // 0x400_0321
+
+        // We create an uncommon trap because of an "unstable if".
+        // If Escape Analysis were to work, it would try to capture the values
+        // from the StoreL above. But because it is mismatched, it should fail.
+        // What happened before that verification: we would take the ConL, and
+        // insert it in a list of ConI. That meant that we eventually applied
+        // that value wrong if the deopt was taken (flag = true).
+        //
+        // What happened when the deopt got the wrong values: It got these values:
+        // [0]=68719476737 = 0x10_0000_0001 -> long value, not correct
+        // [1]=1048576     =      0x10_0000 -> this entry is not updated!
+        // [2]=256         =          0x100
+        // [3]=16777216    =     0x100_0000
+        //
+        // This is serialized as a long and 3 ints, and that looks like 5 ints.
+        // This creates an array of 5 elements (and not 4):
+        // [0]             =            0x1
+        // [1]             =           0x10
+        // [2]             =      0x10_0000 -> this entry is "inserted"
+        // [3]             =          0x100
+        // [4]             =     0x100_0000
+        //
+        // This creates the wrong state:
+        // 0x30_0421
+        // And we can actually read that the arr.length is 5, below.
+        if (flag) { System.out.println("unstable if: " + arr.length); }
+
+        // Delay the allocation elimination until after loop opts, so that it
+        // happens after MergeStores. Without this, we would immediately
+        // eliminate the allocation during Escape Analysis, and then MergeStores
+        // would not find the stores that would be removed with the allocation.
+        for (int i = 0; i < 10_000; i++) {
+            arr[3] = 0x0000_1000;
+        }
+        // Coming from the correct value, we should have transition of state:
+        // 0x400_0321 -> 0x4321
+        // But coming from the bad (rematerialized) state, we transition:
+        // 0x30_0421 -> 0x30_4021
+
+        // Tag each entry with an index number
+        // We expect: 0x4321
+        return 1 * arr[0] + 2 * arr[1] + 3 * arr[2] + 4 * arr[3];
+    }
+
+    public static void main(String[] args) {
+        // Capture interpreter result.
+        int gold = test(false);
+        // Repeat until we get compilation.
+        for (int i = 0; i < 10_000; i++) {
+            test(false);
+        }
+        // Capture compiled results.
+        int res0 = test(false);
+        int res1 = test(true);
+        if (res0 != gold || res1 != gold) {
+            throw new RuntimeException("Unexpected result: " + Integer.toHexString(res0) + " and " + Integer.toHexString(res1) + ", should be: " + Integer.toHexString(gold));
+        }
+    }
+}