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

[Lindenmaier, Goetz]

Hi Nils,

looks good now, thanks!

Best regards,
  Goetz.

> -----Original Message-----
> From: Nils Eliasson [mailto:nils.eliasson at oracle.com]
> Sent: Donnerstag, 20. Oktober 2016 11:28
> 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,
> 
> Agree, lets keep the assert and remove the redundant checks.
> 
> Added the assert as suggested.
> 
> In the if-statements it is the "klass->is_loaded()" that should be removed.
> 
> http://cr.openjdk.java.net/~neliasso/8160543/webrev.11
> 
> Best regards,
> Nils
> 
> 
> 
> On 2016-10-17 11:06, Lindenmaier, Goetz wrote:
> > 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