[9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some java.sql tests
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Mon Nov 7 14:07:05 UTC 2016
>>
>> My point was that for "effectively final" methods (holder class is
>> final) UEP will be used even if a method is loaded - it won't be
>> marked as optimized (since it's non-final method). It seems C2 may be
>> affected as well.
>
> I had to go back and double check:
>
> bool optimized = x->target_is_loaded() && x->target_is_final();
> [...]
> In C1_LIRGenerator:
> if (x->code() == Bytecodes::_invokespecial || optimized) {
> __ call_opt_virtual(target, receiver, result_register,
>
> Where:
> set_flag(TargetIsFinalFlag, target_is_loaded() &&
> target->is_final_method());
> And:
> bool is_final_method() const { return is_final()
> || holder()->is_final(); }
>
> So both final method and effectively final methods will be
> call_opt_virtual's.
>
> Webrev:
> http://cr.openjdk.java.net/~neliasso/8160543/webrev.15/
Looks good then!
Best regards,
Vladimir Ivanov
> I expanded 'optimized' in c1_LIRGenerator.cpp into its use, but removed
> x->target_is_loaded, since it is already checked by target_is_final.
>
> Regards,
> Nils
>
>>
>>> CallableStatementTest covers exactly this case. Class is loaded and
>>> final, but there is no ciMethod for target method yet.
>>>
>>>>
>>>> Also, should the following line be adjusted as well?
>>>>
>>>> info.set_compiled_entry(entry, (static_bound || is_optimized) ?
>>>> NULL : receiver_klass(), is_optimized);
>>>
>>> No, the static_bound calls are still special treated as optimized calls,
>>> just using the unverified entry point.
>>
>> Got it.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>>> [1]
>>>> bool Method::can_be_statically_bound(AccessFlags class_access_flags)
>>>> const {
>>>> if (is_final_method(class_access_flags)) return true;
>>>> return vtable_index() == nonvirtual_vtable_index;
>>>> }
>>>>
>>>> bool Method::is_final_method(AccessFlags class_access_flags) const {
>>>> if (is_overpass() || is_default_method()) return false;
>>>> return is_final() || class_access_flags.is_final();
>>>> }
>>>>
>>>>> On 2016-11-01 17:34, Nils Eliasson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2016-10-26 14:37, Vladimir Ivanov wrote:
>>>>>>>> http://cr.openjdk.java.net/~neliasso/8160543/webrev.11
>>>>>>>
>>>>>>> src/share/vm/c1/c1_GraphBuilder.cpp:
>>>>>>>
>>>>>>> + assert(will_link == target->is_loaded(), "Check");
>>>>>>>
>>>>>>> Please, add meaningful assert message or leave it empty.
>>>>>>
>>>>>> Removed comment.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> src/share/vm/c1/c1_LIRGenerator.cpp:
>>>>>>>
>>>>>>> + bool optimized = x->target_is_final(); // implies that it is
>>>>>>> loaded too.
>>>>>>
>>>>>> What do you suggest here?
>>>>>>
>>>>>>>
>>>>>>> src/share/vm/code/compiledIC.cpp:
>>>>>>>
>>>>>>> - if (static_bound || is_optimized) {
>>>>>>> + if (is_optimized) {
>>>>>>>
>>>>>>> It seems static_bound == true not only for final methods:
>>>>>>> methodHandle SharedRuntime::resolve_sub_helper(JavaThread *thread,
>>>>>>> ...
>>>>>>> bool static_bound =
>>>>>>> call_info.resolved_method()->can_be_statically_bound();
>>>>>>>
>>>>>>> bool Method::can_be_statically_bound(AccessFlags class_access_flags)
>>>>>>> const {
>>>>>>> if (is_final_method(class_access_flags)) return true;
>>>>>>> return vtable_index() == nonvirtual_vtable_index;
>>>>>>> }
>>>>>>>
>>>>>>> Maybe mark other cases (but when a method is loaded) w/ optimized
>>>>>>> flag as well?
>>>>>>
>>>>>> I tested this and it is not enough either. When optimized is true -
>>>>>> the callee is final and it was loaded, and then we know we can use
>>>>>> the
>>>>>> verified entry. Otherwise we can't.
>>>>>>
>>>>>> There is a comment a few lines down I didn't see before, it explains
>>>>>> well why static_bound was used. I will change the comment.
>>>>>>
>>>>>> // Note: the following problem exists with Compiler1:
>>>>>> // - at compile time we may or may not know if the destination
>>>>>> is final
>>>>>> // - if we know that the destination is final, we will emit an
>>>>>> optimized
>>>>>> // virtual call (no inline cache), and need a Method* to make
>>>>>> a call
>>>>>> // to the interpreter
>>>>>> >> // - if we do not know if the destination is final, we emit a
>>>>>> standard
>>>>>> // virtual call, and use CompiledICHolder to call
>>>>>> interpreted code
>>>>>> // (no static call stub has been generated)
>>>>>> // However in that case we will now notice it is static_bound
>>>>>> // and convert the call into what looks to be an optimized
>>>>>> // virtual call. This causes problems in verifying the IC
>>>>>> because
>>>>>> // it look vanilla but is optimized. Code in
>>>>>> is_call_to_interpreted
>>>>>> // is aware of this and weakens its asserts.
>>>>>>
>>>>>> This static_bound case is the one at the arrows. It is wrong because
>>>>>> if the call was final and the target was loaded - optimized would be
>>>>>> set. If it was not loaded, then we couldn't emit a null check, and we
>>>>>> must use unverified entry even if we later learn it was final. The
>>>>>> same goes for the alternative vtable check.
>>>>>>
>>>>>> I use the CallableStatementTests (listed in the bug report) to
>>>>>> wrap my
>>>>>> head around this. The target method is not loaded at compile time,
>>>>>> but
>>>>>> we will find it final in compute_monomorphic.
>>>>>>
>>>>>>> Also, please, add a comment in CompiledIC::compute_monomorphic_entry
>>>>>>> why static_bound isn't relevant anymore.
>>>>>>>
>>>>>>
>>>>>> Will do. I also must look at the other used of static_bound and
>>>>>> see if
>>>>>> they are still relevant.
>>>>>>
>>>>>>> Otherwise, looks good.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>
>>>>>> Thanks for having a look at this!
>>>>>>
>>>>>> I will post a new webrev.
>>>>>>
>>>>>> //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