Request for reviews (S): 7179138: Incorrect result with String concatenation optimization

[Vladimir Kozlov]

Thank you, Kris

Unfortunately I already pushed my fix.

I filed RFE 7179968: improve String concatenation optimization

There are other cases where we bailout and which could be fixed. We will combine 
those fixes.

Thanks,
Vladimir

Krystal Mok wrote:
> Hi Vladimir,
> 
> Sorry for being too late for this CR, but I'd like to suggest one 
> further change in StringConcat::argument_uncast():
> 
> diff -r 751bd303aa45 src/share/vm/opto/stringopts.cpp
> --- a/src/share/vm/opto/stringopts.cpp Tue Jun 26 09:06:16 2012 -0700
> +++ b/src/share/vm/opto/stringopts.cpp Wed Jun 27 03:50:45 2012 +0800
> @@ -172,7 +172,7 @@
>      int amode = mode(i);
>      if (amode == StringConcat::StringMode ||
>          amode == StringConcat::StringNullCheckMode) {
> -      arg = skip_string_null_check(arg);
> +      arg = skip_string_null_check(arg)->uncast();
>      }
>      return arg;
>    }
> 
> 
> This could help situations where there are extra levels of toString() 
> calls in the way, e.g.
> 
> public class MultipleUse {
>   public static String foo() {
>     String s = "testing";
>     s = new StringBuilder(s).append(s).toString().toString();
>     s = new StringBuilder(s).append(s).toString();
>     return s;
>   }
> 
>   public static void main(String[] args) throws Exception {
>     final String s = "testing";
>     for (int i = 0; i < 20000; i++) {
>       if (!(s + s + s + s).equals(foo())) {
>         System.out.println("bad string concat");
>       }
>     }
>     System.out.println("done");
>     System.in.read();
>   }
> }
> 
> By adding the uncast() call, coalescing string concats would still work 
> in this case.
> This should still be safe because argument_uncast() is only used when 
> coalesing string concats, and that the result of a SB.toString() cannot 
> be null.
> 
> Regards,
> Kris
> 
> On Tue, Jun 26, 2012 at 7:19 AM, Vladimir Kozlov 
> <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote:
> 
>     http://cr.openjdk.java.net/~__kvn/7179138/webrev
>     <http://cr.openjdk.java.net/~kvn/7179138/webrev>
> 
>     7179138: Incorrect result with String concatenation optimization
> 
>     The problem happens when one StringBuilder append/toString sequence
>     uses the result of previous StringBuilder sequence and both
>     sequences are merged by string concatenation optimization.
>     Additional condition is the usage of String.valueOf(s) call as
>     argument of StringBuilder constructor (which is implicitly generated
>     by Eclipse java compiler).
> 
>     In normal case, when toString() result is directly referenced by the
>     constructor, string concatenation optimization will use input
>     arguments of previous StringBuilder append/toString sequence as
>     additional arguments of merged sequence instead of toString() result
>     itself. It is done because string concatenation optimization
>     replaces the original code with new one and it will throw away
>     original calls, including intermediate toString() calls.
> 
>     The problem with bug's case is toString() result is separated by Phi
>     node from diamond shaped code from valueOf(s) method (s==null ?
>     "null" : s) and it is kept as argument to new String concatenation
>     code. But the input to valueOf(s) check become dead after the call
>     to toString() is removed from graph. As result "null" string is used
>     incorrectly instead.
> 
>     The fix is to look for diamond shaped code which checks for NULL the
>     result of toString() call and look through it as we do now for
>     directly referenced toString() results.
> 
>     I also fixed call nodes elimination code which did not remove
>     Initialize nodes from merged StringBuilder sequences. Currently it
>     removes only first one.
> 
>     Added regression tests from bug report.
> 
>     Tested with Hotspot regression tests, jdk java/lang tests, CTW.
> 
>     Thanks,
>     Vladimir
> 
>