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

[David Holmes]

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