RFR(M): 8165550: Add class loader names to ClassCastException message

[Rachel Protacio]

Updated webrev: http://cr.openjdk.java.net/~rprotacio/8165550.02/
     - added macro in ModuleEntry.hpp for "java.base" to address Lois' 
comment (opened enhancement 
https://bugs.openjdk.java.net/browse/JDK-8169734 for other existing cases)
     - added an assert instead of a NULL check for the class loader data
     - fixed NEW_RESOURCE_ARRAY_RETURN_NULL calls

Thanks!
Rachel

On 11/15/2016 9:45 AM, Lois Foltan wrote:
>
> On 11/15/2016 9:38 AM, harold seigel wrote:
>> Please note that "java.base" is hard coded in multiple places in the 
>> JVM.  For example:
>>
>>    src/share/vm/runtime/arguments.cpp:  if (strcmp(module_name,
>>    "java.base") == 0) {
>>
>>    src/share/vm/classfile/classLoader.cpp:        location =
>>    (*JImageFindResource)(_jimage, "java.base", get_jimage_version_s
>>
>>    src/share/vm/classfile/modules.cpp:                "Bad package name
>>    for module: java.base");
>>
>>    src/share/vm/oops/instanceKlass.cpp:
>>    assert(ModuleEntryTable::javabase_moduleEntry() != NULL, "java.base
>>    module is NULL");
>>
>> Should we enter a new bug for this or just ignore it?
>
> Sure, enter a bug to clean those up as well.
> Lois
>
>>
>> Thanks, Harold
>>
>>
>> On 11/15/2016 7:36 AM, Lois Foltan wrote:
>>>
>>> On 11/14/2016 4:00 PM, Rachel Protacio wrote:
>>>> New webrev: http://cr.openjdk.java.net/~rprotacio/8165550.01/
>>>
>>> Hi Rachel,
>>>
>>> Looks good.  One comment about hard coding the string "java.base" on 
>>> line #1989.  Please consider obtaining the string for the java.base 
>>> module from vmSymbols::java_base()->as_C_string().
>>>
>>> Thanks,
>>> Lois
>>>
>>>>
>>>> Corrected according to David's review. Comments below.
>>>>
>>>>
>>>> On 11/10/2016 6:24 PM, David Holmes wrote:
>>>>> Okay on with the review:
>>>>>
>>>>> src/share/vm/classfile/moduleEntry.cpp
>>>>>
>>>>> +     const char* loc = "";
>>>>> +     loc = location()->as_C_string();
>>>>>
>>>>> can just be:
>>>>>
>>>>> +     const char* loc = location()->as_C_string();
>>>>>
>>>> Fixed.
>>>>> ---
>>>>>
>>>>> src/share/vm/runtime/sharedRuntime.cpp
>>>>>
>>>>> 1941 const char* class_loader_and_module_name(Klass* klass, Klass* 
>>>>> base_klass) {
>>>>>
>>>>> I'm unclear in what way "base_klass" is a "base" class ?? And why, 
>>>>> if it is not an instance class must be it in java.base ??
>>>> I really meant "bottom" class, following the logic used to parse it 
>>>> out. I've moved that internal to the function now. So if the klass 
>>>> is an instance klass or instance klass array, we look for further 
>>>> information. Otherwise (if it's a primitive array), it's from 
>>>> java.base.
>>>>>
>>>>> Nit: I don't think these need to be #defines nor have such verbose 
>>>>> names:
>>>>>
>>>>> 1936 #define CLASS_LOADER_NAME_DELIMITER "/"
>>>>> 1937 #define CLASS_LOADER_NAME_DELIMITER_LENGTH 
>>>>> strlen(CLASS_LOADER_NAME_DELIMITER)
>>>>>
>>>>> why not just
>>>>>
>>>>> const char* delim = "\";
>>>>> int delim_len = strlen(delim);
>>>>>
>>>>> Plus you are inconsistent with the handling of the "/" delimiter 
>>>>> versus the "@" delimiter.
>>>> Fixed.
>>>>>
>>>>> ---
>>>>>
>>>>>  test/runtime/modules/CCE_module_msg.java
>>>>>
>>>>> Nit: in the catch block from line #95 can you store getMessage() 
>>>>> into a local so it isn't called 5 times. Thanks. :)
>>>> Fixed too. Thanks!
>>>> Rachel
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Rachel
>>>>>>
>>>>>> On 11/10/2016 5:02 PM, David Holmes wrote:
>>>>>>> Hi Rachel,
>>>>>>>
>>>>>>> Just wondering, in the most complex case, exactly what will this 
>>>>>>> new
>>>>>>> message look like? We seem to be including module names as well as
>>>>>>> classloader names, and of course class names.
>>>>>>>
>>>>>>> Arguably we should only add the additional info when they differ
>>>>>>> between the types ie if both in same classloader and same module 
>>>>>>> then
>>>>>>> only report class name; if same loader but different module report
>>>>>>> module:class; if different loaders then report everything
>>>>>>> loader:module:class. Also not sure module version is needed at 
>>>>>>> all in
>>>>>>> this message.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>> On 11/11/2016 7:38 AM, Rachel Protacio wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this Jigsaw enhancement which adds the new format 
>>>>>>>> of class
>>>>>>>> loader names to VM ClassCastException messages. Includes an
>>>>>>>> updated/expanded jtreg test.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8165550
>>>>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8165550.00/
>>>>>>>>
>>>>>>>> Passes JPRT and jck vm and lang tests.
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> Rachel
>>>>>>
>>>>
>>>
>>
>