8362117: C2: compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java fails with a wrong result due to invalidated liveness assumptions for data phis

src/hotspot/share/opto/stringopts.cpp

@@ -919,6 +919,11 @@ bool StringConcat::validate_control_flow() {
 
   int null_check_count = 0;
   Unique_Node_List ctrl_path;
+  VectorSet argument_set;
+
+  for (int i = 0; i < num_arguments(); i++) {
+    argument_set.set(argument(i)->_idx);
+  }
 
   assert(_control.contains(_begin), "missing");
   assert(_control.contains(_end), "missing");
@@ -1005,7 +1010,7 @@ bool StringConcat::validate_control_flow() {
           if (_multiple &&
               ((v1->is_Proj() && is_SB_toString(v1->in(0)) && ctrl_path.member(v1->in(0))) ||
                (v2->is_Proj() && is_SB_toString(v2->in(0)) && ctrl_path.member(v2->in(0))))) {
-            // iftrue -> if -> bool -> cmpp -> resproj -> tostring
+            // iftrue <- if <- bool <- cmpp <- resproj <- tostring
             fail = true;
             break;
           }
@@ -1062,6 +1067,32 @@ bool StringConcat::validate_control_flow() {
         // XXX should check for possibly merging stores.  simple data merges are ok.
         // The IGVN will make this simple diamond go away when it
         // transforms the Region. Make sure it sees it.
+
+        // First exclude the following pattern:
+        // append <- Phi <- Region <- (True, False) <- If <- Bool <- CmpP <- Proj (Result) <- toString;
+        // in order to prevent an unsafe transformation in eliminate_unneeded_control,
+        // where the Bool would be replaced by a constant zero but the Phi stays live
+        // as it is a parameter of the concatenation itself.
+        Node* iff = ptr->in(1)->in(0);
+        Node* bol = iff->in(1);
+        Node* cmp = bol->in(1);
+        assert(cmp->is_Cmp(), "unexpected if shape");
+        Node* v1 = cmp->in(1);
+        Node* v2 = cmp->in(2);
+        if (_multiple &&
+            ((v1->is_Proj() && is_SB_toString(v1->in(0)) && ctrl_path.member(v1->in(0))) ||
+             (v2->is_Proj() && is_SB_toString(v2->in(0)) && ctrl_path.member(v2->in(0))))) {
+          for (SimpleDUIterator i(ptr); i.has_next(); i.next()) {
+            Node* use = i.get();
+            if (use->is_Phi() && argument_set.test(use->_idx)) {
+              fail = true;
+              break;
+            }
+          }
+          if (fail) {
+            break;
+          }
+        }
         Compile::current()->record_for_igvn(ptr);
         _control.push(ptr);
         ptr = ptr->in(1)->in(0)->in(0);

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsPhiUseOfDiamondRegion.java

@@ -0,0 +1,57 @@
+/*
+ * 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.
+ */
+
+/*
+ * @test
+ * @bug 8362117
+ * @summary Test stacked string concatenations where the toString result
+ *          of the first StringBuilder chain is used as a test for a
+ *          simple diamond in the second StringBuilder. If the region of
+ *          the simple diamond has a Phi that is used as a parameter in the
+ *          concatenation, a wrong result should not be produced.
+ * @library /test/lib /
+ * @run main/othervm compiler.stringopts.TestStackedConcatsPhiUseOfDiamondRegion
+ * @run main/othervm -XX:-TieredCompilation -Xcomp
+ *                   -XX:CompileOnly=compiler.stringopts.TestStackedConcatsPhiUseOfDiamondRegion::f
+ *                   compiler.stringopts.TestStackedConcatsPhiUseOfDiamondRegion
+ */
+
+package compiler.stringopts;
+
+import jdk.test.lib.Asserts;
+
+public class TestStackedConcatsPhiUseOfDiamondRegion {
+
+    public static void main (String... args) {
+        new StringBuilder(); // load the class
+        f();
+    }
+
+    static String f() {
+        String s = "a";
+        s = new StringBuilder().append(s).append(s).toString();
+        s = new StringBuilder().append(s).append((s == "xx") ? s : "aa").toString();
+        Asserts.assertEQ(s, "aaaa"); // in particular, we should not have s.equals("aaxx").
+        return s;
+    }
+}