[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 13:48:00 UTC 2016
On 2016-11-04 00:03, 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/
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