RFR(10)(XXS): 8175341: "java/util/Arrays/ParallelPrefix.java" Crash Internal Error ...diagnosticCommand.cpp...assert(k != __null) failed: FinalizerHistogram class is not accessible

[Chris Plummer]

On 3/7/17 1:38 AM, serguei.spitsyn at oracle.com wrote:
> On 3/6/17 23:15, David Holmes wrote:
>> Sorry for the delay TB decided not to pull any email down from my 
>> openjdk folder for 3 hours :(
>>
>> On 7/03/2017 3:30 PM, Chris Plummer wrote:
>>> On 3/6/17 6:25 PM, David Holmes wrote:
>>>> On 7/03/2017 12:14 PM, Chris Plummer wrote:
>>>>> On 3/6/17 5:58 PM, David Holmes wrote:
>>>>>> On 7/03/2017 11:51 AM, David Holmes wrote:
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> On 7/03/2017 10:57 AM, Chris Plummer wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the following simple fix. Explanation of the 
>>>>>>>> cause and
>>>>>>>> the
>>>>>>>> fix are in the CR. Testing so far has only been done with
>>>>>>>> serviceability/dcmd/gc/FinalizerInfoTest.java as described in the
>>>>>>>> CR. I
>>>>>>>> think the addition of JPRT push testing will be sufficient.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8175341/webrev.00/webrev.hotspot/ 
>>>>>>>>
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8175341
>>>>>>>
>>>>>>> Looking at the other potentially-exception-throwing actions in that
>>>>>>> DCmd
>>>>>>> and others, I think a simple delete of the assert and change:
>>>>>>>
>>>>>>>  425     vmSymbols::finalizer_histogram_klass(), THREAD);
>>>>>>>
>>>>>>> to
>>>>>>>
>>>>>>>  425     vmSymbols::finalizer_histogram_klass(), CHECK);
>>>>>>>
>>>>>>> would suffice. Something upstream has to handle potential errors
>>>>>>> and the
>>>>>>> pending exception in any case.
>>>>>>
>>>>>> Oops! This explains why no exception:
>>>>>>
>>>>>> 424   Klass* k = SystemDictionary::resolve_or_null(
>>>>>>  425     vmSymbols::finalizer_histogram_klass(), THREAD);
>>>>>>
>>>>>> it uses resolve_or_null, not resolve_or_fail! I think it should use
>>>>>> the latter and simply throw like the other Dcmds.
>>>>>>
>>>>> Ok. That seems to work. Bit of a hurry here before I step out, so
>>>>> trimmed diff is inline:
>>>>>
>>>>> -  Klass* k = SystemDictionary::resolve_or_null(
>>>>> -    vmSymbols::finalizer_histogram_klass(), THREAD);
>>>>> -  assert(k != NULL, "FinalizerHistogram class is not accessible");
>>>>> +  Klass* k = SystemDictionary::resolve_or_fail(
>>>>> +    vmSymbols::finalizer_histogram_klass(), true, CHECK);
>>>>> +  if (k == NULL) {
>>>>> +    return;
>>>>> +  }
>>>>>
>>>>> I'm a bit unclear about THREAD vs CHECK. They both seem to work. Not
>>>>> sure which is correct here.
>>>>
>>>> THREAD is just a reference to the current thread (as passed down the
>>>> call chain). CHECK automatically expands into a pending exception
>>>> check plus a return. So your "if (k==null) ..." is unreachable with
>>>> CHECK - as k will only be NULL if an exception is pending.
>>> Ok. This seems to work as well:
>>>
>>> -  Klass* k = SystemDictionary::resolve_or_null(
>>> -    vmSymbols::finalizer_histogram_klass(), THREAD);
>>> -  assert(k != NULL, "FinalizerHistogram class is not accessible");
>>> +  Klass* k = SystemDictionary::resolve_or_fail(
>>> +    vmSymbols::finalizer_histogram_klass(), true, CHECK);
>>
>> Yep looks good to me.
>
> +1
>
> Thanks,
> Serguei
David and Serguie, thanks for the reviews!

Chris
>
>
>>
>> Thanks,
>> David
>> -----
>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> When I forced an exception by dropping the last character of the 
>>>>> class
>>>>> name, here's what I see in the test output:
>>>>>
>>>>>  stderr: [java.lang.NoClassDefFoundError:
>>>>> java/lang/ref/FinalizerHistogra
>>>>> ]
>>>>>
>>>>> cheers,
>>>>>
>>>>> Chris
>>>>>> David
>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>
>>>
>