8273914: Indy string concat changes order of operations

Reviewed-by: andrew
Backport-of: cfee451

src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java

@@ -140,25 +140,6 @@ private List<JCTree> collect(JCTree tree, List<JCTree> res) {
         return res.append(tree);
     }
 
-    /**
-     * If the type is not accessible from current context, try to figure out the
-     * sharpest accessible supertype.
-     *
-     * @param originalType type to sharpen
-     * @return sharped type
-     */
-    Type sharpestAccessible(Type originalType) {
-        if (originalType.hasTag(ARRAY)) {
-            return types.makeArrayType(sharpestAccessible(types.elemtype(originalType)));
-        }
-
-        Type type = originalType;
-        while (!rs.isAccessible(gen.getAttrEnv(), type.asElement())) {
-            type = types.supertype(type);
-        }
-        return type;
-    }
-
     /**
      * "Legacy" bytecode flavor: emit the StringBuilder.append chains for string
      * concatenation.
@@ -303,6 +284,14 @@ protected List<List<JCTree>> split(List<JCTree> args) {
 
             return splits.toList();
         }
+
+        /**
+         * Returns true if the argument should be converted to a string eagerly, to preserve
+         * possible side-effects.
+         */
+        protected boolean shouldConvertToStringEagerly(Type argType) {
+            return !types.unboxedTypeOrType(argType).isPrimitive() && argType.tsym != syms.stringType.tsym;
+        }
     }
 
     /**
@@ -331,14 +320,18 @@ protected void emit(JCDiagnostic.DiagnosticPosition pos, List<JCTree> args, bool
                 for (JCTree arg : t) {
                     Object constVal = arg.type.constValue();
                     if ("".equals(constVal)) continue;
-                    if (arg.type == syms.botType) {
-                        dynamicArgs.add(types.boxedClass(syms.voidType).type);
-                    } else {
-                        dynamicArgs.add(sharpestAccessible(arg.type));
+                    Type argType = arg.type;
+                    if (argType == syms.botType) {
+                        argType = types.boxedClass(syms.voidType).type;
                     }
                     if (!first || generateFirstArg) {
                         gen.genExpr(arg, arg.type).load();
                     }
+                    if (shouldConvertToStringEagerly(argType)) {
+                        gen.callMethod(pos, syms.stringType, names.valueOf, List.of(syms.objectType), true);
+                        argType = syms.stringType;
+                    }
+                    dynamicArgs.add(argType);
                     first = false;
                 }
                 doCall(type, pos, dynamicArgs.toList());
@@ -438,10 +431,15 @@ protected void emit(JCDiagnostic.DiagnosticPosition pos, List<JCTree> args, bool
                     } else {
                         // Ordinary arguments come through the dynamic arguments.
                         recipe.append(TAG_ARG);
-                        dynamicArgs.add(sharpestAccessible(arg.type));
+                        Type argType = arg.type;
                         if (!first || generateFirstArg) {
                             gen.genExpr(arg, arg.type).load();
                         }
+                        if (shouldConvertToStringEagerly(argType)) {
+                            gen.callMethod(pos, syms.stringType, names.valueOf, List.of(syms.objectType), true);
+                            argType = syms.stringType;
+                        }
+                        dynamicArgs.add(argType);
                         first = false;
                     }
                 }

test/hotspot/jtreg/runtime/modules/AccessCheck/MethodAccessReadTwice.java

@@ -29,8 +29,8 @@
  *          after the module read edge is added.
  * @compile ModuleLibrary.java
  *          p2/c2.java
- *          p5/c5.java
- *          p7/c7.java
+ *          p5/c5.jasm
+ *          p7/c7.jasm
  * @run main/othervm MethodAccessReadTwice
  */
 

test/hotspot/jtreg/runtime/modules/AccessCheck/p5/c5.jasm

@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2021, Google LLC. 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 input for the fix for JDK-8174954, which checks for an expected
+ * IllegalAccessError when the parameter type of an invokedynamic is
+ * inaccessible.
+ *
+ * The test assumes that given the string concatenation expression "" + param,
+ * javac generates an invokedynamic that uses the specific type of param. The
+ * fix for JDK-8273914 make javac eagerly convert param to a String before
+ * passing it to the invokedynamic call, which avoids the accessibility issue
+ * the test is trying to exercise.
+ *
+ * This jasm file contains the bytecode javac generated before the fix for
+ * JDK-8273914, to continue to exercise the invokedynamic behaviour that
+ * JDK-8174954 is testing.
+ */
+
+package  p5;
+
+super public class c5
+	version 55:0
+{
+  public Method "<init>":"()V"
+	stack 1 locals 1
+  {
+		aload_0;
+		invokespecial	Method java/lang/Object."<init>":"()V";
+		return;
+  }
+  public Method method5:"(Lp2/c2;)V"
+	stack 2 locals 2
+  {
+		getstatic	Field java/lang/System.out:"Ljava/io/PrintStream;";
+		aload_1;
+		invokedynamic	InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Lp2/c2;)Ljava/lang/String;" {
+			String "In c5\'s method5 with param = 
"
+		};
+		invokevirtual	Method java/io/PrintStream.println:"(Ljava/lang/String;)V";
+		return;
+  }
+  public Method methodAddReadEdge:"(Ljava/lang/Module;)V"
+	stack 2 locals 2
+  {
+		ldc	class c5;
+		invokevirtual	Method java/lang/Class.getModule:"()Ljava/lang/Module;";
+		aload_1;
+		invokevirtual	Method java/lang/Module.addReads:"(Ljava/lang/Module;)Ljava/lang/Module;";
+		pop;
+		return;
+  }
+
+  public static final InnerClass Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles;
+
+} // end Class c5

