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

[Lois Foltan]

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