[9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some java.sql tests
Nils Eliasson
nils.eliasson at oracle.com
Thu Nov 3 21:44:53 UTC 2016
On 2016-11-03 16:53, Vladimir Ivanov wrote:
>> 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.
No, statically bound methods will still end up as fixed calls, but to
the unverified entry point. Lets call them sub-optimized calls :)
>
> 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.
>
> 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. )
> 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.)
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.
>
> Best regards,
> Vladimir Ivanov
Thanks for your attention,
Nils
>
> [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