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

[Rachel Protacio]

Thanks for the review, David!

Rachel


On 11/16/2016 12:18 AM, David Holmes wrote:
> Looks good Rachel!
>
> No further comments from me.
>
> Thanks,
> David
>
> On 16/11/2016 6:49 AM, Rachel Protacio wrote:
>> 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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>