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

[Vladimir Ivanov]

Vladimir,

The changes look good for me.

I have a couple of small comments though:
  1) Don't you want to add some assertions into skip_string_null_check
to check the following precondition on value?
 149         // phi->region->if_proj->ifnode->bool
 150         BoolNode* b = value->in(0)->in(1)->in(0)->in(1)->as_Bool();

  2) Why do you check only result string length in the tests and not
it's content, since you know it? Moreover, "null" and "test" are both
4-char words :-)

Best regards,
Vladimir Ivanov

On 06/26/12 03:19, Vladimir Kozlov wrote:
> 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