review (S) for 6920293: OptimizeStringConcat causing core dumps

[Vladimir Kozlov]

Good.

Vladimir

Tom Rodriguez wrote:
> I had to change this a little bit since I was putting the cast in the wrong place.  The old change was casting the Phi to NotNull when it should have been casting the null checked value before it was merged into the phi.  It's acceptable to put it where I did but it's more correct to put it on the value instead of the phi.  The fix operates the same otherwise.  Could I get a rereview?
> 
> tom
> 
> On Feb 4, 2010, at 2:39 AM, Christian Thalinger wrote:
> 
>> On 02/ 3/10 10:41 PM, Tom Rodriguez wrote:
>>> http://cr.openjdk.java.net/~never/6920293
>>>
>>> 6920293: OptimizeStringConcat causing core dumps
>>> Reviewed-by:
>>>
>>> The code for the idiom a == null ? "null" : a) in OptimizeStringConcat
>>> was putting an explicit NOTNULL on the result Phi which allowed the
>>> optimizer to move some loads above the null check which caused a
>>> crash.  The fix is to use the same idiom the parser uses which is to
>>> cast the resulting value to be notnull and leave the phi type alone.
>>> Tested with failing test from report.  I also made some changes to
>>> error reporting to make the crash output more useful.  Currently if we
>>> don't find an implicit exception handler we die inside the VM and
>>> don't print out much useful information.  Instead we should simply
>>> return null and let the VMError machinery produce a normal crash dump.
>>> I also fixed the formatting for the register in 64 bit mode on solaris
>>> to match the linux formatting.
>>>
>>> src/share/vm/opto/stringopts.cpp
>>> src/share/vm/runtime/sharedRuntime.cpp
>>> src/share/vm/code/nmethod.cpp
>>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>> Looks good.  -- Christian
>