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

[Vicente Romero]

Hi,

On 06/06/2018 11:50 AM, B. Blaser wrote:
> 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'

just pushed it a while ago :( but yes I like your suggestion I will 
leave a note for a future improvement

> instead.
>
> Cheers,
> Bernard

Thanks,
Vicente

>
>
> 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();
>                       }
>                   }