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

Nils Eliasson nils.eliasson at oracle.com
Thu Oct 20 09:27:54 UTC 2016 

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

More information about the hotspot-compiler-dev mailing list