[9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some java.sql tests
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed Oct 26 12:37:28 UTC 2016
> 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.
src/share/vm/c1/c1_LIRGenerator.cpp:
+ bool optimized = x->target_is_final(); // implies that it is loaded too.
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?
Also, please, add a comment in CompiledIC::compute_monomorphic_entry why
static_bound isn't relevant anymore.
Otherwise, looks good.
Best regards,
Vladimir Ivanov
> 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