[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 23:03:52 UTC 2016
Thanks for the clarifications, Nils.
> No, statically bound methods will still end up as fixed calls, but to
> the unverified entry point. Lets call them sub-optimized calls :)
Agree :-)
>>
>> First of all, IC code is shared between C1 & C2. Do your changes
>> affect C2-generated code in any way (I hope not)?
>
> The sharedRuntime code is shared :-/ It still needs to be done. If C2
> also can see calls to targets that are not loaded at compile time, but
> observable at resolve time - then it also will need this change. If it
> is the case that all calls to unloaded methods end up as uncommontraps -
> then this change will affect nothing for C2.
Yes, I expect C2 to issue uncommon traps for unloaded methods. But I'm
also concerned about the case with loaded methods as well. More details
follow.
>>
>> Based on your previous explanation, I assume that for non-static methods
>> "vtable_index() == nonvirtual_vtable_index" condition is true only for
>> final methods.
> Yes. Based om my understanding of the code, and the available comments -
> that is the case. I will do some additional experiments to verify it.
>>
>> 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.
> Correct. (In the CallableStatementTest-failure that path is triggered. )
Good.
>> Extend optimized check to cover that?
>> + bool optimized = x->target_is_final(); // implies that it is loaded
>> too.
>
> hmm... yes and no.
>
> No - because currently target_is_final means "method is loaded and
> final." If we include the class finalness then it would be "final -
> perhaps loaded". And then "optimized" won't imply having a null-check
> anymore. (Note-to-self: rename "optimized").
>
> Yes - that would make target_is_final what it says, and remove some
> surprise and complexity in the resolve stage. But then we need to
> include the knowledge about null-check in the Invoke. (And I need to
> investigate if we can remove static_bound, or if there still are cases
> that needs it.)
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.
> 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