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

[Lindenmaier, Goetz]

Hi Nils, 

the webrev still looks good.  

> I removed the assert because it will always hold. 
I would probably keep the assert like this:
assert(!target->is_loaded() || klass->is_loaded(), "loaded target must imply loaded klass");

> I may remove some more
> klass->is_loaded() calls but I need to test it thoroughly first.
Yes, I think the two below could be removed safely. Or do you expect
concurrent changes of this information?  See below. 

Best regards,
  Goetz.


diff -r 30e7565e278d src/share/vm/c1/c1_GraphBuilder.cpp
--- a/src/share/vm/c1/c1_GraphBuilder.cpp       Mon Oct 17 10:50:54 2016 +0200
+++ b/src/share/vm/c1/c1_GraphBuilder.cpp       Mon Oct 17 11:03:40 2016 +0200
@@ -1814,6 +1814,7 @@
   const Bytecodes::Code bc_raw = stream()->cur_bc_raw();
   assert(declared_signature != NULL, "cannot be null");
   assert(will_link == target->is_loaded(), "Check");
+  assert(!target->is_loaded() || klass->is_loaded(), "Loaded target must imply loaded klass.");

   ciInstanceKlass* klass = target->holder();

@@ -1863,7 +1864,7 @@
   ciMethod* cha_monomorphic_target = NULL;
   ciMethod* exact_target = NULL;
   Value better_receiver = NULL;
-  if (UseCHA && DeoptC1 && klass->is_loaded() && target->is_loaded() &&
+  if (UseCHA && DeoptC1 && klass->is_loaded() &&
       !(// %%% FIXME: Are both of these relevant?
         target->is_method_handle_intrinsic() ||
         target->is_compiled_lambda_form()) &&
@@ -1983,8 +1984,7 @@
   }

   // check if we could do inlining
-  if (!PatchALot && Inline &&
-      klass->is_loaded() && target->is_loaded() &&
+  if (!PatchALot && Inline && klass->is_loaded() &&
       (klass->is_initialized() || klass->is_interface() && target->holder()->is_initialized())
       && !patch_for_appendix) {
     // callee is known => check if we have static binding



> -----Original Message-----
> From: Nils Eliasson [mailto:nils.eliasson at oracle.com]
> Sent: Donnerstag, 13. Oktober 2016 15:56
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-compiler-
> dev at openjdk.java.net compiler <hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: [9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in
> some java.sql tests
> 
> 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