RFR (XS): 8025566: EXCEPTION_ACCESS_VIOLATION in compiled by C1 String.valueOf method

[Christian Thalinger]

On Oct 4, 2013, at 3:43 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

> On 10/4/13 2:52 PM, Christian Thalinger wrote:
>> Here is the (hopefully) correct fix for this problem:
>> 
>> http://cr.openjdk.java.net/~twisti/8025566/webrev.01/
>> 
>> I've made ciMethod::ensure_method_counters() call Method::get_method_counters() because they did the same job.
>> 
>> For C1 I can provoke the failure situation and C1 bails out of the compilation just fine:
>> 
>>    1652  157       2       java.io.FilterInputStream::read (11 bytes)
>> compilation bailout: method counters allocation failed
>>    1652  157       2       java.io.FilterInputStream::read (11 bytes)   COMPILE SKIPPED: method counters allocation failed (retry at different tier)
>> 
>> For C2 I wasn't able to get to the point where the code in question is executed.  I opted for a more conservative way:  record a failure but continue the compile.  If we'd return straightaway the graph might be in a bad state and we could crash somewhere else.  When continuing the compile we can be sure the graph is sane and another failing() check somewhere will bail out for us eventually.
> 
> It is used only when C2 generates profiling code (SAP).

Ahh!  That explains why I couldn't trigger it.

> I think you should return from increment_and_test_invocation_counter() if counters_adr is NULL - don't generate increment code.

Ok.

> The code path after that has failing() check so I agree that you don't need to add new one.
> 
> There is other place missing NULL check: ciReplay::initialize()

Thanks for catching that.  I used Method::get_method_counters() there as well and a guarantee because if we  cannot allocate the method counters it doesn't make sense to continue.

http://cr.openjdk.java.net/~twisti/8025566/webrev.02/

> 
> Thanks,
> Vladimir
> 
>> 
>> On Oct 3, 2013, at 7:01 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>> 
>>> On 10/3/13 6:34 PM, Christian Thalinger wrote:
>>>> 
>>>> On Oct 3, 2013, at 4:15 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>> 
>>>>> Christian,
>>>>> 
>>>>> Can you explain how _metadata pointer could be NULL if it was just created and there is check h_m()->method_data() != NULL?
>>>> 
>>>> You are right.  Since we cannot reproduce this bug it's a guessing game.
>>>> 
>>>> Your comment made me go back and look again.  The reported error happened with a tier 2 compile and there is a different code path in LIRGenerator::increment_event_counter_impl() for CompLevel_limited_profile compiles:
>>>> 
>>>>   if (level == CompLevel_limited_profile) {
>>>>     address counters_adr = method->ensure_method_counters();
>>>>     counter_holder = new_pointer_register();
>>>>     __ move(LIR_OprFact::intptrConst(counters_adr), counter_holder);
>>>> 
>>>> Method::ensure_method_counters() tries to build a MethodCounters data structure if it's null but:
>>>> 
>>>>     counter = Method::build_method_counters(mh(), CHECK_AND_CLEAR_NULL);
>>>> 
>>>> it clears the exception and returns null!
>>> 
>>> Right. This is it! And the only place where result is checked for null callers of get_method_counters(). All other places which call build_method_counters() do not check.
>>> 
>>> Vladimir
>>> 
>>>> 
>>>> I really want C++ exceptions in the compiler…
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 10/2/13 8:17 PM, Christian Thalinger wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8025566
>>>>>> http://cr.openjdk.java.net/~twisti/8025566
>>>>>> 
>>>>>> 8025566: EXCEPTION_ACCESS_VIOLATION in compiled by C1 String.valueOf method
>>>>>> Reviewed-by:
>>>>>> 
>>>>>> The problem is in ciMethod::ensure_method_data().  The call to ciMethodData::load_data() can fail when the backing metadata pointer is null and return with a still empty method data.  So we have to check here for non-emptiness.
>>>>>> 
>>>>>> I also removed ciMethodData::set_mature().  This method was only used with the old JSR 292 implementation.
>>>>>> 
>>>> 
>>