[9] RFR (M): 8160543: C1: Crash in java.lang.String.indexOf in some java.sql tests
Nils Eliasson
nils.eliasson at oracle.com
Tue Nov 1 16:34:11 UTC 2016
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/20161101/9f1956b2/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list