RFR: 8204322: "+=" applied to String operands can provoke side effects

[B. Blaser]

Hi Vicente,

On 6 June 2018 at 15:23, Vicente Romero <vicente.romero at oracle.com> wrote:
> Please review the fix for [1] at [2]. Javac was generating incorrect code
> for the case when the `+=` operand was applied to Strings and it is visible
> for test cases like:
>
> public class Test {
>    static int test() {
>        System.out.println("evaluated");
>        return 0;
>    }
>
>    public static void main(String[] args) {
>        String[] array = {""};
>        array[test()] += "a";
>    }
> }
>
> the string "evaluated" is printed twice. Some items were missing in the
> stack, the solution is to duplicate those missing items plus not to evaluate
> the lhs expression twice.
>
> Thanks in advance for the comments,
> Vicente
>
> Thanks to Aleksey Shipilev for the great regression test.
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8204322
> [2] http://cr.openjdk.java.net/~vromero/8204322/webrev.01/
>

I hope I'm not too late, but I was also trying to solve this problem
(here under).
So, our solutions are quite identical (yours being a bit better), but
I think you could avoid 'boolean first' using 'arg == args.head'
instead.

Cheers,
Bernard


diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
@@ -261,18 +261,18 @@
         public Item makeConcat(JCTree.JCAssignOp tree) {
             List<JCTree> args = collectAll(tree.lhs, tree.rhs);
             Item l = gen.genExpr(tree.lhs, tree.lhs.type);
-            emit(args, tree.type, tree.pos());
+            emit(l, args, tree.type, tree.pos());
             return l;
         }

         @Override
         public Item makeConcat(JCTree.JCBinary tree) {
             List<JCTree> args = collectAll(tree.lhs, tree.rhs);
-            emit(args, tree.type, tree.pos());
+            emit(null, args, tree.type, tree.pos());
             return gen.getItems().makeStackItem(syms.stringType);
         }

-        protected abstract void emit(List<JCTree> args, Type type,
JCDiagnostic.DiagnosticPosition pos);
+        protected abstract void emit(Item lhs, List<JCTree> args,
Type type, JCDiagnostic.DiagnosticPosition pos);

         /** Peel the argument list into smaller chunks. */
         protected List<List<JCTree>> split(List<JCTree> args) {
@@ -318,7 +318,7 @@
         }

         /** Emit the indy concat for all these arguments, possibly
peeling along the way */
-        protected void emit(List<JCTree> args, Type type,
JCDiagnostic.DiagnosticPosition pos) {
+        protected void emit(Item lhs, List<JCTree> args, Type type,
JCDiagnostic.DiagnosticPosition pos) {
             List<List<JCTree>> split = split(args);

             for (List<JCTree> t : split) {
@@ -333,7 +333,13 @@
                     } else {
                         dynamicArgs.add(sharpestAccessible(arg.type));
                     }
-                    gen.genExpr(arg, arg.type).load();
+
+                    if (lhs != null && arg == args.head) {
+                        lhs.duplicate();
+                        lhs.load();
+                    }
+                    else
+                        gen.genExpr(arg, arg.type).load();
                 }

                 doCall(type, pos, dynamicArgs.toList());
@@ -402,7 +408,7 @@
         }

         @Override
-        protected void emit(List<JCTree> args, Type type,
JCDiagnostic.DiagnosticPosition pos) {
+        protected void emit(Item lhs, List<JCTree> args, Type type,
JCDiagnostic.DiagnosticPosition pos) {
             List<List<JCTree>> split = split(args);

             for (List<JCTree> t : split) {
@@ -433,7 +439,13 @@
                         // Ordinary arguments come through the
dynamic arguments.
                         recipe.append(TAG_ARG);
                         dynamicArgs.add(sharpestAccessible(arg.type));
-                        gen.genExpr(arg, arg.type).load();
+
+                        if (lhs != null && arg == args.head) {
+                            lhs.duplicate();
+                            lhs.load();
+                        }
+                        else
+                            gen.genExpr(arg, arg.type).load();
                     }
                 }