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

[harold seigel]

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?

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