test/hotspot/jtreg/runtime/modules/AccessCheck/p7/c7.jasm

@@ -0,0 +1,113 @@
+/*
+ * Copyright (c) 2021, Google LLC. 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 input for the fix for JDK-8174954, which checks for an expected
+ * IllegalAccessError when the parameter type of an invokedynamic is
+ * inaccessible.
+ *
+ * The test assumes that given the string concatenation expression "" + param,
+ * javac generates an invokedynamic that uses the specific type of param. The
+ * fix for JDK-8273914 make javac eagerly convert param to a String before
+ * passing it to the invokedynamic call, which avoids the accessibility issue
+ * the test is trying to exercise.
+ *
+ * This jasm file contains the bytecode javac generated before the fix for
+ * JDK-8273914, to continue to exercise the invokedynamic behaviour that
+ * JDK-8174954 is testing.
+ */
+
+package  p7;
+
+super public class c7
+	version 55:0
+{
+  public Method "<init>":"()V"
+	stack 1 locals 1
+  {
+		aload_0;
+		invokespecial	Method java/lang/Object."<init>":"()V";
+		return;
+  }
+  public Method method7:"(Lp2/c2;Ljava/lang/Module;)V"
+	stack 3 locals 4
+  {
+		try t0;
+		getstatic	Field java/lang/System.out:"Ljava/io/PrintStream;";
+		aload_1;
+		invokedynamic	InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Lp2/c2;)Ljava/lang/String;" {
+			String "In c7\'s method7 with param = 
"
+		};
+		invokevirtual	Method java/io/PrintStream.println:"(Ljava/lang/String;)V";
+		new	class java/lang/RuntimeException;
+		dup;
+		ldc	String "c7 failed to throw expected IllegalAccessError";
+		invokespecial	Method java/lang/RuntimeException."<init>":"(Ljava/lang/String;)V";
+		athrow;
+		endtry t0;
+		catch t0 java/lang/IllegalAccessError;
+		stack_frame_type stack1;
+		stack_map class java/lang/IllegalAccessError;
+		astore_3;
+		aload_0;
+		aload_2;
+		invokevirtual	Method methodAddReadEdge:"(Ljava/lang/Module;)V";
+		try t1;
+		getstatic	Field java/lang/System.out:"Ljava/io/PrintStream;";
+		aload_1;
+		invokedynamic	InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Lp2/c2;)Ljava/lang/String;" {
+			String "In c7\'s method7 with param = 
"
+		};
+		invokevirtual	Method java/io/PrintStream.println:"(Ljava/lang/String;)V";
+		endtry t1;
+		goto	L61;
+		catch t1 java/lang/IllegalAccessError;
+		stack_frame_type stack1;
+		stack_map class java/lang/IllegalAccessError;
+		astore_3;
+		new	class java/lang/RuntimeException;
+		dup;
+		aload_3;
+		invokevirtual	Method java/lang/IllegalAccessError.getMessage:"()Ljava/lang/String;";
+		invokedynamic	InvokeDynamic REF_invokeStatic:Method java/lang/invoke/StringConcatFactory.makeConcatWithConstants:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;":makeConcatWithConstants:"(Ljava/lang/String;)Ljava/lang/String;" {
+			String "Unexpected IllegalAccessError: 
"
+		};
+		invokespecial	Method java/lang/RuntimeException."<init>":"(Ljava/lang/String;)V";
+		athrow;
+	L61:	stack_frame_type same;
+		return;
+  }
+  public Method methodAddReadEdge:"(Ljava/lang/Module;)V"
+	stack 2 locals 2
+  {
+		ldc	class c7;
+		invokevirtual	Method java/lang/Class.getModule:"()Ljava/lang/Module;";
+		aload_1;
+		invokevirtual	Method java/lang/Module.addReads:"(Ljava/lang/Module;)Ljava/lang/Module;";
+		pop;
+		return;
+  }
+
+  public static final InnerClass Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles;
+
+} // end Class c7

test/langtools/tools/javac/StringConcat/StringAppendEvaluatesInOrder.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2021, Google LLC. 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     8273914
+ * @summary Indy string concat changes order of operations
+ *
+ * @clean *
+ * @compile -XDstringConcat=indy              StringAppendEvaluatesInOrder.java
+ * @run main StringAppendEvaluatesInOrder
+ *
+ * @clean *
+ * @compile -XDstringConcat=indyWithConstants StringAppendEvaluatesInOrder.java
+ * @run main StringAppendEvaluatesInOrder
+ *
+ * @clean *
+ * @compile -XDstringConcat=inline            StringAppendEvaluatesInOrder.java
+ * @run main StringAppendEvaluatesInOrder
+ */
+
+public class StringAppendEvaluatesInOrder {
+    static String test() {
+        StringBuilder builder = new StringBuilder("foo");
+        int i = 15;
+        return "Test: " + i + " " + (++i) + builder + builder.append("bar");
+    }
+
+    static String compoundAssignment() {
+        StringBuilder builder2 = new StringBuilder("foo");
+        Object oo = builder2;
+        oo += "" + builder2.append("bar");
+        return oo.toString();
+    }
+
+    public static void main(String[] args) throws Exception {
+        assertEquals(test(), "Test: 15 16foofoobar");
+        assertEquals(compoundAssignment(), "foofoobar");
+    }
+
+    private static void assertEquals(String actual, String expected) {
+      if (!actual.equals(expected)) {
+        throw new AssertionError("expected: " + expected + ", actual: " + actual);
+      }
+    }
+}