[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 12:43:25 UTC 2016
I have updated and rewritten the comments in CompiledIC.cpp.
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.
Regards,
Nils
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
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20161103/742a0201/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list