[9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some java.sql tests
Nils Eliasson
nils.eliasson at oracle.com
Mon Nov 7 14:44:25 UTC 2016
Thanks Vladimir!
I'll test overnight and push tomorrow.
Best regards,
Nils
On 2016-11-07 15:07, Vladimir Ivanov wrote:
>>>
>>> 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