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

[David Holmes]

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.

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
>>>>>>
>>>
>