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

[Lindenmaier, Goetz]

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