[9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some java.sql tests
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu Nov 3 15:53:16 UTC 2016
> Webrev: http://cr.openjdk.java.net/~neliasso/8160543/webrev.13/
>
> One could argue that the case of a not optimized but static_bound call
> is a premature optimization that could be removed, but I would need some
> perf numbers before touching that. Fixing this bug is good enough at the
> moment.
It seems to me that your fix does exactly that: removes the case when a
statically bound method wasn't optimized by a compiler.
First of all, IC code is shared between C1 & C2. Do your changes affect
C2-generated code in any way (I hope not)?
Based on your previous explanation, I assume that for non-static methods
"vtable_index() == nonvirtual_vtable_index" condition is true only for
final methods.
After looking more at static_bound I noticed the following: the flag is
true not only for final methods, but for virtual methods of final class
[1]. is_optimized flag doesn't cover that case.
Extend optimized check to cover that?
+ bool optimized = x->target_is_final(); // implies that it is loaded too.
Also, should the following line be adjusted as well?
info.set_compiled_entry(entry, (static_bound || is_optimized) ?
NULL : receiver_klass(), is_optimized);
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