[9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some java.sql tests

[Nils Eliasson]

Hi Goetz,

Thanks for the feedback!

I have been busy with a performance regression that this code triggered 
- JDK-8167656.

I removed the assert because it will always hold. I may remove some more 
klass->is_loaded() calls but I need to test it thoroughly first.

The indentation looks crappy in the webrev but ok in my editor, 
something with the diff I guess.

New webrev: http://cr.openjdk.java.net/~neliasso/8160543/webrev.10/

Regards,
Nils

On 2016-09-07 12:07, Lindenmaier, Goetz wrote:
> Hi Nils,
>
> I encountered this issue in our nightly jck test runs with -Xcomp.
> I applied your fix to the VM tested, and I can no more observe
> the error.
>
> Also, I had a look at the code. It looks good.
> The if around assert(klass->is_loaded(), "sanity"); could be merged into the assertion.
> Also, if this holds, a row of calls to klass->is_loaded() can be removed.
>
> Please fix the indentation in c1_GraphBuilder.cpp 2056ff.
>
> Thanks for fixing this,
>    Goetz.
>
>> -----Original Message-----
>> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
>> bounces at openjdk.java.net] On Behalf Of Nils Eliasson
>> Sent: Mittwoch, 31. August 2016 16:31
>> To: hotspot-compiler-dev at openjdk.java.net compiler <hotspot-compiler-
>> dev at openjdk.java.net>
>> Subject: [9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some
>> java.sql tests
>>
>> Hi,
>>
>> This is fixes for bug [1] JDK-8160543 "C1: Crash in java.lang.String.indexOf in
>> some java.sql tests" and [2] JDK-8154172 "NPE is thrown instead of linkage
>> error when invoking nonexistent method "
>>
>> * Description:
>>
>> Problem in bug #2: A method that is not loaded must not have null check at
>> the call. The unloaded method may not exist and then we may throw a NPE
>> on a null receiver before LinkageError and violate the VM spec.
>>
>> Problem in bug #1: A final method that is not loaded at compile time (the
>> final-property is unknown), but is actually loaded from another classloader
>> (and may already be compiled) must null check its receiver. The null check
>> can not be at the call site since it would violate #2. Instead the call will have to
>> use the target methods unverified entry point.
>>
>> An additional problem i encountered is that profiling requires a null check,
>> but if the method isn't loaded we can't add one. So we can't profile unloaded
>> methods.
>>
>> The solution to these problems shouldn't introduce any regression in the
>> normal use case. Unloaded methods is only common in the compiler when
>> using -Xcomp when the interpreter hasn't made sure everything is loaded. I
>> have made the trade-off that it is acceptable to have an performance
>> regression in the -Xcomp case in order to meet the VM specification.
>>
>> * Summary of changes:
>>
>> hotspot/src/share/vm/code/compiledIC.cpp
>>
>> -    if (static_bound || is_optimized) {
>> +    if (is_optimized) {
>>
>> static_bound is true if the method at resolve time is declared final. This is
>> uninteresting, we need to know if the call was known final at compile time.
>> is_optimized however is only true if the targets was loaded and was final at
>> compile time. This change makes sure that we get a call to the unverified
>> entry point if there was no null check emitted.
>>
>> hotspot/src/share/vm/c1/c1_GraphBuilder.cpp
>> Contains both changed and some simplifications. The is_resolved method
>> has been exploded, and redundant check was removed. The major change is
>> where we decide if a null check should be emitted and when profiling can be
>> added.
>>
>> * Testing
>>
>> These are some useful test I have run with and without -Xcomp and with and
>> without -XX:TieredStopAtLevel=1:
>> - jdk/test/java/sql/testng/test/sql/CallableStatementTests.java (for bug #1)
>> - JCK/BINC (binc02908m01 for bug #2 and all binc0500)
>> - hotspot/test/compiler/linkage/LinkageErrors.java
>>
>> I will await complete runs of hs-comp tier 0 - 5 before checkin.
>>
>> Please review,
>>
>> Regards,
>> Nils Eliasson
>>
>>
>>
>> Webrev: http://cr.openjdk.java.net/~neliasso/8160543/webrev.09/
>> Bug [1]: https://bugs.openjdk.java.net/browse/JDK-8160543
>> Bug [2]: https://bugs.openjdk.java.net/browse/JDK-8160383