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

[Kris Mok]

I'm fine with that. Thank you, Vladimir.

- Kris

On 2012-6-27, at 5:53, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

> 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