RFR 8056900: Enhance NoClassDefFound exception messaging

[harold seigel]

Thanks Serguei!  Since it's just a test I'll leave it as is.

Harold


On 5/3/2017 5:24 PM, serguei.spitsyn at oracle.com wrote:
> Hi Harold,
>
> Thank you for the update.
> It looks great!
>
> One tip is that you can pre-define the static String constants:
>   "Class name exceeds maximum length of "
>   "defineClass did not throw expected NoClassDefFoundError".
>
>
> But I leave it up to you.
> No webrev is needed if you decide to tweak it.
>
> Thanks,
> Serguei
>
>
> On 5/3/17 14:06, harold seigel wrote:
>> Hi,
>>
>> Please review this updated RFR that includes test cases for the JNI 
>> modifications.
>>
>> RFR: http://cr.openjdk.java.net/~hseigel/bug_8056900.3/webrev/index.html
>>
>> Thanks, Harold
>>
>>
>> On 5/3/2017 3:39 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Harold,
>>>
>>> The fix looks good to me.
>>> The test does not cover all the product changes.
>>> Is it possible to extend it to check the JNI FindClass as well?
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>> On 5/3/17 07:23, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this updated webrev: 
>>>> http://cr.openjdk.java.net/~hseigel/bug_8056900.2/webrev/index.html
>>>>
>>>> It includes the below changes suggested by David.
>>>>
>>>> Thanks, Harold
>>>>
>>>>
>>>> On 5/3/2017 12:32 AM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 3/05/2017 4:29 AM, harold seigel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this small JDK-10 change to enhance 
>>>>>> NoClassDefFoundError
>>>>>> exception messages.
>>>>>>
>>>>>> Open Webrev:
>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8056900/webrev/index.html
>>>>>
>>>>> A few comments:
>>>>>
>>>>> In src/share/vm/oops/instanceKlass.cpp
>>>>>
>>>>> 517     // This is for CDS dumping phase only -- we use the 
>>>>> in_error_state to indicate that
>>>>> 518     // the class has failed verification.
>>>>>
>>>>> That comment is somewhat misleading. The is_in_error_state() 
>>>>> indicates that class initialization failed, not verification. 
>>>>> However CDS dumping also uses it to indicate if a supertype was 
>>>>> is_in_error_state(). I think using:
>>>>>
>>>>> 527                        "Verification failed for class %s",
>>>>>
>>>>> would be confusing to an end user trying to figure out how a 
>>>>> perfectly valid class could fail verification! I suggest the more 
>>>>> direct:
>>>>>
>>>>> "Class %s, or one of its supertypes, failed class initialization"
>>>>>
>>>>> ---
>>>>>
>>>>> src/share/vm/prims/jni.cpp
>>>>>
>>>>> Suggestion:
>>>>>
>>>>> "Class name exceeds maximum length of %d: %s", ...
>>>>>
>>>>> in both cases.
>>>>>
>>>>> 386 THROW_MSG_0(vmSymbols::java_lang_NoClassDefFoundError(), 
>>>>> "Class name is null");
>>>>>
>>>>> Suggestion: "No class name given"
>>>>>
>>>>> ---
>>>>>
>>>>> Ditto for src/share/vm/prims/jvm.cpp changes.
>>>>>
>>>>> ---
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>
>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8056900
>>>>>>
>>>>>> The change was tested with JCK tests, the JTreg hotspot, java/io,
>>>>>> java/lang, java/util and other tests, the RBT tier2 -tier5 tests, 
>>>>>> the
>>>>>> co-located NSK tests, JPRT, and with the new test provided with 
>>>>>> the change.
>>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>
>>>
>>
>